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

[Ilias Apalodimas]

Hi Simon,


On Thu, 30 Nov 2023 at 04:46, Simon Glass <sjg at chromium.org> wrote:

> 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.
>

Why? Do you think we need to fill in something different that "unknown"?


> >         int i = 1;
> >         char *p = ctx->eos;
> >
> > -       if (!*str)
> > +       if (!str || !*str)
>
> Does it ever happen that the string is present but empty?
>

With the changes on this patchset yes.


> >                 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?
>

Hmm I don't know, but I wonder why I am not just checking t->bios_ver for
Unknown.
I'll have a look and change it

[...]

Thanks
/Ilias