[PATCH phosphor-host-ipmid] FRU validator CLI

[Patrick Williams]

On Wed, Oct 14, 2015 at 09:23:35AM -0500, OpenBMC Patches wrote:
> From: vishwabmc <vishwanath at in.ibm.com>
> 
> ---
>  fruvalidator.C | 252 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 252 insertions(+)
>  create mode 100644 fruvalidator.C
> 
> diff --git a/fruvalidator.C b/fruvalidator.C
> new file mode 100644
> index 0000000..5e2ffaf
> --- /dev/null
> +++ b/fruvalidator.C

> +unsigned char calculate_checksum(unsigned char *ptr, int len)
> +{
> +    // REFER :: http://coreipm.googlecode.com/svn-history/r40/trunk/coreipm/ipmi_pkt.c
> +    char checksum = 0;
> +    int byte = 0;
> +
> +    for(byte = 0; byte < len; byte++)
> +    {
> +        checksum += *ptr++;
> +    }
> +    
> +    return( -checksum );
> +}

With the 'refer' statement, you pointed us to code that is GPL licensed
but your file is not GPL license.  I know this is a simple algorithm.
Best to point to either the IPMI spec itself or a non-GPL licensed
example.  Hostboot has one and I bet skiboot has one as well.

> +    //common_header_t com_hdr;
> +    memset(common_hdr, 0x0, sizeof(common_hdr));

Is this a global?  Please do not reference globals.

Wait... this is an undefined variable.  Did you ask us to review code
that doesn't even compile?

> +
> +    // For now, this is user supplied in CLI. But this will be a parameter from
> +    // fru handler
> +    FILE *fru_file = fopen(fru_file_name,"rb");
> +    if(fru_file == NULL)
> +    {
> +        fprintf(stderr, "ERROR: opening:[%s]\n",fru_file_name);

Want to print out the perror too?  This is a general comment anytime a
syscall fails.

> +    // 1: Validate for first byte to always have a value of [1]
> +    if(common_hdr[0] != HDR_BYTE_ZERO)
> +    {
> +        fprintf(stderr, "ERROR: Common Header entry_1:[0x%X] Invalid.\n",common_hdr[0]);
> +        return false;
> +    }
> +    else
> +    {
> +        printf("SUCCESS: Validated [0x%X] in common header\n",common_hdr[0]);
> +    }
> +
> +    // 2: Validate the header checskum that is at last byte ( Offset: 7 )
> +    checksum = calculate_checksum(common_hdr, sizeof(common_hdr)-1);
> +    if(checksum != common_hdr[HDR_CRC_OFFSET])
> +    {
> +        fprintf(stderr, "ERROR: Common Header checksum mismatch."
> +                " Calculated:[0x%X], Embedded:[0x%X]\n", 
> +                checksum, common_hdr[HDR_CRC_OFFSET]);
> +    
> +        return false;
> +    }

We probably don't want to print errors for these validation failures
because we expect to get them regularly as part of the "good path".

> +            // 1: Goto beginning of the FILE
> +            rewind(fru_file);
> +
> +            // 2: Move the file pointer to record_offset
> +            fseek(fru_file, record_offset, SEEK_SET);

Why do you need 'rewind' if the next operation is an fseek(...SET)?

> +            uint8_t fru_record_data[fru_record_len];
> +            memset(fru_record_data, 0x0, sizeof(fru_record_data));

Memset is redundant if we're going to fread below.

> +            // Since the data is from record data[0], move the file pointer again to
> +            // beginning of record.
> +            fseek(fru_file, -2, SEEK_CUR);
> +            fread(fru_record_data, sizeof(fru_record_data), 1, fru_file);

Need to check return codes on all of these fseek, freed calls.
-- 
Patrick Williams
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20151015/175690db/attachment-0001.sig>