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

[Mario_Limonciello]

Jean,

> -----Original Message-----
> From: Jean Delvare [mailto:jdelvare@xxxxxxx]
> Sent: Thursday, September 1, 2016 1:01 PM
> To: Limonciello, Mario <Mario_Limonciello@xxxxxxxx>
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; Hung, Allen <Allen_Hung@xxxxxxxx>;
> rmk+kernel@xxxxxxxxxxxxxxxx; somlo@xxxxxxx; jens.wiklander@xxxxxxxxxx;
> agross@xxxxxxxxxxxxxx; arnd@xxxxxxxx; sudeep.holla@xxxxxxx;
> eric@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3 2/2] dmi-id: add dmi/id/oem group for exporting oem
> strings to sysfs
> 
> 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.

Yeah that's sorta what I was thinking it was for too.  Considering it was 
cheap to create those sysfs nodes without a clear standard consumer
is what was making me think that exposing OEM strings in kernel space 
made sense too.

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

Sure

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

At least on Dell systems the number and contents of OEM strings is 
dynamic.  You won't be able to know in advance how many strings
will exist on a given box since some strings may only be present
based upon configuration settings.

The original kernel patch this wasn't a concern because you could
count number of sysfs files to determine how many strings were
present.

With the current PoC implementation of yours, an app would need
to just keep calling with monotonically increasing OEM string index
values until an empty output was returned to find the number of 
strings present.

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

I don't know the context of those complaints, but as long as you return 
an error code from dmidecode and output
to stderr rather than stdout this makes sense to me.

Apps should only be looking at the output of stdout anyhow.
 
> > 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.

I do too for the same reasons.  I didn't look close enough at the code
to realize it was intended behavior on your behalf.  No concerns here.