[PATCH ipmi-fru-parser v3 3/4] Added interface function to parse wirte fru data message into a dictionary.

[Patrick Williams]

On Fri, Oct 16, 2015 at 07:37:48AM -0500, OpenBMC Patches wrote:
> From: Hariharasubramanian R <hramasub at in.ibm.com>
> +const char* vpd_key_names [] = 
> +{
> +  "Key Names Table Start", 
> +  "Chassis Type", /*OPENBMC_VPD_KEY_CHASSIS_TYPE*/
> +  "Chassis Part Number", /*OPENBMC_VPD_KEY_CHASSIS_PART_NUM,*/
> +  "Chassis Serial Number", /*OPENBMC_VPD_KEY_CHASSIS_SERIAL_NUM,*/
> +
> +  /* TODO: chassis_custom_fields */
> +
> +  "Board Mfg Date", /* OPENBMC_VPD_KEY_BOARD_MFG_DATE, */ /* not a type/len */
> +  "Board Manufacturer", /* OPENBMC_VPD_KEY_BOARD_MFR, */
> +  "Board Name", /* OPENBMC_VPD_KEY_BOARD_NAME, */
> +  "Board Serial Number", /* OPENBMC_VPD_KEY_BOARD_SERIAL_NUM, */
> +  "Board Part Number", /* OPENBMC_VPD_KEY_BOARD_PART_NUM, */
> +  "Board FRU File ID", /* OPENBMC_VPD_KEY_BOARD_FRU_FILE_ID, */
> +  /* TODO: board_custom_fields */
> +
> +  "Product Manufacturer", /* OPENBMC_VPD_KEY_PRODUCT_MFR, */
> +  "Product Name", /* OPENBMC_VPD_KEY_PRODUCT_NAME, */
> +  "Product Model Number", /* OPENBMC_VPD_KEY_PRODUCT_PART_MODEL_NUM, */
> +  "Product Version", /* OPENBMC_VPD_KEY_PRODUCT_VER, */
> +  "Product Serial Number", /* OPENBMC_VPD_KEY_PRODUCT_SERIAL_NUM, */
> +  "Product Asset Tag", /* OPENBMC_VPD_KEY_PRODUCT_ASSET_TAG, */
> +  "Product FRU File ID", /* OPENBMC_VPD_KEY_PRODUCT_FRU_FILE_ID, */
> +  /* TODO: product_custom_fields */
> +
> +  "Key Names Table End" /*OPENBMC_VPD_KEY_MAX,*/
> +};

These should be lower-case single-word strings like 'chassis-type',
'chassis-part-number'.

Based on these names, it seems like we almost need to have a dictionary
of dictionaries:
{ 'chassis' :
    { 'part-number' : 1234,
      'serial-number' : 5678 
    },
  'board' : { ...}
}

How do we plan to split this into two different DBus objects?  One for
the chassis and one for the board.

>  
> +
> +/*
> + * --------------------------------------------------------------------
> + *
> + * --------------------------------------------------------------------
> + */
> +
> +/* private method to parse type/length */
>  static int
>  _parse_type_length (const void *areabuf,
>                      unsigned int areabuflen,
> @@ -84,14 +194,14 @@ _parse_type_length (const void *areabuf,
>    
>    type_length = areabufptr[current_area_offset];
>  
> -  /* IPMI Workaround 
> +  /* ipmi workaround 
>     *
> -   * Dell P weredge R610
> +   * dell p weredge r610

Why did you make so many case changes in the comments?  I think we
should revert this.  This is through the whole file.

> +  /* Populate VPD Table */
> +  for (i=1; i<OPENBMC_VPD_KEY_MAX; i++)
> +  {
> +    if (i==OPENBMC_VPD_KEY_CHASSIS_TYPE)
> +    {
> +        sd_bus_message_append (vpdtbl, "sd", vpd_key_names[i], chassis_type);

'd' is double.  This does not seem correct.

> +        continue;
> +    }
> +
> +    if (i==OPENBMC_VPD_KEY_BOARD_MFG_DATE)
> +    {
> +        sd_bus_message_append (vpdtbl, "sd", vpd_key_names[i], mfg_date_time);

'd' is double.  Didn't you intend 'date'?

> +        continue;
> +    }
> +    
> +    sd_bus_message_append (vpdtbl, "ss", vpd_key_names[i], vpd_info [i].type_length_field); 

How do you know that all these fields were actually obtained from the VPD?
Don't you need something to look for unpopulated entries?

How does this create entries in the dictionary?  I don't think this is
correct.

Aren't these all suppose to be "sv"?  String to variant.
-- 
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/20151016/04bd5614/attachment.sig>