Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem strings to sysfs

[Jean Delvare]

Hi Mario,

On Wed, 31 Aug 2016 21:51:22 +0000, Mario_Limonciello@xxxxxxxx wrote:
> > > Ah, yeah, just use dmidecode, much simpler, keeps the kernel smaller, I
> > > like it.
> 
> The main fundamental difference between kernel and userspace
> will be that applications will need to run dmidecode multiple times
> to get at all this data rather than read in a handful of files from sysfs.

That's correct, and I agree it is slightly less efficient. But the
number of strings being small I don't think it's really a problem in
practice.

> I'd like to ask more on the history of why *any* SMBIOS data was
> exposed to sysfs in the first place rather than making all of
> userspace do this same exercise of calling dmidecode to get at data?

If I recall properly it was introduced for kernel module auto-loading
based on DMI data, through udev.
Specifically /sys/devices/virtual/dmi/id/uevent was for this purpose.
The other attributes must have been added because it was cheap at that
point.

I suppose it could have been implemented using dmidecode as well, but
doing it in sysfs was more natural because this is where "real" devices
are also declared. So I guess there was almost no code to add to udev
to make it work.

> Why are the strings already exposed by the kernel in sysfs any more 
> valuable than OEM strings?

Because OEM strings are not standard so generic tools have no use for
them. As a matter of fact the other attributes were added 9 years ago
and only now someone (you) is asking for OEM strings.

> (...)
> I applied your patch locally and looked a little bit at it.

Thanks for testing.

> The main downside I see from this approach versus what Allen did in
> the kernel is you don't know in advance how many OEM strings will
> exist.
>
> Allen's kernel approach you knew how many would be there by the
> number of sysfs items that were created.  Your userspace approach I
> can only really see working by trial and error based upon the
> argument you give it.
> 
> For example on a Precision 5510 I see 7 OEM strings, but on a T5810 I
> only have 4.

My expectation was that whoever needs some OEM string would know its
index as well. There is no description attached to each string anyway,
so the index is the only key to figure out what is what. The vendor is
responsible for getting it right (that is, be consistent where it
matters.)

> Maybe one way to solve this would be if no arguments were given to
> --oem-string return the number of OEM strings rather than an error.

Can you explain why you need to know? If I knew your use case I would
feel more motivated to come up with a solution ;-)

(I am also not a fan of parameters with optional arguments in general,
as this can make the command lines ambiguous, but this is an
implementation detail really.)

> I know it was just a PoC, but if you do end up including this in
> dmidecode some other functional comments:
> 1)  --help would need to be updated too for the new option.

Thanks for the review, I appreciate it.

You are right, I forgot to update --help, and the manual page too.

> 2) There is testing for some invalid arguments, but if you put a
> larger number than number of OEM strings no error is displayed.

That was on purpose. People kept complaining to me over the past years
when option --string returned an error message. I finally "fixed" it for
--string so I thought --oem-string should behave the same for
consistency. The assumption is that the caller would test if it gets an
empty string. Empty strings are not allowed in DMI data so if you get
an empty string it means there was no string by that index.

But I can print an error message on invalid index if you think it makes
sense, that's easy.

> 3) -O didn't seem to work for me, only --oem-string.

On purpose as well. There is no short option for --dump-bin,
--from-dump or --no-sysfs either, because I do not expect them to be
used frequently. You still have to give each option a one-char
identifier to make getopt_long() happy, even if you don't support the
corresponding short option. I chose "O" but it is arbitrary and
internal only at this point.

Maybe it's me getting old, but I tend to prefer using long options in
scripts now. I find it easier to read later, no need to remember what
the short options do nor look it up in the manual page.

-- 
Jean Delvare
SUSE L3 Support