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

[Anthony PERARD <anthony.perard@xxxxxxxxx>]

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,

-- 
Anthony PERARD