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 <[email protected]> > > --- > > 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 ----------------------------------------------------------------------------- _______________________________________________ https://lists.nongnu.org/mailman/listinfo/dmidecode-devel
