| Message ID | 20240405121128.260493-5-fouad.hilly@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/xen-ucode: Introduce --force option | expand |
On 05.04.2024 14:11, Fouad Hilly wrote: > @@ -82,9 +86,16 @@ static void show_curr_cpu(FILE *f) > int main(int argc, char *argv[]) > { > int fd, ret; > - char *filename, *buf; > + char *filename = NULL, *buf; Why? > size_t len; > struct stat st; > + int opt; > + > + const static struct option options[] = { Nit: Canonically "static" comes first and all type information is kept together. > + {"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 ) > @@ -94,20 +105,33 @@ int main(int argc, char *argv[]) > exit(1); > } > > - if ( argc < 2 ) > + if ( argc != 2 ) > + goto ext_err; > + > + while ( (opt = getopt_long(argc, argv, "hs:", options, NULL)) != -1 ) > { > - usage(argv[0]); > - show_curr_cpu(stderr); > - exit(2); > + switch (opt) > + { > + case 'h': > + usage(argv[0]); > + exit(EXIT_SUCCESS); > + case 's': > + if ( argc > 2 ) > + goto ext_err; This looks redundant with the earlier check that you adjust above. > + show_curr_cpu(stdout); > + exit(EXIT_SUCCESS); > + default: > + goto ext_err; > + } Nit: Case labels want indenting the same as the immediately enclosing braces. > } > > - if ( !strcmp(argv[1], "show-cpu-info") ) > + filename = argv[1]; > + if ( filename == NULL ) Can this really happen when argc == 2? > { > - show_curr_cpu(stdout); > - return 0; > + printf("File name error\n"); > + goto ext_err; > } > > - filename = argv[1]; > fd = open(filename, O_RDONLY); > if ( fd < 0 ) > { > @@ -149,4 +173,9 @@ int main(int argc, char *argv[]) > close(fd); > > return 0; > + > +ext_err: Labels indented by at least on blank please. Jan
On Fri, Apr 05, 2024 at 01:11:27PM +0100, Fouad Hilly wrote: > diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c > index 1edcebfb9f9c..9bde991c5df5 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; > > @@ -20,7 +21,10 @@ static const char amd_id[] = "AuthenticAMD"; > static void usage(const char *name) > { > printf("%s: Xen microcode updating tool\n" > - "Usage: %s [<microcode file> | show-cpu-info]\n" > + "Usage: %s [<microcode file> | --show-cpu-info]\n" This look like a change worth mentioning to users, can we add something in the CHANGELOG to say "show-cpu-info" is no longer an option and users/admin should use "--show-cpu-info" instead? > + "\n" > + " -h, --help display this help and exit\n" > + " -s, --show-cpu-info show CPU information and exit\n" > "\n" > , name, name); > } > @@ -82,9 +86,16 @@ static void show_curr_cpu(FILE *f) > int main(int argc, char *argv[]) > { > int fd, ret; > - char *filename, *buf; > + char *filename = NULL, *buf; > size_t len; > struct stat st; > + int opt; > + > + const static 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 ) > @@ -94,20 +105,33 @@ int main(int argc, char *argv[]) > exit(1); > } > > - if ( argc < 2 ) > + if ( argc != 2 ) This is overly restrictive, and doesn't need to be, especially when this patch introduces the use of getopt_long(). > + goto ext_err; > + > + while ( (opt = getopt_long(argc, argv, "hs:", options, NULL)) != -1 ) `-s` requires an argument but `--show-cpu-info`, looks there's an extra ':' in the `optstring`, it should read "hs", not "hs:". > { > - usage(argv[0]); > - show_curr_cpu(stderr); > - exit(2); > + switch (opt) > + { > + case 'h': > + usage(argv[0]); > + exit(EXIT_SUCCESS); > + case 's': > + if ( argc > 2 ) Why is `-s` only allowed alone? What if want to include some other option like "--json" to print the cpu-info in a different format? I think one way to deal with this would be to record the fact that we want to display the cpu information, and after the getopt_long() loop, check that they are no more arguments. (Check out `optind` in the man page) > + goto ext_err; > + show_curr_cpu(stdout); > + exit(EXIT_SUCCESS); > + default: > + goto ext_err; > + } > } > > - if ( !strcmp(argv[1], "show-cpu-info") ) > + filename = argv[1]; > + if ( filename == NULL ) > { > - show_curr_cpu(stdout); > - return 0; > + printf("File name error\n"); > + goto ext_err; > } > > - filename = argv[1]; > fd = open(filename, O_RDONLY); > if ( fd < 0 ) > { > @@ -149,4 +173,9 @@ int main(int argc, char *argv[]) > close(fd); > > return 0; > + > +ext_err: > + usage(argv[0]); > + show_curr_cpu(stderr); Why is show_curr_cpu() called on an error path? > + exit(STDERR_FILENO); STDERR_FILENO isn't an exit code, it's a file descriptor. Thanks,
diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c index 1edcebfb9f9c..9bde991c5df5 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; @@ -20,7 +21,10 @@ static const char amd_id[] = "AuthenticAMD"; static void usage(const char *name) { printf("%s: Xen microcode updating tool\n" - "Usage: %s [<microcode file> | show-cpu-info]\n" + "Usage: %s [<microcode file> | --show-cpu-info]\n" + "\n" + " -h, --help display this help and exit\n" + " -s, --show-cpu-info show CPU information and exit\n" "\n" , name, name); } @@ -82,9 +86,16 @@ static void show_curr_cpu(FILE *f) int main(int argc, char *argv[]) { int fd, ret; - char *filename, *buf; + char *filename = NULL, *buf; size_t len; struct stat st; + int opt; + + const static 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 ) @@ -94,20 +105,33 @@ int main(int argc, char *argv[]) exit(1); } - if ( argc < 2 ) + if ( argc != 2 ) + goto ext_err; + + while ( (opt = getopt_long(argc, argv, "hs:", options, NULL)) != -1 ) { - usage(argv[0]); - show_curr_cpu(stderr); - exit(2); + switch (opt) + { + case 'h': + usage(argv[0]); + exit(EXIT_SUCCESS); + case 's': + if ( argc > 2 ) + goto ext_err; + show_curr_cpu(stdout); + exit(EXIT_SUCCESS); + default: + goto ext_err; + } } - if ( !strcmp(argv[1], "show-cpu-info") ) + filename = argv[1]; + if ( filename == NULL ) { - show_curr_cpu(stdout); - return 0; + printf("File name error\n"); + goto ext_err; } - filename = argv[1]; fd = open(filename, O_RDONLY); if ( fd < 0 ) { @@ -149,4 +173,9 @@ int main(int argc, char *argv[]) close(fd); return 0; + +ext_err: + usage(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. Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com> --- tools/misc/xen-ucode.c | 49 +++++++++++++++++++++++++++++++++--------- 1 file changed, 39 insertions(+), 10 deletions(-)