Re: [PATCH 19/28] qemu-img: resize: do not always eat last argument

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Feb 22, 2024 at 12:16:00AM +0300, Michael Tokarev wrote:
> 'qemu-img resize --help' does not work, since it wants more
> arguments.  Also it -size is only recognized as a very last
> argument, but it is common for tools to handle other options
> after positional arguments too.
> 
> Tell getopt_long() to return non-options together with options,
> and process filename and size in the loop, and check if there's
> an argument right after filename which looks like -N (number),
> and treat it as size (decrement).  This way we can handle --help,
> and we can also have options after filename and size, and `--'
> will be handled fine too.
> 
> The only case which is not handled right is when there's an option
> between filename and size, and size is given as decrement, - in
> this case -size will be treated as option, not as size.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>  qemu-img.c | 41 +++++++++++++++++++++++++++--------------
>  1 file changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/qemu-img.c b/qemu-img.c
> index 2a4bff2872..c8b0b68d67 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -4296,7 +4296,7 @@ static int img_resize(const img_cmd_t *ccmd, int argc, char **argv)
>  {
>      Error *err = NULL;
>      int c, ret, relative;
> -    const char *filename, *fmt, *size;
> +    const char *filename = NULL, *fmt = NULL, *size = NULL;
>      int64_t n, total_size, current_size;
>      bool quiet = false;
>      BlockBackend *blk = NULL;
> @@ -4319,17 +4319,7 @@ static int img_resize(const img_cmd_t *ccmd, int argc, char **argv)
>      bool image_opts = false;
>      bool shrink = false;
>  
> -    /* Remove size from argv manually so that negative numbers are not treated
> -     * as options by getopt. */
> -    if (argc < 3) {
> -        error_exit(argv[0], "Not enough arguments");
> -        return 1;
> -    }
> -
> -    size = argv[--argc];
> -
>      /* Parse getopt arguments */
> -    fmt = NULL;
>      for(;;) {
>          static const struct option long_options[] = {
>              {"help", no_argument, 0, 'h'},
> @@ -4339,7 +4329,7 @@ static int img_resize(const img_cmd_t *ccmd, int argc, char **argv)
>              {"shrink", no_argument, 0, OPTION_SHRINK},
>              {0, 0, 0, 0}
>          };
> -        c = getopt_long(argc, argv, ":f:hq",
> +        c = getopt_long(argc, argv, "-:f:hq",

In other patches you removed the initial ':' from gopt_long arg strings.

>                          long_options, NULL);
>          if (c == -1) {
>              break;
> @@ -4377,12 +4367,35 @@ static int img_resize(const img_cmd_t *ccmd, int argc, char **argv)
>          case OPTION_SHRINK:
>              shrink = true;
>              break;
> +        case 1: /* a non-optional argument */
> +            if (!filename) {
> +                filename = optarg;
> +                /* see if we have -size (number) next to filename */
> +                if (optind < argc) {
> +                    size = argv[optind];
> +                    if (size[0] == '-' && size[1] >= '0' && size[1] <= '9') {
> +                        ++optind;
> +                    } else {
> +                        size = NULL;
> +                    }
> +                }
> +            } else if (!size) {
> +                size = optarg;
> +            } else {
> +                error_exit(argv[0], "Extra argument(s) in command line");
> +            }
> +            break;

Can you say what scenario exercises this code 'case 1' ?  I couldn't
get it to run in any scenarios i tried, and ineed removing this,
and removing the 'getopt_long' change, I could still run  'qemu-img resize --help'
OK, and also run 'qemu-img resize foo -43' too.

>          }
>      }
> -    if (optind != argc - 1) {
> +    if (!filename && optind < argc) {
> +        filename = argv[optind++];
> +    }
> +    if (!size && optind < argc) {
> +        size = argv[optind++];
> +    }
> +    if (!filename || !size || optind < argc) {
>          error_exit(argv[0], "Expecting image file name and size");
>      }
> -    filename = argv[optind++];
>  
>      /* Choose grow, shrink, or absolute resize mode */
>      switch (size[0]) {
> -- 
> 2.39.2
> 
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|