Hi Ilias,
On Mon, 27 Nov 2023 at 10:11, Ilias Apalodimas
<ilias.apalodi...@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.apalodi...@linaro.org>
> Reviewed-by: Peter Robinson <pbrobin...@gmail.com>
> Tested-by: Peter Robinson <pbrobin...@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
