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.per...@cloud.com> > 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
