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

[Anthony PERARD <anthony.perard@xxxxxxxxx>]

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


F