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

[Jean Delvare]

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.

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

>                       && !((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.

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:

# 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
admittedly not perfect, but if anyone is unhappy with that, it should
be fixed for all options at once, for consistency.)

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

Typo: number.

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

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

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

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

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

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

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

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

Thanks,
-- 
Jean Delvare
SUSE L3 Support