Re: [PATCH v2 25/31] qemu-img: Use user_creatable_process_cmdline() for --object

[Eric Blake <eblake@redhat.com>]

On 2/26/21 3:56 PM, Eric Blake wrote:
> On 2/24/21 7:52 AM, Kevin Wolf wrote:
>> This switches qemu-img from a QemuOpts-based parser for --object to
>> user_creatable_process_cmdline() which uses a keyval parser and enforces
>> the QAPI schema.
>>
>> Apart from being a cleanup, this makes non-scalar properties accessible.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  qemu-img.c | 239 ++++++++---------------------------------------------
>>  1 file changed, 33 insertions(+), 206 deletions(-)
>>
> 
>> @@ -1423,15 +1373,9 @@ static int img_compare(int argc, char **argv)
>>          case 'U':
>>              force_share = true;
>>              break;
>> -        case OPTION_OBJECT: {
>> -            QemuOpts *opts;
>> -            opts = qemu_opts_parse_noisily(&qemu_object_opts,
>> -                                           optarg, true);
>> -            if (!opts) {
>> -                ret = 2;
>> -                goto out4;
> 
> Our exit status here of 2 on failure appears to be intentional (since we
> reserve 0 for identical, 1 for mismatch, >1 for error)...
> 
>> -            }
>> -        }   break;
>> +        case OPTION_OBJECT:
>> +            user_creatable_process_cmdline(optarg);
>> +            break;
> 
> ...but becomes 1 here.  Does that matter?
> 
> /me goes and tests...
> 
> Ouch: with current qemu.git master and none of this series applied:
> 
> $ ./qemu-img compare --object foo,id=x /dev/null /dev/null
> qemu-img: invalid object type: foo
> $ echo $?
> 1

Okay, that didn't do what I expected, but this does:

$ ./qemu-img compare --object foo,id=1 /dev/null /dev/null
qemu-img: Parameter 'id' expects an identifier
Identifiers consist of letters, digits, '-', '.', '_', starting with a
letter.
$ echo $?
2

> $ gdb --args ./qemu-img compare --object foo,id=x /dev/null /dev/null
> (gdb) b qemu_opts_pars
> (gdb) r
> (gdb) fin
> Run till exit from #0  qemu_opts_parse_noisily (
>     list=0x55555578f020 <qemu_object_opts>, params=0x7fffffffd8a8
> "foo,id=x",
>     permit_abbrev=true) at ../util/qemu-option.c:948
> 0x00005555555805f9 in img_compare (argc=5, argv=0x7fffffffd480)
>     at ../qemu-img.c:1428
> 1428	            opts = qemu_opts_parse_noisily(&qemu_object_opts,
> Value returned is $1 = (QemuOpts *) 0x55555583b4b0
> (gdb) p *opts
> $3 = {id = 0x5555557a0d58 <qemu_trace_opts+24> "`\264\203UUU", list = 0x51,

and this may be my confusion with gdb.  Right after 'fin', *opts is not
the same as *$1 (apparently gdb has stopped at a point where the 'opts'
currently in scope is not the opts set by qemu_opts_parse_noisily, but
before the opts in scope has actually been assigned the returned value).

> 
> That looks buggy.  qemu_opts_parse_noisily() is NOT returning NULL, but
> rather a pointer to something garbage (that id pointing to a garbage
> string in the middle of qemu_trace_opts is fishy), and so we've been
> exiting with status 1 in spite of the code.
> 
> Looks like we'll want a separate patch fixing that first.

So I was wrong on when qemu_opts_parse_noisily() returns NULL - it does
NOT reject unknown object names (that was the job of the
qemu_opts_foreach call later), but merely rejects bad/duplicate ids.
Thus this code was indeed giving an exit status of 2 when actually
triggered correctly,

> 
>>          case OPTION_IMAGE_OPTS:
>>              image_opts = true;
>>              break;
>> @@ -1450,13 +1394,6 @@ static int img_compare(int argc, char **argv)
>>      filename1 = argv[optind++];
>>      filename2 = argv[optind++];
>>  
>> -    if (qemu_opts_foreach(&qemu_object_opts,
>> -                          user_creatable_add_opts_foreach,
>> -                          qemu_img_object_print_help, &error_fatal)) {
>> -        ret = 2;
>> -        goto out4;
> 
> Same deal with return value.  Except here we used &error_fatal (which
> forces an exit status of 1 rather than returning), and so never even
> reach the ret=2 code.  Looks like we broke that in commit 334c43e2c3,
> where we used to pass NULL instead of &error_fatal (although that commit
> was in turn fixing another problem).

...and THIS spot is why my original attempt to prove that your code was
causing a regression was seeing an exit status of 1, where I instead
ended up proving that we already regressed.

> 
> The rest of this patch looks fine, although maybe
> user_creatable_process_cmdline() should be given an 'int status'
> parameter for specifying 1 vs. 2 (or any other non-zero value) if we
> intend to fix the status of qemu-img compare failures.  (Thankfully,
> even though qemu-img check also has a variety of documented return
> values other than 1, at least it documented 1 as internal errors and was
> already using 1 for --object failures).
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org