Re: [PATCH v5 2/4] x86/ucode: refactor xen-ucode to utilize getopt

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Fri, Jul 12, 2024 at 02:07:47PM +0100, Fouad Hilly wrote:
> diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
> index 390969db3d1c..8de82e5b8a10 100644
> --- a/tools/misc/xen-ucode.c
> +++ b/tools/misc/xen-ucode.c
> @@ -71,12 +72,29 @@ static void show_curr_cpu(FILE *f)
>      }
>  }
>  
> +static void usage(FILE *stream, const char *name)
> +{
> +    fprintf(stream,
> +            "%s: Xen microcode updating tool\n"
> +            "options:\n"
> +            "  -h, --help            display this help\n"
> +            "  -s, --show-cpu-info   show CPU information\n"
> +            "Usage: %s [microcode file] [options]\n", name, name);

FYI, I disagree with Andy about the order of this message. First is
"Usage:" which explain where the option (dash-prefixed) can go, and
which are the mandatory arguments, sometime having all the single-letter
option in this line as well. Then there's an explanation of what the
options are. I've check `bash`, `cat`, `xl`, `gcc`.

I wonder which CLI program would print the minimum amount of information
on how to run the program as the last line of the help message.

> @@ -86,22 +104,34 @@ int main(int argc, char *argv[])
>          exit(1);
>      }
>  
> -    if ( argc < 2 )
> +    while ( (opt = getopt_long(argc, argv, "hs", options, NULL)) != -1 )
>      {
> -        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);
> +        switch (opt)
> +        {
> +        case 'h':
> +            usage(stdout, argv[0]);
> +            exit(EXIT_SUCCESS);
> +
> +        case 's':
> +            show_curr_cpu(stdout);
> +            exit(EXIT_SUCCESS);
> +
> +        default:
> +            goto ext_err;
> +        }
>      }
>  
> -    if ( !strcmp(argv[1], "show-cpu-info") )
> +    if ( optind == argc )
> +        goto ext_err;
> +
> +    /* For backwards compatibility to the pre-getopt() cmdline handling */
> +    if ( !strcmp(argv[optind], "show-cpu-info") )
>      {
>          show_curr_cpu(stdout);
>          return 0;
>      }
>  
> -    filename = argv[1];
> +    filename = argv[optind];
>      fd = open(filename, O_RDONLY);
>      if ( fd < 0 )
>      {
> @@ -146,4 +176,10 @@ int main(int argc, char *argv[])
>      close(fd);
>  
>      return 0;
> +
> + ext_err:
> +    fprintf(stderr,
> +            "%s: unable to process command line arguments\n", argv[0]);

A nice to have would be to have a better error message to point out
what's wrong with the arguments. For that you could print the error
message before "goto ext_err". One would be "unknown option" for the
first goto, and "missing microcode file" for the second goto, that is
instead of printing this more generic error message.

Cheers,

-- 

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech