Re: [dmidecode] [Patch v2 4/4] dmidecode: add dmi sysfs support

[Jean Delvare]

Hi Ivan,

On Wed, 25 Feb 2015 17:56:46 +0200, Ivan Khoronzhuk wrote:
> In cases when access to /dev/mem is forbidden the dmi sysfs can
> be used. This patch adds dmi sysfs support.
> 
> To add support of dmi sysfs the newly introduced dmifs library were
> used - libdmifs. Currently it's situated at
> https://git.linaro.org/people/ivan.khoronzhuk/libdmifs.git
> 
> The library is needed to simplify working with DMI sysfs.
> 
> Reported-by: Leif Lindholm <address@hidden>
> Signed-off-by: Ivan Khoronzhuk <address@hidden>
> ---
>  Makefile    |  2 +-
>  dmidecode.c | 31 ++++++++++++++++++++++++++++++-
>  version.h   |  2 +-
>  3 files changed, 32 insertions(+), 3 deletions(-)
> (...)

Out of curiosity, I fetched the latest version of libdmifs and tried
this patch set. I compared the new output with what dmidecode would
output before and I get the following difference:

--- /tmp/dmidecode.old  2015-02-26 12:23:17.624968398 +0100
+++ /tmp/dmidecode.new  2015-02-26 12:25:05.643322338 +0100
@@ -813,6 +813,7 @@
        Base Address: 0x0000000000000CA2 (Memory-mapped)
        Register Spacing: Successive Byte Boundaries
 
-Handle 0x0042, DMI type 127, 4 bytes
-End Of Table
+Handle 0xBABA, DMI type 186, 186 bytes
+       <TRUNCATED>
 
+Wrong DMI structures length: 2500 bytes announced, structures occupy 2682 
bytes.

This doesn't look good. Running dmidecode under valgrind reveals errors:

==5298== Conditional jump or move depends on uninitialised value(s)
==5298==    at 0x409513: dmi_table (dmidecode.c:4410)
==5298==    by 0x409A3B: dmifs_smbios_decode (dmidecode.c:4552)
==5298==    by 0x409F17: main (dmidecode.c:4721)
==5298== 
==5298== Use of uninitialised value of size 8
==5298==    at 0x507C84B: _itoa_word (in /lib64/libc-2.18.so)
==5298==    by 0x508091C: vfprintf (in /lib64/libc-2.18.so)
==5298==    by 0x5086F58: printf (in /lib64/libc-2.18.so)
==5298==    by 0x40952C: dmi_table (dmidecode.c:4412)
==5298==    by 0x409A3B: dmifs_smbios_decode (dmidecode.c:4552)
==5298==    by 0x409F17: main (dmidecode.c:4721)
==5298== 
==5298== Conditional jump or move depends on uninitialised value(s)
==5298==    at 0x507C855: _itoa_word (in /lib64/libc-2.18.so)
==5298==    by 0x508091C: vfprintf (in /lib64/libc-2.18.so)
==5298==    by 0x5086F58: printf (in /lib64/libc-2.18.so)
==5298==    by 0x40952C: dmi_table (dmidecode.c:4412)
==5298==    by 0x409A3B: dmifs_smbios_decode (dmidecode.c:4552)
==5298==    by 0x409F17: main (dmidecode.c:4721)
==5298== 
==5298== Conditional jump or move depends on uninitialised value(s)
==5298==    at 0x5080968: vfprintf (in /lib64/libc-2.18.so)
==5298==    by 0x5086F58: printf (in /lib64/libc-2.18.so)
==5298==    by 0x40952C: dmi_table (dmidecode.c:4412)
==5298==    by 0x409A3B: dmifs_smbios_decode (dmidecode.c:4552)
==5298==    by 0x409F17: main (dmidecode.c:4721)
==5298== 
==5298== Conditional jump or move depends on uninitialised value(s)
==5298==    at 0x50801C5: vfprintf (in /lib64/libc-2.18.so)
==5298==    by 0x5086F58: printf (in /lib64/libc-2.18.so)
==5298==    by 0x40952C: dmi_table (dmidecode.c:4412)
==5298==    by 0x409A3B: dmifs_smbios_decode (dmidecode.c:4552)
==5298==    by 0x409F17: main (dmidecode.c:4721)
==5298== 
==5298== Conditional jump or move depends on uninitialised value(s)
==5298==    at 0x5080248: vfprintf (in /lib64/libc-2.18.so)
==5298==    by 0x5086F58: printf (in /lib64/libc-2.18.so)
==5298==    by 0x40952C: dmi_table (dmidecode.c:4412)
==5298==    by 0x409A3B: dmifs_smbios_decode (dmidecode.c:4552)
==5298==    by 0x409F17: main (dmidecode.c:4721)
==5298== 
(...)
==5298== ERROR SUMMARY: 6 errors from 6 contexts (suppressed: 2 from 2)

This only confirms my initial feeling that the complexity added by the
extra layer is going to cause more trouble than is worth.

I also made some performance measurements. As I expected, the new
libdmifs implementation is 3 times slower than the /dev/mem access
(from 1.56 ms to 4.71 ms on average on my workstation.) The system CPU
time grows even more (0.74 ms to 3.41 ms, 4.6 times slower).

The only good news is that the entries are in the right order, which I
did not think the dmi-sysfs interface would allow (I now see the
"position" attribute makes that possible.) But that alone won't be
enough to change my position.

-- 
Jean Delvare
SUSE L3 Support