Re: [dmidecode] [PATCH 3/4] dmioem: Use product name to determine Generation.

[Jean Delvare]

Hi Jerry,

On Thu, 19 Nov 2020 12:55:15 -0700, Jerry Hoemann wrote:
> Generally, HPE OEM records are extended by adding additional
> fields at the end or record and increasing the length.  But,
> this isn't always the case and will less likely be so going
> into the future.
> 
> Determine the generation of the HPE OEM records by examining
> the "Product Name" string.
> 
> Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> ---
>  dmioem.c | 33 +++++++++++++++++++++++++++++++--
>  1 file changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/dmioem.c b/dmioem.c
> index 36820e4..16f4488 100644
> --- a/dmioem.c
> +++ b/dmioem.c
> @@ -150,12 +150,41 @@ static void dmi_print_hp_net_iface_rec(u8 id, u8 bus, 
> u8 dev, const u8 *mac)
>       }
>  }
>  
> -static int dmi_decode_hp(const struct dmi_header *h)
> +typedef enum { G6=6, G7, G8, G9, G10, G10P } dmi_hpegen_t;

Spaces around the equal sign.

> +
> +static int dmi_hpegen(const char *s)
> +{
> +     struct { const char *name; dmi_hpegen_t gen; } table[] = {
> +             { "Gen10 Plus", G10P },
> +             { "Gen10", G10 },
> +             { "Gen9",  G9 },
> +             { "Gen8",  G8 },
> +             { "G7",    G7 },
> +             { "G6",    G6 },

If you are aligning, you might as well do it for all lines including
the first ;-)

> +     };
> +     unsigned int i;

Blank line here please.

> +     if (!strstr(s, "ProLiant") && !strstr(s, "Apollo") &&
> +         !strstr(s, "Synergy")  && !strstr(s, "Edgeline"))
> +             return -1;
> +
> +     for (i = 0;  i < sizeof(table)/sizeof(table[0]); i++) {

We have ARRAY_SIZE() defined in util.h, that would be more readable.

Also you have a double space after the first semicolon.

> +             if ( strstr(s, table[i].name) )

Coding style: no spaces inside parentheses.

> +                     return(table[i].gen);
> +     }

Blank line here please.

> +     return (dmi_vendor == VENDOR_HPE) ? G10P: G6;

Space before colon. 

> +}
> +
> +

Please avoid double blank lines.

> +static int dmi_decode_hp(const struct dmi_header *h, const char *product)
>  {
>       u8 *data = h->data;
>       int nic, ptr;
>       u32 feat;
>       const char *company = (dmi_vendor == VENDOR_HP) ? "HP" : "HPE";
> +     int gen = dmi_hpegen(product);
> +
> +     if (gen < 0)
> +             return 0;

Style-wise, I prefer when there is no blank line between a command and
the validation of its result. So I'd go for:

        int gen;

        gen = dmi_hpegen(product);
        if (gen < 0)
                return 0;

>  
>       switch (h->type)
>       {
> @@ -419,7 +448,7 @@ int dmi_decode_oem(const struct dmi_header *h)
>       {
>               case VENDOR_HP:
>               case VENDOR_HPE:
> -                     return dmi_decode_hp(h);
> +                     return dmi_decode_hp(h, dmi_product);

dmi_product is a global variable, so I wouldn't mind if you don't pass
it as a parameter and use it directly in dmi_decode_hp() instead.

>               case VENDOR_ACER:
>                       return dmi_decode_acer(h);
>               case VENDOR_IBM:

Thanks,
-- 
Jean Delvare
SUSE L3 Support