[04/28] qemu-img: global option processing and error printing

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 whole argv[] array, so argv[0] is
the executable name, not subcommand name.  In order to
do that, avoid resetting optind but continue with the
next option.  Also don't require at least 3 options on
the command line: it makes no sense with options before
subcommand.

Before invoking a subcommand, replace argv[0] to include
the subcommand name.

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 | 75 +++++++++++++++++++++++++++---------------------------
 1 file changed, 38 insertions(+), 37 deletions(-)

On Thu, Feb 22, 2024 at 12:15:45AM +0300, Michael Tokarev wrote:
> 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 whole argv[] array, so argv[0] is
> the executable name, not subcommand name.  In order to
> do that, avoid resetting optind but continue with the
> next option.  Also don't require at least 3 options on
> the command line: it makes no sense with options before
> subcommand.
> 
> Before invoking a subcommand, replace argv[0] to include
> the subcommand name.
> 
> 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 | 75 +++++++++++++++++++++++++++---------------------------
>  1 file changed, 38 insertions(+), 37 deletions(-)
> @@ -5602,10 +5602,11 @@ int main(int argc, char **argv)
>      /* find the command */
>      for (cmd = img_cmds; cmd->name != NULL; cmd++) {
>          if (!strcmp(cmdname, cmd->name)) {
> +            argv[0] = g_strdup_printf("%s %s", argv[0], cmdname);
>              return cmd->handler(argc, argv);

This is going to result in valgrind warning that argv[0] is leaked.

How about:

  g_autofree char *cmdargv0 = g_strdup_printf("%s %s", argv[0], cmdname);
  argv[0] = cmdargv0;
  return cmd->handler(argc, argv);

>          }
>      }
>  
>      /* not found */
> -    error_exit("Command not found: %s", cmdname);
> +    error_exit(argv[0], "Command not found: %s", cmdname);
>  }
> -- 
> 2.39.2
> 
> 

With regards,
Daniel

On Thu, Feb 22, 2024 at 12:15:45AM +0300, Michael Tokarev wrote:
> 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 whole argv[] array, so argv[0] is
> the executable name, not subcommand name.  In order to
> do that, avoid resetting optind but continue with the
> next option.  Also don't require at least 3 options on
> the command line: it makes no sense with options before
> subcommand.
> 
> Before invoking a subcommand, replace argv[0] to include
> the subcommand name.
> 
> 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 | 75 +++++++++++++++++++++++++++---------------------------
>  1 file changed, 38 insertions(+), 37 deletions(-)

I'm not sure how, but this change seems to have broken the iotests.
Just one example:

$ (cd  tests/qemu-iotests/ && ./check -qcow2 249)
QEMU          -- "/var/home/berrange/src/virt/qemu/build/qemu-system-x86_64" -nodefaults -display none -accel qtest
QEMU_IMG      -- "/var/home/berrange/src/virt/qemu/build/qemu-img" 
QEMU_IO       -- "/var/home/berrange/src/virt/qemu/build/qemu-io" --cache writeback --aio threads -f qcow2
QEMU_NBD      -- "/var/home/berrange/src/virt/qemu/build/qemu-nbd" 
IMGFMT        -- qcow2
IMGPROTO      -- file
PLATFORM      -- Linux/x86_64 toolbox 6.6.12-200.fc39.x86_64
TEST_DIR      -- /var/home/berrange/src/virt/qemu/build/tests/qemu-iotests/scratch
SOCK_DIR      -- /tmp/qemu-iotests-0t8h94bu
GDB_OPTIONS   -- 
VALGRIND_QEMU -- 
PRINT_QEMU_OUTPUT -- 

249   fail       [15:39:25] [15:39:25]   0.2s   (last: 0.4s)  failed, exit status 1
--- /var/home/berrange/src/virt/qemu/tests/qemu-iotests/249.out
+++ /var/home/berrange/src/virt/qemu/build/tests/qemu-iotests/scratch/qcow2-file-249/249.out.bad
@@ -1,47 +1,7 @@
 QA output created by 249
 Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=1048576
-Formatting 'TEST_DIR/t.IMGFMT.int', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT.base backing_fmt=IMGFMT
-Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 backing_file=TEST_DIR/t.IMGFMT.int backing_fmt=IMGFMT
-{ 'execute': 'qmp_capabilities' }
-{"return": {}}

...snip....

-*** done
+qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for
+qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes.
+qemu-img: Invalid image size specified. You may use k, M, G, T, P or E suffixes for
+qemu-img: kilobytes, megabytes, gigabytes, terabytes, petabytes and exabytes.
+Timeout waiting for capabilities on handle 0
Failures: 249
Failed 1 of 1 iotests



With regards,
Daniel

26.02.2024 18:40, Daniel P. Berrangé :
..
> I'm not sure how, but this change seems to have broken the iotests.
> Just one example:

Heh.  Thank you for trying that.  I wanted to do that but forgot.

The reason is most likely the argv/argc handling (lack of optind reset).
In the later change it is fixed but at that stage it's broken.

I'll take a look later today.

/mjt

26.02.2024 18:43, Michael Tokarev wrote:

> The reason is most likely the argv/argc handling (lack of optind reset).
> In the later change it is fixed but at that stage it's broken.

Nope. GNU getopt_long really needs resetting the state.
Or else it keeps return_in_order/permute/etc setting from
the previous init call.

So this patch needs tweaking, - the reset must be kept, and argv[0] init
should be done a bit differently.

An easy change.

/mjt