[PATCH 1/2 v2] smbios: Simplify reporting of unknown values

[Simon Glass]

Hi Ilias,

On Mon, 27 Nov 2023 at 10:11, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> If a value is not valid during the DT or SYSINFO parsing,  we explicitly
> set that to "Unknown Product" and "Unknown" for the product and
> manufacturer respectively.  It's cleaner if we move the checks insisde
> smbios_add_string() and always report "Unknown" regardless of the missing
> field.
>
> pre-patch dmidecode
> <snip>
> Handle 0x0001, DMI type 1, 27 bytes
> System Information
>         Manufacturer: Unknown
>         Product Name: Unknown Product
>         Version: Not Specified
>         Serial Number: Not Specified
>         UUID: Not Settable
>         Wake-up Type: Reserved
>         SKU Number: Not Specified
>         Family: Not Specified
>
> [...]
>
> post-patch dmidecode:
>
> Handle 0x0001, DMI type 1, 27 bytes
> System Information
>         Manufacturer: Unknown
>         Product Name: Unknown
>         Version: Unknown
>         Serial Number: Unknown
>         UUID: Not Settable
>         Wake-up Type: Reserved
>         SKU Number: Unknown
>         Family: Unknown
> [...]
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> Reviewed-by: Peter Robinson <pbrobinson at gmail.com>
> Tested-by: Peter Robinson <pbrobinson at gmail.com>
> ---
> Changes since v1:
> - None
>
>  lib/smbios.c | 17 +++--------------
>  1 file changed, 3 insertions(+), 14 deletions(-)
>
> diff --git a/lib/smbios.c b/lib/smbios.c
> index d7f4999e8b2a..fcc8686993ef 100644
> --- a/lib/smbios.c
> +++ b/lib/smbios.c
> @@ -102,7 +102,7 @@ static int smbios_add_string(struct smbios_ctx *ctx, const char *str)

I suggest that this function have an additional str property
indicating the default string. Then you can pass DEFAULT_VAL or
something like that, to each call.

>         int i = 1;
>         char *p = ctx->eos;
>
> -       if (!*str)
> +       if (!str || !*str)

Does it ever happen that the string is present but empty?

>                 str = "Unknown";
>
>         for (;;) {
> @@ -151,8 +151,7 @@ static int smbios_add_prop_si(struct smbios_ctx *ctx, const char *prop,
>                 const char *str;
>
>                 str = ofnode_read_string(ctx->node, prop);
> -               if (str)
> -                       return smbios_add_string(ctx, str);
> +               return smbios_add_string(ctx, str);
>         }
>
>         return 0;
> @@ -231,7 +230,7 @@ static int smbios_write_type0(ulong *current, int handle,
>         t->vendor = smbios_add_string(ctx, "U-Boot");
>
>         t->bios_ver = smbios_add_prop(ctx, "version");
> -       if (!t->bios_ver)
> +       if (!strcmp(ctx->last_str, "Unknown"))

That is really ugly...checking the ctx value looking for a side effect.

Can you not have smbios_add_prop() continue to return NULL in this case?

>                 t->bios_ver = smbios_add_string(ctx, PLAIN_VERSION);
>         if (t->bios_ver)
>                 gd->smbios_version = ctx->last_str;
> @@ -281,11 +280,7 @@ static int smbios_write_type1(ulong *current, int handle,
>         fill_smbios_header(t, SMBIOS_SYSTEM_INFORMATION, len, handle);
>         smbios_set_eos(ctx, t->eos);
>         t->manufacturer = smbios_add_prop(ctx, "manufacturer");
> -       if (!t->manufacturer)
> -               t->manufacturer = smbios_add_string(ctx, "Unknown");
>         t->product_name = smbios_add_prop(ctx, "product");
> -       if (!t->product_name)
> -               t->product_name = smbios_add_string(ctx, "Unknown Product");
>         t->version = smbios_add_prop_si(ctx, "version",
>                                         SYSINFO_ID_SMBIOS_SYSTEM_VERSION);
>         if (serial_str) {
> @@ -315,11 +310,7 @@ static int smbios_write_type2(ulong *current, int handle,
>         fill_smbios_header(t, SMBIOS_BOARD_INFORMATION, len, handle);
>         smbios_set_eos(ctx, t->eos);
>         t->manufacturer = smbios_add_prop(ctx, "manufacturer");
> -       if (!t->manufacturer)
> -               t->manufacturer = smbios_add_string(ctx, "Unknown");
>         t->product_name = smbios_add_prop(ctx, "product");
> -       if (!t->product_name)
> -               t->product_name = smbios_add_string(ctx, "Unknown Product");
>         t->version = smbios_add_prop_si(ctx, "version",
>                                         SYSINFO_ID_SMBIOS_BASEBOARD_VERSION);
>         t->asset_tag_number = smbios_add_prop(ctx, "asset-tag");
> @@ -344,8 +335,6 @@ static int smbios_write_type3(ulong *current, int handle,
>         fill_smbios_header(t, SMBIOS_SYSTEM_ENCLOSURE, len, handle);
>         smbios_set_eos(ctx, t->eos);
>         t->manufacturer = smbios_add_prop(ctx, "manufacturer");
> -       if (!t->manufacturer)
> -               t->manufacturer = smbios_add_string(ctx, "Unknown");
>         t->chassis_type = SMBIOS_ENCLOSURE_DESKTOP;
>         t->bootup_state = SMBIOS_STATE_SAFE;
>         t->power_supply_state = SMBIOS_STATE_SAFE;
> --
> 2.40.1
>

Regards,
Simon