Tomasz Majchrzak <tomasz.majchrzak@xxxxxxxxx> writes:
> Always allocate memory for all log entries to avoid a need for memory
> allocation when monitor requests to record a bad block.
>
> Also some extra checks added to make static code analyzer happy.
>
> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@xxxxxxxxx>
> ---
> super-intel.c | 158 +++++++++++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 112 insertions(+), 46 deletions(-)
Tomasz,
A couple of comments on this one:
First of all: *Always* provide a cover letter when submitting
multi-patch sets.
> diff --git a/super-intel.c b/super-intel.c
> index c146bbd..5d6d534 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -217,22 +217,24 @@ struct imsm_super {
> } __attribute__ ((packed));
>
> #define BBM_LOG_MAX_ENTRIES 254
> +#define BBM_LOG_MAX_LBA_ENTRY_VAL 256 /* Represents 256 LBAs */
> +#define BBM_LOG_SIGNATURE 0xABADB10C
I know the old code is messy with regard to this, but lets get it right
from now on. Numbers are lower-case and #define NAMES are upper case.
> @@ -785,6 +787,92 @@ static struct imsm_dev *get_imsm_dev(struct intel_super *super, __u8 index)
> return NULL;
> }
>
> +#if BYTE_ORDER == LITTLE_ENDIAN
> +static inline unsigned long long __le48_to_cpu(const struct bbm_log_block_addr
> + *addr)
> +{
> + return ((((__u64)addr->dw1) << 16) | addr->w1);
> +}
> +
> +static inline struct bbm_log_block_addr __cpu_to_le48(unsigned long long sec)
> +{
> + struct bbm_log_block_addr addr;
> +
> + addr.w1 = (__u16)(sec & 0xFFFF);
> + addr.dw1 = (__u32)((sec >> 16) & 0xFFFFFFFF);
> + return addr;
> +}
Again, 0xffff/0xffffffff
> +#elif BYTE_ORDER == BIG_ENDIAN
> +static inline unsigned long long __le48_to_cpu(const struct bbm_log_block_addr
> + *addr)
> +{
> + return ((((__u64)__le32_to_cpu(addr->dw1)) << 16) |
> + __le16_to_cpu(addr->w1));
> +}
> +
> +static inline struct bbm_log_block_addr __cpu_to_le48(unsigned long long sec)
> +{
> + struct bbm_log_block_addr addr;
> +
> + addr.w1 = __cpu_to_le16((__u16)(sec & 0xFFFF));
> + addr.dw1 = __cpu_to_le32((__u32)(sec >> 16) & 0xFFFFFFFF);
> + return addr;
> +}
Given that __cpu_to_le*()/__le*_to_cpu*() are no-ops on little-endian,
you don't really need two versions of these. The big-endian version
should suffice for both, which makes it far less cluttered. Unless I got
something wrong here of course.
> +#else
> +# error "unknown endianness."
> +#endif
> +
> +#ifndef MDASSEMBLE
> +/* get size of the bbm log */
> +static __u32 get_imsm_bbm_log_size(struct bbm_log *log)
> +{
> + if (!log || log->entry_count == 0)
> + return 0;
> +
> + return sizeof(log->signature) +
> + sizeof(log->entry_count) +
> + log->entry_count * sizeof(struct bbm_log_entry);
> +}
> +#endif /* MDASSEMBLE */
> +
> +/* allocate and load BBM log from metadata */
> +static int load_bbm_log(struct intel_super *super)
> +{
> + struct imsm_super *mpb = super->anchor;
> + __u32 bbm_log_size = __le32_to_cpu(mpb->bbm_log_size);
> +
> + super->bbm_log = malloc(sizeof(struct bbm_log));
> + if (!super->bbm_log)
> + return 1;
> +
> + if (bbm_log_size) {
> + struct bbm_log *log = (void *)mpb +
> + __le32_to_cpu(mpb->mpb_size) - bbm_log_size;
> + __u32 entry_count;
Put the assignment on a separate line please when it's this long.
> +
> + if (bbm_log_size < sizeof(log->signature) +
> + sizeof(log->entry_count))
> + return 2;
> +
> + entry_count = __le32_to_cpu(log->entry_count);
> + if ((__le32_to_cpu(log->signature) != BBM_LOG_SIGNATURE) ||
> + (entry_count > BBM_LOG_MAX_ENTRIES))
> + return 3;
> +
> + if (bbm_log_size !=
> + sizeof(log->signature) + sizeof(log->entry_count) +
> + entry_count * sizeof(struct bbm_log_entry))
> + return 4;
> +
> + memcpy(super->bbm_log, log, bbm_log_size);
> + } else {
> + super->bbm_log->signature = __cpu_to_le32(BBM_LOG_SIGNATURE);
> + super->bbm_log->entry_count = 0;
> + }
This part looks problematic - you don't clear super->bbm_log and if
bbm_log_size == 0 you end up leaving it with random data. Wouldn't it be
better to just use calloc()?
Jes
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
