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

[Jean Delvare]

Hi Mario, Allen,

Sorry for taking so long to get back to you. No excuses really :-(

On Thu, 1 Sep 2016 19:14:54 +0000, Mario_Limonciello@xxxxxxxx wrote:
> > -----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
> > 
> > On Wed, 31 Aug 2016 21:51:22 +0000, Mario_Limonciello@xxxxxxxx wrote:
> > > 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.

That I understand. What I do not understand is how software is supposed
to figure out which string represents what, even if it knows how many
strings there are in total.

Note that I am not blaming Dell here, the SMBIOS specification is
really weak in this respect, it would have better defined string pairs,
so that the BIOS can specify both string names and values; or defined a
standard format to include both in one string (simply defining a
standard separator character such as ";" or "=" would have done the
trick.) Leaving it to each vendor to sort it out was the best way to
ensure there wouldn't be any consistency. Sigh.

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

Alternatively the software could call:
$ dmidecode -q -t 11 | grep -c '^[[:space:]][[:space:]]*String'

But I agree we can make it easier. My proposal is that "--oem-string
count" will return the number of OEM strings.

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

You are correct. At the time of these complaints, there was also some
confusion about what went to stdout and what when to stderr. As it has
been sorted out meanwhile, spitting an error message on bad OEM string
index shouldn't be a problem. I have implemented it.

Below is my second proposal of a dmidecode patch implementing what you
need. I believe I have addressed all your concerns, but please report
if there is anything left to fix [1]. If you are happy with this patch,
I will include it in the upcoming release 3.1 of dmidecode.

[1] One thing I am not sure of is how to deal with the case where a DMI
table has multiple type 11 records. While this doesn't make much sense,
the DMI specification does note mandate uniqueness of this record type,
and I have seen several systems from one vendor with multiple 1-string
type 11 records instead of a single multiple-string type 11 record.
With my implementation, "dmidecode --oem-string 1" will return all of
them, in record order (which may seem strange, but is how option
--string works for other records, so it is at least consistent.)
Allen's kernel implementation would number all OEM strings linearly
instead, giving them a unique number (which is good) but losing their
origin (not good.) All I can think of at the moment is either a generic
--handle command line option that would restrict the output of
dmidecode to the specified record, or something like --oem-string
[handle,]number, that is, optionally restricting the output of
--oem-string to a specific handle (or maybe instance number.) Opinions?

Anyway, here's the patch:

Subject: dmidecode: New option --oem-string

Add a new option to extract OEM strings, like we already have for
many other strings.

Signed-off-by: Jean Delvare <jdelvare@xxxxxxx>
---
Changes since v1:
 * Document the new option in --help and man page.
 * Return an error message if index is out of range.
 * Special value "count" returns the number of OEM strings.

 dmidecode.c     |   15 +++++++++++++++
 dmiopt.c        |   40 ++++++++++++++++++++++++++++++++++++++++
 man/dmidecode.8 |    7 ++++++-
 3 files changed, 61 insertions(+), 1 deletion(-)

--- dmidecode.orig/dmiopt.c	2017-04-04 10:03:43.885412547 +0200
+++ dmidecode/dmiopt.c	2017-05-01 12:23:44.207581011 +0200
@@ -20,6 +20,7 @@
  */
 
 #include <stdio.h>
+#include <string.h>
 #include <strings.h>
 #include <stdlib.h>
 #include <getopt.h>
@@ -171,6 +172,10 @@ static const struct string_keyword opt_s
 	{ "processor-frequency", 4, 0x16 },     /* dmi_processor_frequency() */
 };
 
