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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/05/2024 4:29 pm, Fouad Hilly wrote:
> Use getopt_long() to handle command line arguments.
> Introduce ext_err for common exit with errors.
> xc_microcode_update() refactored to accept flags and utilize 
> xenpf_microcode_update2

Importantly, not.  That's deferred until the next patch.

> Introducing usage() to handle usage\help messages in a common block.
> show_curr_cpu is printed to stdout only.
>
> Signed-off-by: Fouad Hilly <fouad.hilly@xxxxxxxxx>
> ---
> [v4]
> 1- Merge three patches into one.
> 2- usage() to print messages to the correct stream.
> 3- Update commit message and description.
> ---
>  tools/misc/xen-ucode.c | 51 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 39 insertions(+), 12 deletions(-)
>
> diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
> index 390969db3d1c..6f9dd2a7e431 100644
> --- a/tools/misc/xen-ucode.c
> +++ b/tools/misc/xen-ucode.c
> @@ -11,12 +11,23 @@
>  #include <sys/stat.h>
>  #include <fcntl.h>
>  #include <xenctrl.h>
> +#include <getopt.h>
>  
>  static xc_interface *xch;
>  
>  static const char intel_id[] = "GenuineIntel";
>  static const char   amd_id[] = "AuthenticAMD";
>  
> +static void usage(FILE *stream, const char *name)
> +{
> +    fprintf(stream, "%s: Xen microcode updating tool\n"
> +           "Usage: %s [microcode file] [options]\n"

Options are typically expressed first when writing this.

> +           "options:\n"
> +           "  -h, --help            display this help and exit\n"
> +           "  -s, --show-cpu-info   show CPU information and exit\n",

The "and exit" is pointless to state like this.

> +           name, name);

Alignment is out by one column here.

Also, previously, usage() was always followed by show_curr_cpu(). 
That's still easy to retain by reordering the two static functions.

> +}
> +
>  static void show_curr_cpu(FILE *f)
>  {
>      int ret;
> @@ -77,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}
> +    };

Typically we prefer static variables before automatic variables.  This
block can just be repositioned.

>  
>      xch = xc_interface_open(NULL, NULL, 0);
>      if ( xch == NULL )
> @@ -86,22 +104,25 @@ 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);

Blank line here, ...

> +        case 's':
> +            show_curr_cpu(stdout);
> +            exit(EXIT_SUCCESS);

... and here.

> +        default:
> +            goto ext_err;
> +        }
>      }
>  
> -    if ( !strcmp(argv[1], "show-cpu-info") )
> -    {
> -        show_curr_cpu(stdout);
> -        return 0;
> -    }
> +    if ( optind == argc )
> +        goto ext_err;

This is a change in API, because now "show-cpu-info" needs to take a --
to be interpreted.

I suspect we want:

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

to retain compatibility.  (Also, this means you won't have the code
rejected by XenRT when this gets added to Xen.)

All of this I can fix on commit, but there's quite a lot in the way of
changes.

~Andrew