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

[Anthony PERARD <anthony.perard@xxxxxxxxx>]

On Tue, Apr 23, 2024 at 04:16:10PM +0100, Fouad Hilly wrote:
> On Mon, Apr 22, 2024 at 6:48 PM Anthony PERARD <anthony.perard@xxxxxxxxx> 
> wrote:
> > On Tue, Apr 16, 2024 at 10:15:45AM +0100, Fouad Hilly wrote:
> > > +    if ( argc != 2 )
> > > +        goto ext_err;
> >
> > Why only two arguments allowed? And why check `argc` before calling
> > getopt_long() ?
> At this stage of the patch series, xen-ucode expects either firmware
> file or a single argument, that is why argc should be 2.
> If there is no argument or many arguments that are not supposed to be,
> we exit with error other than try to parse the arguments.

Right, but what's the point of introducing getopt_long() if we are not
going to use it? The patch tries to make it nicer to introduce more
options to the tool but then put an extra check that make this actually
harder. This condition is even change in the next patch, that's one more
reason that says that it's a wrong check.

You can let getopt_long() parse all the options, deal with them, then
you can deal with the nonoptions after that, and check that's there's
only one nonoption.

> > > + 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?
> As there is an error, stderr should be used, which was a comment on V1.
> >
> > > +    show_curr_cpu(stderr);
> >
> > Why call show_curr_cpu() on the error path?
> Informative, but could be removed.

We are on the error path, it's looks like the usage message is printed
before the cpu information, so if the error is due to wrong options
then that information is lost. We should print why there's an error, not
much else would be useful. Error messages should be as late as possible
or they are getting lost in the scroll of the terminal. So yes, it's
probably best to remove.

Thanks,

-- 
Anthony PERARD