Re: [PATCH 03/27] qemu-img: global option processing and error printing

[Kevin Wolf]

Am 27.09.2024 um 08:10 hat Michael Tokarev geschrieben:
> In order to correctly print executable name in various
> error messages, pass argv[0] to error_exit() function.
> This way, error messages will refer to actual executable
> name, which may be different from 'qemu-img'.
> 
> For subcommands, pass original command name from the
> qemu-img argv[0], plus the subcommand name, as its own
> argv[0] element, so error messages can be more useful.
> Also don't require at least 3 options on the command
> line: it makes no sense with options before subcommand.
> 
> Introduce tryhelp() function which just prints
> 
>  try 'command-name --help' for more info
> 
> and exits.  When tryhelp() is called from within a subcommand
> handler, the message will look like:
> 
>  try 'command-name subcommand --help' for more info
> 
> qemu-img uses getopt_long() with ':' as the first char in
> optstring parameter, which means it doesn't print error
> messages but return ':' or '?' instead, and qemu-img uses
> unrecognized_option() or missing_argument() function to
> print error messages.  But it doesn't quite work:
> 
>  $ ./qemu-img -xx
>  qemu-img: unrecognized option './qemu-img'
> 
> so the aim is to let getopt_long() to print regular error
> messages instead (removing ':' prefix from optstring) and
> remove handling of '?' and ':' "options" entirely.  With
> concatenated argv[0] and the subcommand, it all finally
> does the right thing in all cases.  This will be done in
> subsequent changes command by command, with main() done
> last.
> 
> unrecognized_option() and missing_argument() functions
> prototypes aren't changed by this patch, since they're
> called from many places and will be removed a few patches
> later.  Only artifical "qemu-img" argv0 is provided in
> there for now.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  qemu-img.c | 80 +++++++++++++++++++++++++++++-------------------------
>  1 file changed, 43 insertions(+), 37 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index fe22986931..130188e287 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -101,8 +101,15 @@ static void format_print(void *opaque, const char *name)
>      printf(" %s", name);
>  }
>  
> -static G_NORETURN G_GNUC_PRINTF(1, 2)
> -void error_exit(const char *fmt, ...)
> +static G_NORETURN
> +void tryhelp(const char *argv0)
> +{
> +    error_printf("Try '%s --help' for more info\n", argv0);
> +    exit(EXIT_FAILURE);
> +}
> +
> +static G_NORETURN G_GNUC_PRINTF(2, 3)
> +void error_exit(const char *argv0, const char *fmt, ...)
>  {
>      va_list ap;
>  
> @@ -110,20 +117,19 @@ void error_exit(const char *fmt, ...)
>      error_vreport(fmt, ap);
>      va_end(ap);
>  
> -    error_printf("Try 'qemu-img --help' for more information\n");
> -    exit(EXIT_FAILURE);
> +    tryhelp(argv0);
>  }

No reason to change "information" into the more colloquial "info". I
would leave it as it is.

Apart from that:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>