+/* This is a template, 3rd field is set at runtime. */
+static struct string_keyword opt_oem_string_keyword =
+	{ NULL, 11, 0x00 };
+
 static void print_opt_string_list(void)
 {
 	unsigned int i;
@@ -206,6 +211,34 @@ static int parse_opt_string(const char *
 	return -1;
 }
 
+static int parse_opt_oem_string(const char *arg)
+{
+	unsigned long val;
+	char *next;
+
+	if (opt.string)
+	{
+		fprintf(stderr, "Only one string can be specified\n");
+		return -1;
+	}
+
+	/* Return the number of OEM strings */
+	if (strcmp(arg, "count") == 0)
+		goto done;
+
+	val = strtoul(arg, &next, 10);
+	if (next == arg || val == 0x00 || val > 0xff)
+	{
+		fprintf(stderr, "Invalid OEM string number: %s\n", arg);
+		return -1;
+	}
+
+	opt_oem_string_keyword.offset = val;
+done:
+	opt.string = &opt_oem_string_keyword;
+	return 0;
+}
+
 
 /*
  * Command line options handling
@@ -225,6 +258,7 @@ int parse_command_line(int argc, char *
 		{ "dump", no_argument, NULL, 'u' },
 		{ "dump-bin", required_argument, NULL, 'B' },
 		{ "from-dump", required_argument, NULL, 'F' },
+		{ "oem-string", required_argument, NULL, 'O' },
 		{ "no-sysfs", no_argument, NULL, 'S' },
 		{ "version", no_argument, NULL, 'V' },
 		{ NULL, 0, NULL, 0 }
@@ -255,6 +289,11 @@ int parse_command_line(int argc, char *
 					return -1;
 				opt.flags |= FLAG_QUIET;
 				break;
+			case 'O':
+				if (parse_opt_oem_string(optarg) < 0)
+					return -1;
+				opt.flags |= FLAG_QUIET;
+				break;
 			case 't':
 				opt.type = parse_opt_type(opt.type, optarg);
 				if (opt.type == NULL)
@@ -315,6 +354,7 @@ void print_help(void)
 		"     --dump-bin FILE    Dump the DMI data to a binary file\n"
 		"     --from-dump FILE   Read the DMI data from a binary file\n"
 		"     --no-sysfs         Do not attempt to read DMI data from sysfs files\n"
+		"     --oem-string N     Only display the value of the given OEM string\n"
 		" -V, --version          Display the version and exit\n";
 
 	printf("%s", help);
--- dmidecode.orig/dmidecode.c	2017-04-27 16:55:26.011215158 +0200
+++ dmidecode/dmidecode.c	2017-05-01 12:25:35.269804990 +0200
@@ -4555,6 +4555,21 @@ static void dmi_table_string(const struc
 	int key;
 	u8 offset = opt.string->offset;
 
+	if (opt.string->type == 11) /* OEM strings */
+	{
+		if (h->length < 5 || offset > data[4])
+		{
+			fprintf(stderr, "No OEM string number %u\n", offset);
+			return;
+		}
+
+		if (offset)
+			printf("%s\n", dmi_string(h, offset));
+		else
+			printf("%u\n", data[4]);	/* count */
+		return;
+	}
+
 	if (offset >= h->length)
 		return;
 
--- dmidecode.orig/man/dmidecode.8	2015-08-06 12:49:52.339237585 +0200
+++ dmidecode/man/dmidecode.8	2017-05-01 12:07:26.188735324 +0200
@@ -134,13 +134,18 @@ Read the DMI data from a binary file pre
 Do not attempt to read DMI data from sysfs files. This is mainly useful for
 debugging.
 .TP
+.BR "  " "  " "--oem-string N"
+Only display the value of the \s-1OEM\s0 string number \fBN\fR. The first
+\s-1OEM\s0 string has number 1. With special value "count", return the
+number of OEM strings instead.
+.TP
 .BR "-h" ", " "--help"
 Display usage information and exit
 .TP
 .BR "-V" ", " "--version"
 Display the version and exit
 .P
-Options --string, --type and --dump-bin
+Options --string, --type, --dump-bin and --oem-string
 determine the output format and are mutually exclusive.
 .P
 Please note in case of


-- 
Jean Delvare
SUSE L3 Support