| Message ID | 20240416091546.11622-5-fouad.hilly@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/xen-ucode: Introduce --force option | expand |
On Tue, Apr 16, 2024 at 10:15:45AM +0100, Fouad Hilly wrote: > diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c > index 0c0b2337b4ea..e3c1943e3633 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; > > @@ -22,7 +23,8 @@ static void usage(const char *name) > printf("%s: Xen microcode updating tool\n" > "Usage: %s [microcode file] [options]\n" > "Options:\n" > - "show-cou-info show CPU information and exit\n", > + " -h, --help display this help and exit\n" > + " -s, --show-cpu-info show CPU information and exit\n", > name, name); > } > > @@ -86,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} > + }; > > xch = xc_interface_open(NULL, NULL, 0); > if ( xch == NULL ) > @@ -95,19 +104,22 @@ int main(int argc, char *argv[]) > exit(1); > } > > - if ( argc < 2 ) > - { > - 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); > - } > + if ( argc != 2 ) > + goto ext_err; Why only two arguments allowed? And why check `argc` before calling getopt_long() ? > > - if ( !strcmp(argv[1], "show-cpu-info") ) > + while ( (opt = getopt_long(argc, argv, "hs", options, NULL)) != -1 ) > { > - show_curr_cpu(stdout); > - return 0; > + switch (opt) > + { > + case 'h': > + usage(argv[0]); > + exit(EXIT_SUCCESS); > + case 's': > + show_curr_cpu(stdout); > + exit(EXIT_SUCCESS); > + default: > + goto ext_err; > + } > } > > filename = argv[1]; So, after calling getopt_long(), the variable `optind` point to the next argument that getopt_long() didn't recognize as an argument. It would be a good time to start using it, and check that the are actually argument left on the command line, even if in the current patch the only possible outcome is that argv[1] has something that isn't an option. > @@ -152,4 +164,11 @@ int main(int argc, char *argv[]) > close(fd); > > return 0; > + > + ext_err: > + fprintf(stderr, > + "xen-ucode: Xen microcode updating tool\n" > + "Usage: %s [microcode file] [options]\n", argv[0]); Isn't there a usage() function that we could use? > + show_curr_cpu(stderr); Why call show_curr_cpu() on the error path? > + exit(STDERR_FILENO); Still not an exit value. Thanks,
On Mon, Apr 22, 2024 at 6:48 PM Anthony PERARD <anthony.perard@cloud.com> wrote: > > On Tue, Apr 16, 2024 at 10:15:45AM +0100, Fouad Hilly wrote: > > diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c > > index 0c0b2337b4ea..e3c1943e3633 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; > > > > @@ -22,7 +23,8 @@ static void usage(const char *name) > > printf("%s: Xen microcode updating tool\n" > > "Usage: %s [microcode file] [options]\n" > > "Options:\n" > > - "show-cou-info show CPU information and exit\n", > > + " -h, --help display this help and exit\n" > > + " -s, --show-cpu-info show CPU information and exit\n", > > name, name); > > } > > > > @@ -86,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} > > + }; > > > > xch = xc_interface_open(NULL, NULL, 0); > > if ( xch == NULL ) > > @@ -95,19 +104,22 @@ int main(int argc, char *argv[]) > > exit(1); > > } > > > > - if ( argc < 2 ) > > - { > > - 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); > > - } > > + if ( argc != 2 ) > > + goto ext_err; > > Why only two arguments allowed? And why check `argc` before calling > getopt_long() ? At this stage of the patch series, xen-ucode expects either firmware file or a single argument, that is why argc should be 2. If there is no argument or many arguments that are not supposed to be, we exit with error other than try to parse the arguments. > > > > > > - if ( !strcmp(argv[1], "show-cpu-info") ) > > + while ( (opt = getopt_long(argc, argv, "hs", options, NULL)) != -1 ) > > { > > - show_curr_cpu(stdout); > > - return 0; > > + switch (opt) > > + { > > + case 'h': > > + usage(argv[0]); > > + exit(EXIT_SUCCESS); > > + case 's': > > + show_curr_cpu(stdout); > > + exit(EXIT_SUCCESS); > > + default: > > + goto ext_err; > > + } > > } > > > > filename = argv[1]; > > So, after calling getopt_long(), the variable `optind` point to the next > argument that getopt_long() didn't recognize as an argument. It would be > a good time to start using it, and check that the are actually argument > left on the command line, even if in the current patch the only possible > outcome is that argv[1] has something that isn't an option. > > > @@ -152,4 +164,11 @@ int main(int argc, char *argv[]) > > close(fd); > > > > return 0; > > + > > + ext_err: > > + fprintf(stderr, > > + "xen-ucode: Xen microcode updating tool\n" > > + "Usage: %s [microcode file] [options]\n", argv[0]); > > Isn't there a usage() function that we could use? As there is an error, stderr should be used, which was a comment on V1. > > > + show_curr_cpu(stderr); > > Why call show_curr_cpu() on the error path? Informative, but could be removed. > > > + exit(STDERR_FILENO); > > Still not an exit value. > > Thanks, > > -- > Anthony PERARD Thanks, Fouad
On Tue, Apr 23, 2024 at 04:16:10PM +0100, Fouad Hilly wrote: > On Mon, Apr 22, 2024 at 6:48 PM Anthony PERARD <anthony.perard@cloud.com> wrote: > > On Tue, Apr 16, 2024 at 10:15:45AM +0100, Fouad Hilly wrote: > > > + if ( argc != 2 ) > > > + goto ext_err; > > > > Why only two arguments allowed? And why check `argc` before calling > > getopt_long() ? > At this stage of the patch series, xen-ucode expects either firmware > file or a single argument, that is why argc should be 2. > If there is no argument or many arguments that are not supposed to be, > we exit with error other than try to parse the arguments. Right, but what's the point of introducing getopt_long() if we are not going to use it? The patch tries to make it nicer to introduce more options to the tool but then put an extra check that make this actually harder. This condition is even change in the next patch, that's one more reason that says that it's a wrong check. You can let getopt_long() parse all the options, deal with them, then you can deal with the nonoptions after that, and check that's there's only one nonoption. > > > + ext_err: > > > + fprintf(stderr, > > > + "xen-ucode: Xen microcode updating tool\n" > > > + "Usage: %s [microcode file] [options]\n", argv[0]); > > > > Isn't there a usage() function that we could use? > As there is an error, stderr should be used, which was a comment on V1. > > > > > + show_curr_cpu(stderr); > > > > Why call show_curr_cpu() on the error path? > Informative, but could be removed. We are on the error path, it's looks like the usage message is printed before the cpu information, so if the error is due to wrong options then that information is lost. We should print why there's an error, not much else would be useful. Error messages should be as late as possible or they are getting lost in the scroll of the terminal. So yes, it's probably best to remove. Thanks,
diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c index 0c0b2337b4ea..e3c1943e3633 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; @@ -22,7 +23,8 @@ static void usage(const char *name) printf("%s: Xen microcode updating tool\n" "Usage: %s [microcode file] [options]\n" "Options:\n" - "show-cou-info show CPU information and exit\n", + " -h, --help display this help and exit\n" + " -s, --show-cpu-info show CPU information and exit\n", name, name); } @@ -86,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} + }; xch = xc_interface_open(NULL, NULL, 0); if ( xch == NULL ) @@ -95,19 +104,22 @@ int main(int argc, char *argv[]) exit(1); } - if ( argc < 2 ) - { - 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); - } + if ( argc != 2 ) + goto ext_err; - if ( !strcmp(argv[1], "show-cpu-info") ) + while ( (opt = getopt_long(argc, argv, "hs", options, NULL)) != -1 ) { - show_curr_cpu(stdout); - return 0; + switch (opt) + { + case 'h': + usage(argv[0]); + exit(EXIT_SUCCESS); + case 's': + show_curr_cpu(stdout); + exit(EXIT_SUCCESS); + default: + goto ext_err; + } } filename = argv[1]; @@ -152,4 +164,11 @@ int main(int argc, char *argv[]) close(fd); return 0; + + ext_err: + fprintf(stderr, + "xen-ucode: Xen microcode updating tool\n" + "Usage: %s [microcode file] [options]\n", argv[0]); + show_curr_cpu(stderr); + exit(STDERR_FILENO); }
Use getopt_long() to handle command line arguments. Introduce ext_err for common exit with errors. [v2] 1- Removed unnecessary NULL initialization. 2- Used static at the beginning of type struct declaration. 3- Corrected switch\case indentations. 4- Removed redundant checks. 5- Corrected label indentation. Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com> --- tools/misc/xen-ucode.c | 43 ++++++++++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 12 deletions(-)