Re: [dmidecode] [PATCH] dmioem: Present HPE type 240 attributes in a nicer way

[Jerry Hoemann]

On Fri, Jan 15, 2021 at 05:46:07PM +0100, Jean Delvare wrote:
> Present HPE type 240 attributes as a proper list instead of
> packing them on a single line. This makes it more readable overall,
> and will also scale better if the number of attributes increases.


Output looks good.  I don't see any errors.  One minor suggestion inline.

> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> ---
> Hi Jerry,
> 
> This is my alternative proposal to display the HPE type 240 attributes.
> Before:
> 
>       Attributes Def: Updatable ResetRequired AuthenticationRequired InUse 
>       Attributes Set: Updatable ResetRequired InUse 
> 
> After:
> 
>       Attributes:


Would we want something like?    Attributes Defined/Set:



>               Updatable: Yes
>               Reset Required: Yes
>               Authentication Required: No
>               In Use: Yes
> 
> Let me know what you think.
> 
>  dmioem.c |   32 +++++++++++++++++---------------
>  1 file changed, 17 insertions(+), 15 deletions(-)
> 
> --- dmidecode.orig/dmioem.c   2021-01-15 17:40:27.277151093 +0100
> +++ dmidecode/dmioem.c        2021-01-15 17:43:49.277831357 +0100
> @@ -187,21 +187,24 @@ static int dmi_hpegen(const char *s)
>       return (dmi_vendor == VENDOR_HPE) ? G10P : G6;
>  }
>  
> -static void dmi_hp_240_attr(const char *fname, u64 code)
> +static void dmi_hp_240_attr(u64 defined, u64 set)
>  {
> -     char attr[80] = "";
> +     static const char *attributes[] = {
> +             "Updatable",
> +             "Reset Required",
> +             "Authentication Required",
> +             "In Use",
> +             "UEFI Image",
> +     };
> +     unsigned int i;
>  
> -     if (code.l & (1ULL << 0))
> -             strcat(attr, "Updatable ");
> -     if (code.l & (1ULL << 1))
> -             strcat(attr, "ResetRequired ");
> -     if (code.l & (1ULL << 2))
> -             strcat(attr, "AuthenticationRequired ");
> -     if (code.l & (1ULL << 3))
> -             strcat(attr, "InUse ");
> -     if (code.l & (1ULL << 4))
> -             strcat(attr, "UefiImage");
> -     pr_attr(fname, "%s", attr);
> +     pr_attr("Attributes", NULL);
> +     for (i = 0; i < ARRAY_SIZE(attributes); i++)
> +     {
> +             if (!(defined.l & (1UL << i)))
> +                     continue;
> +             pr_subattr(attributes[i], "%s", set.l & (1UL << i) ? "Yes" : 
> "No");
> +     }
>  }
>  
>  static void dmi_hp_203_assoc_hndl(const char *fname, u16 num)
> @@ -591,8 +594,7 @@ static int dmi_decode_hp(const struct dm
>                       else
>                               pr_attr("Image Size", "Not Available");
>  
> -                     dmi_hp_240_attr("Attributes Def", QWORD(data + 0x13));
> -                     dmi_hp_240_attr("Attributes Set", QWORD(data + 0x1B));
> +                     dmi_hp_240_attr(QWORD(data + 0x13), QWORD(data + 0x1B));
>  
>                       if (DWORD(data + 0x23))
>                               pr_attr("Lowest Supported Version", "0x%08X", 
> DWORD(data + 0x23));
> -- 
> Jean Delvare
> SUSE L3 Support

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------