Re: [dmidecode] [PATCH 3/4] Add reading of SMBIOS tables from sysfs

[Jean Delvare]

On Thu, 16 Apr 2015 21:14:00 -0700, Roy Franz wrote:
> This patch adds preferential reading of the SMBIOS tables from
> /sys/firmware/dmi/tables.  If these files are not present or
> not valid, the previously supported methods of locating SMBIOS
> tables are attempted.
> This patch does change the output of dmidecode slightly:
> - Messages indicating which source is used for the tables
>   have been added.  These are printed before the tables have
>   been validated so they can go at the top of the output.  This
>   also shows what methods have been tried and failed due invalid
>   tables.
> - the address of the entrypoint is not known when read from sysfs,
>   so it is not printed in that case.
> 
> A placeholder print is added where 64 bit entry point processing
> will be added.
> 
> Signed-off-by: Roy Franz <address@hidden>
> ---
>  dmidecode.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/dmidecode.c b/dmidecode.c
> index 1d19a1b..ef4259d 100644
> --- a/dmidecode.c
> +++ b/dmidecode.c
> @@ -70,6 +70,8 @@
>  static const char *bad_index = "<BAD INDEX>";
>  
>  #define SUPPORTED_SMBIOS_VER 0x0208
> +#define SYS_ENTRY_FILE "/sys/firmware/dmi/tables/smbios_entry_point"
> +#define SYS_TABLE_FILE "/sys/firmware/dmi/tables/DMI"
>  
>  /*
>   * Type-independant Stuff
> @@ -4674,6 +4676,35 @@ int main(int argc, char * const argv[])
>               goto done;
>       }
>  
> +     /*
> +      * First try reading from sysfs tables.  The entry point file could
> +      * contain one of several types of entry points, so read enough for
> +      * the largest one, then determine what type it contains.
> +      */
> +     if ((buf = read_file(0x20, SYS_ENTRY_FILE)) != NULL)
> +     {
> +             printf("Getting SMBIOS data from sysfs\n");

This shouldn't be printed in quiet mode, and same for the other printfs
below. Otherwise it breaks option -s.

> +             if (memcmp(buf, "_DMI_", 5) == 0)
> +             {
> +                     if (legacy_decode(buf, SYS_TABLE_FILE, 0))
> +                             found++;
> +             }
> +             else if (memcmp(buf, "_SM_", 4) == 0)
> +             {
> +                     if (smbios_decode(buf, SYS_TABLE_FILE, 0))
> +                             found++;
> +             }
> +             else if (memcmp(buf, "_SM3_", 5) == 0)
> +             {
> +                     printf("SMBIOS v3 64 bit entry point found, but not 
> supported\n");

"64-bit" (missing dash). Please add a trailing dot for consistency with
other messages (same above and below, BTW.)

> +             }
> +
> +             if (found)
> +                     goto done;
> +             else
> +                     printf("Failed to get SMBIOS data from sysfs, trying 
> /dev/mem\n");

"else" after "goto" doesn't seem terribly useful ;)

This may or may not be "/dev/mem" next, opt.devmem would be better. But
given that the next message will mention that, maybe drop the second
part of the sentence altogether?

> +     }
> +
>       /* Next try EFI (ia64, Intel-based Mac) */
>       efi = address_from_efi(&fp);
>       switch (efi)
> @@ -4685,6 +4716,7 @@ int main(int argc, char * const argv[])
>                       goto exit_free;
>       }
>  
> +     printf("Found SMBIOS entry point in EFI, reading table from %s.\n", 
> opt.devmem);
>       if ((buf = mem_chunk(fp, 0x20, opt.devmem)) == NULL)
>       {
>               ret = 1;
> @@ -4696,6 +4728,7 @@ int main(int argc, char * const argv[])
>       goto done;
>  
>  memory_scan:
> +     printf("Scanning %s for entry point.\n", opt.devmem);
>       /* Fallback to memory scan (x86, x86_64) */
>       if ((buf = mem_chunk(0xF0000, 0x10000, opt.devmem)) == NULL)
>       {

This integrates all nicely, I like it very much :)

-- 
Jean Delvare
SUSE L3 Support