Re: [dmidecode] [PATCH] dmidecode: Add option to filter output based upon handle

[Jerry Hoemann]

On Fri, Jun 29, 2018 at 11:08:36AM +0200, Jean Delvare wrote:
> Hi Jerry,
> 
> On Thu, 28 Jun 2018 14:08:06 -0600, Jerry Hoemann wrote:
> > Add option "--handle HANDLE" to dmiopt to allow user to filter
> > ouput to only those entrie(s) that match HANDLE.
> 
> I am curious what you need this feature for? Handle numbers are
> arbitrary and not portable, so it never occurred to me that someone
> could need to query for a specific handle.
> 

Covered in your exchange w/ Robert.

> > 
> > Signed-off-by: Jerry Hoemann <address@hidden>
> > ---
> >  dmidecode.c     |  2 ++
> >  dmiopt.c        | 22 +++++++++++++++++++++-
> >  dmiopt.h        |  1 +
> >  man/dmidecode.8 |  4 ++++
> >  4 files changed, 28 insertions(+), 1 deletion(-)
> > 
> > diff --git a/dmidecode.c b/dmidecode.c
> > index f8c3b30..023ed58 100644
> > --- a/dmidecode.c
> > +++ b/dmidecode.c
> > @@ -4732,6 +4732,7 @@ static void dmi_table_decode(u8 *buf, u32 len, u16 
> > num, u16 ver, u32 flags)
> >  
> >             to_dmi_header(&h, data);
> >             display = ((opt.type == NULL || opt.type[h.type])
> > +                   && ((opt.handle == ~0U) || (opt.handle == h.handle))
> 
> This is more parentheses than needed.

changed.

> 
> >                     && !((opt.flags & FLAG_QUIET) && (h.type == 126 || 
> > h.type == 127))
> >                     && !opt.string);
> >  
> > @@ -5144,6 +5145,7 @@ int main(int argc, char * const argv[])
> >     /* Set default option values */
> >     opt.devmem = DEFAULT_MEM_DEV;
> >     opt.flags = 0;
> > +   opt.handle = ~0U;
> >  
> >     if (parse_command_line(argc, argv)<0)
> >     {
> > diff --git a/dmiopt.c b/dmiopt.c
> > index a36cf16..6c74c7f 100644
> > --- a/dmiopt.c
> > +++ b/dmiopt.c
> > @@ -240,6 +240,19 @@ static int parse_opt_oem_string(const char *arg)
> >     return 0;
> >  }
> >  
> > +static u32 parse_opt_handle(const char *arg)
> > +{
> > +   u32 val;
> > +   char *next;
> > +
> > +   val = strtoul(arg, &next, 0);
> > +   if ((next && *next != '\0') || val > 0xffff)
> 
> "next" can't be NULL, so the first test will always succeed.

Defensive programming.

Changed.

> 
> Also, checking for "*next != '\0'" alone doesn't guarantee that the
> input is valid: if arg is an empty string, it is not valid, but *next
> will be '\0'. As a result,
> 
> # dmidecode --handle ""
> 
> will happily display the DMI entry with handle 0x0000. This is not
> consistent with the behavior of --oem-string:

Okay, getopt_long handles totally missing argument, but not the degenerate case.

Fixed.

> 
> # dmidecode --oem-string ""
> Invalid OEM string number: 
> 
> So for consistency with the other options, I would prefer if you check
> for "next == arg" to detect an error in the input. (This also is

Note, next != arg doesn't gurantee against badly formated argument.

e.g.  --handle 1w4

This is badly formated, next != arg, and val == 1.

So test should be:

        if (next == arg || *next != '\0' || val > 0xffff)


> admittedly not perfect, but if anyone is unhappy with that, it should
> be fixed for all options at once, for consistency.)


I'll fix the other uses of strtoul in a separate patch.

> 
> > +   {
> > +           fprintf(stderr, "Invalid handle nubmer: %s\n", arg);
> 
> Typo: number.

Fixed.

> 
> > +           return ~0;
> > +   }
> > +   return val;
> > +}
> >  
> >  /*
> >   * Command line options handling
> > @@ -249,10 +262,11 @@ static int parse_opt_oem_string(const char *arg)
> >  int parse_command_line(int argc, char * const argv[])
> >  {
> >     int option;
> > -   const char *optstring = "d:hqs:t:uV";
> > +   const char *optstring = "d:hqs:t:uHV";
> 
> You are missing a colon after "H" here. This causes getopt to not feed
> optarg when "-H" is passed, ultimately leading to a segmentation fault
> in parse_opt_handle.

Fixed.

> 
> >     struct option longopts[] = {
> >             { "dev-mem", required_argument, NULL, 'd' },
> >             { "help", no_argument, NULL, 'h' },
> > +           { "handle", required_argument, NULL, 'H' },
> >             { "quiet", no_argument, NULL, 'q' },
> >             { "string", required_argument, NULL, 's' },
> >             { "type", required_argument, NULL, 't' },
> 
> Please use the same order in short options list and long options list.
> The former fits better in the current scheme.


Fixed.


> 
> > @@ -295,6 +309,11 @@ int parse_command_line(int argc, char * const argv[])
> >                                     return -1;
> >                             opt.flags |= FLAG_QUIET;
> >                             break;
> > +                   case 'H':
> > +                           opt.handle = parse_opt_handle(optarg);
> > +                           if (opt.handle  == ~0U)
> > +                                   return -1;
> > +                           break;
> >                     case 't':
> >                             opt.type = parse_opt_type(opt.type, optarg);
> >                             if (opt.type == NULL)
> 
> Later in this function, there is a check for mutually exclusive
> options. I believe it should be extended to check this new option.
> 
> Note that the combination of --handle with -s could make sense in
> theory, if for example you want to get a processor-specific string on a
> multi-processor system, but it is not currently handled due to the
> logic in dmi_table_decode(). So for now they should be checked and
> advertised as mutually exclusive as well.

Fixed.

> 
> > @@ -351,6 +370,7 @@ void print_help(void)
> >             " -q, --quiet            Less verbose output\n"
> >             " -s, --string KEYWORD   Only display the value of the given 
> > DMI string\n"
> >             " -t, --type TYPE        Only display the entries of given 
> > type\n"
> > +           " -H, --handle HANDLE    Only display the entries of given 
> > handle\n"
> 
> "entry" (see below).


Fixed.


> 
> >             " -u, --dump             Do not decode the entries\n"
> >             "     --dump-bin FILE    Dump the DMI data to a binary file\n"
> >             "     --from-dump FILE   Read the DMI data from a binary file\n"
> > diff --git a/dmiopt.h b/dmiopt.h
> > index c676308..34adf3a 100644
> > --- a/dmiopt.h
> > +++ b/dmiopt.h
> > @@ -35,6 +35,7 @@ struct opt
> >     u8 *type;
> >     const struct string_keyword *string;
> >     char *dumpfile;
> > +   u32  handle;
> 
> Extra space before "handle".

Fixed.

> 
> >  };
> >  extern struct opt opt;
> >  
> > diff --git a/man/dmidecode.8 b/man/dmidecode.8
> > index e3b6b2a..858e56e 100644
> > --- a/man/dmidecode.8
> > +++ b/man/dmidecode.8
> > @@ -101,6 +101,10 @@ typically from files under
> >  .IR /sys/devices/virtual/dmi/id .
> >  Most of these files are even readable by regular users.
> >  .TP
> > +.BR "-H" ", " "--handle HANDLE"
> > +Only display the entries whose handle matches \fBHANDLE\fR.  \fBHANDLE\fR
> > +is a 16 bit integer.
> 
> The use of plural ("entries") is confusing because each handle number
> must be unique according to the SMBIOS specification.

OK. Wasn't sure handles were unique.  The code works either way.

I'll change to the singular form here and the other locations.


> 
> "16-bit" used as an adjective takes an hyphen, as far as I know.

Fixed.

> 
> > +.TP
> >  .BR "-t" ", " "--type TYPE"
> >  Only display the entries of type \fBTYPE\fR. \fBTYPE\fR can be either a
> >  \s-1DMI\s0 type number, or a comma-separated list of type numbers, or a
> 
> I would appreciate if options -H, -t and -s would show up in the same
> order everywhere. A the moment, you have s(O)Ht in parse_command_line(),
> stH in --help and sHt in the manual page. Especially the last two
> should be the same because they are user-visible.


Will make order:  stH


> 
> Thanks,
> -- 
> Jean Delvare
> SUSE L3 Support

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------