| Message ID | 20240712130749.1272741-3-fouad.hilly@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/xen-ucode: Introduce --force option | expand |
On 12.07.2024 15:07, Fouad Hilly wrote: > --- a/tools/misc/xen-ucode.c > +++ b/tools/misc/xen-ucode.c > @@ -11,6 +11,7 @@ > #include <sys/stat.h> > #include <fcntl.h> > #include <xenctrl.h> > +#include <getopt.h> > > static xc_interface *xch; > > @@ -71,12 +72,29 @@ static void show_curr_cpu(FILE *f) > } > } > > +static void usage(FILE *stream, const char *name) > +{ > + fprintf(stream, > + "%s: Xen microcode updating tool\n" > + "options:\n" > + " -h, --help display this help\n" > + " -s, --show-cpu-info show CPU information\n" > + "Usage: %s [microcode file] [options]\n", name, name); Isn't it more like [microcode file | options] at this point? Even when --force support is added, neither of the two options here go together with a microcode file. > @@ -86,22 +104,34 @@ 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) Nit (style): Missing blanks inside the parentheses. Jan
On Tue, Jul 16, 2024 at 3:51 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 12.07.2024 15:07, Fouad Hilly wrote: > > --- a/tools/misc/xen-ucode.c > > +++ b/tools/misc/xen-ucode.c > > @@ -11,6 +11,7 @@ > > #include <sys/stat.h> > > #include <fcntl.h> > > #include <xenctrl.h> > > +#include <getopt.h> > > > > static xc_interface *xch; > > > > @@ -71,12 +72,29 @@ static void show_curr_cpu(FILE *f) > > } > > } > > > > +static void usage(FILE *stream, const char *name) > > +{ > > + fprintf(stream, > > + "%s: Xen microcode updating tool\n" > > + "options:\n" > > + " -h, --help display this help\n" > > + " -s, --show-cpu-info show CPU information\n" > > + "Usage: %s [microcode file] [options]\n", name, name); > > Isn't it more like [microcode file | options] at this point? Even when > --force support is added, neither of the two options here go together > with a microcode file. Yes, I will fix it in v6 > > > @@ -86,22 +104,34 @@ 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) > > Nit (style): Missing blanks inside the parentheses. Will be fixed in v6 > > Jan Thanks, Fouad
On Fri, Jul 12, 2024 at 02:07:47PM +0100, Fouad Hilly wrote: > diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c > index 390969db3d1c..8de82e5b8a10 100644 > --- a/tools/misc/xen-ucode.c > +++ b/tools/misc/xen-ucode.c > @@ -71,12 +72,29 @@ static void show_curr_cpu(FILE *f) > } > } > > +static void usage(FILE *stream, const char *name) > +{ > + fprintf(stream, > + "%s: Xen microcode updating tool\n" > + "options:\n" > + " -h, --help display this help\n" > + " -s, --show-cpu-info show CPU information\n" > + "Usage: %s [microcode file] [options]\n", name, name); FYI, I disagree with Andy about the order of this message. First is "Usage:" which explain where the option (dash-prefixed) can go, and which are the mandatory arguments, sometime having all the single-letter option in this line as well. Then there's an explanation of what the options are. I've check `bash`, `cat`, `xl`, `gcc`. I wonder which CLI program would print the minimum amount of information on how to run the program as the last line of the help message. > @@ -86,22 +104,34 @@ 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); > + > + case 's': > + show_curr_cpu(stdout); > + exit(EXIT_SUCCESS); > + > + default: > + goto ext_err; > + } > } > > - if ( !strcmp(argv[1], "show-cpu-info") ) > + 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; > } > > - filename = argv[1]; > + filename = argv[optind]; > fd = open(filename, O_RDONLY); > if ( fd < 0 ) > { > @@ -146,4 +176,10 @@ int main(int argc, char *argv[]) > close(fd); > > return 0; > + > + ext_err: > + fprintf(stderr, > + "%s: unable to process command line arguments\n", argv[0]); A nice to have would be to have a better error message to point out what's wrong with the arguments. For that you could print the error message before "goto ext_err". One would be "unknown option" for the first goto, and "missing microcode file" for the second goto, that is instead of printing this more generic error message. Cheers,
On Wed, Jul 24, 2024 at 5:55 PM Anthony PERARD <anthony.perard@vates.tech> wrote: > On Fri, Jul 12, 2024 at 02:07:47PM +0100, Fouad Hilly wrote: > > diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c > > index 390969db3d1c..8de82e5b8a10 100644 > > --- a/tools/misc/xen-ucode.c > > +++ b/tools/misc/xen-ucode.c > > @@ -71,12 +72,29 @@ static void show_curr_cpu(FILE *f) > > } > > } > > > > +static void usage(FILE *stream, const char *name) > > +{ > > + fprintf(stream, > > + "%s: Xen microcode updating tool\n" > > + "options:\n" > > + " -h, --help display this help\n" > > + " -s, --show-cpu-info show CPU information\n" > > + "Usage: %s [microcode file] [options]\n", name, name); > > FYI, I disagree with Andy about the order of this message. First is > "Usage:" which explain where the option (dash-prefixed) can go, and > which are the mandatory arguments, sometime having all the single-letter > option in this line as well. Then there's an explanation of what the > options are. I've check `bash`, `cat`, `xl`, `gcc`. > > I wonder which CLI program would print the minimum amount of information > on how to run the program as the last line of the help message. > My Bad, I misinterpreted Andy's comment, will fix in v7: static void usage(FILE *stream, const char *name) { fprintf(stream, "%s: Xen microcode updating tool\n" "Usage: %s [options | microcode-file]\n" "options:\n" " -h, --help display this help\n" " -s, --show-cpu-info show CPU information\n", name, name); show_curr_cpu(stream); } > > > @@ -86,22 +104,34 @@ 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); > > + > > + case 's': > > + show_curr_cpu(stdout); > > + exit(EXIT_SUCCESS); > > + > > + default: > > + goto ext_err; > > + } > > } > > > > - if ( !strcmp(argv[1], "show-cpu-info") ) > > + 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; > > } > > > > - filename = argv[1]; > > + filename = argv[optind]; > > fd = open(filename, O_RDONLY); > > if ( fd < 0 ) > > { > > @@ -146,4 +176,10 @@ int main(int argc, char *argv[]) > > close(fd); > > > > return 0; > > + > > + ext_err: > > + fprintf(stderr, > > + "%s: unable to process command line arguments\n", argv[0]); > > A nice to have would be to have a better error message to point out > what's wrong with the arguments. For that you could print the error > message before "goto ext_err". One would be "unknown option" for the > first goto, and "missing microcode file" for the second goto, that is > instead of printing this more generic error message. > Sure, I will have specific error messages instead of generic one in v7 > > Cheers, > > -- > > Anthony Perard | Vates XCP-ng Developer > > XCP-ng & Xen Orchestra - Vates solutions > > web: https://vates.tech Thanks, Fouad
diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c index 390969db3d1c..8de82e5b8a10 100644 --- a/tools/misc/xen-ucode.c +++ b/tools/misc/xen-ucode.c @@ -11,6 +11,7 @@ #include <sys/stat.h> #include <fcntl.h> #include <xenctrl.h> +#include <getopt.h> static xc_interface *xch; @@ -71,12 +72,29 @@ static void show_curr_cpu(FILE *f) } } +static void usage(FILE *stream, const char *name) +{ + fprintf(stream, + "%s: Xen microcode updating tool\n" + "options:\n" + " -h, --help display this help\n" + " -s, --show-cpu-info show CPU information\n" + "Usage: %s [microcode file] [options]\n", name, name); + show_curr_cpu(stream); +} + int main(int argc, char *argv[]) { + static const struct option options[] = { + {"help", no_argument, NULL, 'h'}, + {"show-cpu-info", no_argument, NULL, 's'}, + {NULL, no_argument, NULL, 0} + }; int fd, ret; char *filename, *buf; size_t len; struct stat st; + int opt; xch = xc_interface_open(NULL, NULL, 0); if ( xch == NULL ) @@ -86,22 +104,34 @@ 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); + + case 's': + show_curr_cpu(stdout); + exit(EXIT_SUCCESS); + + default: + goto ext_err; + } } - if ( !strcmp(argv[1], "show-cpu-info") ) + 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; } - filename = argv[1]; + filename = argv[optind]; fd = open(filename, O_RDONLY); if ( fd < 0 ) { @@ -146,4 +176,10 @@ int main(int argc, char *argv[]) close(fd); return 0; + + ext_err: + fprintf(stderr, + "%s: unable to process command line arguments\n", argv[0]); + usage(stderr, argv[0]); + exit(EXIT_FAILURE); }
Use getopt_long() to handle command line arguments. Introduce ext_err for common exit with errors. Introducing usage() to handle usage\help messages in a common block. show_curr_cpu is printed to stdout only. Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com> --- [v5] 1- Update message description. 2- re-arrange static and automatic variables. 3- Fix indentations. 4- reverted the deletion of show-cpu-info for backwards compatibility. [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 | 52 +++++++++++++++++++++++++++++++++++------- 1 file changed, 44 insertions(+), 8 deletions(-)