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

[Michael Tokarev]

26.02.2024 17:52, Daniel P. Berrangé wrote:
> 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.

Yes, this is done in the next patch, "resize: refresh options/help".

> >                           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.


I was thinking about
  qemu-img resize foo -43 -f qcow2 ..

if not only to make it all consistent with everything else
(options has always been recognized after non-optional args
in gnu/linux world, all utils does that).

But in all scenarios, after changing first char of optstring to include
'-', this code will be called for any non-optional argument.  In this
case, it will be done for argument `foo', and there. -43 will  be
recognized by this piece of code as a size modification since it
starts with minus and follows with a number.

The handling of positional args after the getopt loop is also needed
to handle situations like

  qemu-img resize -- foo 43

-- everything after `--' will be left to that code.

/mjt