On Wed, Jul 24, 2024 at 5:55 PM Anthony PERARD <[email protected]>
wrote:
> 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.
>
My Bad, I misinterpreted Andy's comment, will fix in v7:
static void usage(FILE *stream, const char *name)
{
fprintf(stream,
"%s: Xen microcode updating tool\n"
"Usage: %s [options | microcode-file]\n"
"options:\n"
" -h, --help display this help\n"
" -s, --show-cpu-info show CPU information\n",
name, name);
show_curr_cpu(stream);
}
>
> > @@ -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.
>
Sure, I will have specific error messages instead of generic one in v7
>
> Cheers,
>
> --
>
> Anthony Perard | Vates XCP-ng Developer
>
> XCP-ng & Xen Orchestra - Vates solutions
>
> web: https://vates.tech
Thanks,
Fouad
