Mailing List Archive: [PATCH 4/5] x86: Use getopt to handle command line args

[PATCH 4/5] x86: Use getopt to handle command line args

Apr 5, 2024, 5:11 AM

Post #1 of 3 (8 views)

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(-)

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);
}
--
2.42.0

Re: [PATCH 4/5] x86: Use getopt to handle command line args [ In reply to ]

jbeulich at suse

Apr 8, 2024, 2:28 AM

Post #2 of 3 (4 views)

Permalink

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

Re: [PATCH 4/5] x86: Use getopt to handle command line args [ In reply to ]

anthony.perard at cloud

Apr 10, 2024, 6:54 AM

Post #3 of 3 (2 views)

Permalink

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,

--
Anthony PERARD