On 28/05/2024 4:29 pm, Fouad Hilly wrote: > Use getopt_long() to handle command line arguments. > Introduce ext_err for common exit with errors. > xc_microcode_update() refactored to accept flags and utilize > xenpf_microcode_update2
Importantly, not. That's deferred until the next patch. > Introducing usage() to handle usage\help messages in a common block. > show_curr_cpu is printed to stdout only. > > Signed-off-by: Fouad Hilly <[email protected]> > --- > [v4] > 1- Merge three patches into one. > 2- usage() to print messages to the correct stream. > 3- Update commit message and description. > --- > tools/misc/xen-ucode.c | 51 ++++++++++++++++++++++++++++++++---------- > 1 file changed, 39 insertions(+), 12 deletions(-) > > diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c > index 390969db3d1c..6f9dd2a7e431 100644 > --- a/tools/misc/xen-ucode.c > +++ b/tools/misc/xen-ucode.c > @@ -11,12 +11,23 @@ > #include <sys/stat.h> > #include <fcntl.h> > #include <xenctrl.h> > +#include <getopt.h> > > static xc_interface *xch; > > static const char intel_id[] = "GenuineIntel"; > static const char amd_id[] = "AuthenticAMD"; > > +static void usage(FILE *stream, const char *name) > +{ > + fprintf(stream, "%s: Xen microcode updating tool\n" > + "Usage: %s [microcode file] [options]\n" Options are typically expressed first when writing this. > + "options:\n" > + " -h, --help display this help and exit\n" > + " -s, --show-cpu-info show CPU information and exit\n", The "and exit" is pointless to state like this. > + name, name); Alignment is out by one column here. Also, previously, usage() was always followed by show_curr_cpu(). That's still easy to retain by reordering the two static functions. > +} > + > static void show_curr_cpu(FILE *f) > { > int ret; > @@ -77,6 +88,13 @@ int main(int argc, char *argv[]) > char *filename, *buf; > size_t len; > struct stat st; > + int opt; > + > + static const struct option options[] = { > + {"help", no_argument, NULL, 'h'}, > + {"show-cpu-info", no_argument, NULL, 's'}, > + {NULL, no_argument, NULL, 0} > + }; Typically we prefer static variables before automatic variables. This block can just be repositioned. > > xch = xc_interface_open(NULL, NULL, 0); > if ( xch == NULL ) > @@ -86,22 +104,25 @@ int main(int argc, char *argv[]) > exit(1); > } > > - if ( argc < 2 ) > + while ( (opt = getopt_long(argc, argv, "hs", options, NULL)) != -1 ) > { > - fprintf(stderr, > - "xen-ucode: Xen microcode updating tool\n" > - "Usage: %s [<microcode file> | show-cpu-info]\n", argv[0]); > - show_curr_cpu(stderr); > - exit(2); > + switch (opt) > + { > + case 'h': > + usage(stdout, argv[0]); > + exit(EXIT_SUCCESS); Blank line here, ... > + case 's': > + show_curr_cpu(stdout); > + exit(EXIT_SUCCESS); ... and here. > + default: > + goto ext_err; > + } > } > > - if ( !strcmp(argv[1], "show-cpu-info") ) > - { > - show_curr_cpu(stdout); > - return 0; > - } > + if ( optind == argc ) > + goto ext_err; This is a change in API, because now "show-cpu-info" needs to take a -- to be interpreted. I suspect we want: if ( optind == argc ) goto ext_err; /* For backwards compatibility to the pre-getopt() cmdline handling */ if ( !strcmp(argv[optind], "show-cpu-info") ) { show_curr_cpu(stdout); return 0; } to retain compatibility. (Also, this means you won't have the code rejected by XenRT when this gets added to Xen.) All of this I can fix on commit, but there's quite a lot in the way of changes. ~Andrew
