Re: [PATCH v2 25/31] qemu-img: Use user_creatable_process_cmdline() for --object
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
>> ---
>> 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=0x5578f020 , params=0x7fffd8a8
> "foo,id=x",
> permit_abbrev=true) at ../util/qemu-option.c:948
> 0x555805f9 in img_compare (argc=5, argv=0x7fffd480)
> at ../qemu-img.c:1428
> 1428 opts = qemu_opts_parse_noisily(&qemu_object_opts,
> Value returned is $1 = (QemuOpts *) 0x5583b4b0
> (gdb) p *opts
> $3 = {id = 0x557a0d58 "`\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
Re: [PATCH v2 25/31] qemu-img: Use user_creatable_process_cmdline() for --object
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
> ---
> 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
$ 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=0x5578f020 , params=0x7fffd8a8
"foo,id=x",
permit_abbrev=true) at ../util/qemu-option.c:948
0x555805f9 in img_compare (argc=5, argv=0x7fffd480)
at ../qemu-img.c:1428
1428opts = qemu_opts_parse_noisily(&qemu_object_opts,
Value returned is $1 = (QemuOpts *) 0x5583b4b0
(gdb) p *opts
$3 = {id = 0x557a0d58 "`\264\203UUU", list = 0x51,
loc = {kind = (unknown: 0x557f08f0), num = 21845,
ptr = 0x5578f020 , prev = 0x0}, head = {
tqh_first = 0x0, tqh_circ = {tql_next = 0x0, tql_prev = 0x0}}, next = {
tqe_next = 0x5583b500, tqe_circ = {tql_next = 0x5583b500,
tql_prev = 0x5583b528}}}
(gdb)
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.
> 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).
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
[PATCH v2 25/31] qemu-img: Use user_creatable_process_cmdline() for --object
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
---
qemu-img.c | 239 -
1 file changed, 33 insertions(+), 206 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index e2952fe955..ebf8661e2a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -226,23 +226,6 @@ static void QEMU_NORETURN help(void)
exit(EXIT_SUCCESS);
}
-static QemuOptsList qemu_object_opts = {
-.name = "object",
-.implied_opt_name = "qom-type",
-.head = QTAILQ_HEAD_INITIALIZER(qemu_object_opts.head),
-.desc = {
-{ }
-},
-};
-
-static bool qemu_img_object_print_help(const char *type, QemuOpts *opts)
-{
-if (user_creatable_print_help(type, opts)) {
-exit(0);
-}
-return true;
-}
-
/*
* Is @optarg safe for accumulate_options()?
* It is when multiple of them can be joined together separated by ','.
@@ -566,14 +549,9 @@ static int img_create(int argc, char **argv)
case 'u':
flags |= BDRV_O_NO_BACKING;
break;
-case OPTION_OBJECT: {
-QemuOpts *opts;
-opts = qemu_opts_parse_noisily(&qemu_object_opts,
- optarg, true);
-if (!opts) {
-goto fail;
-}
-} break;
+case OPTION_OBJECT:
+user_creatable_process_cmdline(optarg);
+break;
}
}
@@ -589,12 +567,6 @@ static int img_create(int argc, char **argv)
}
optind++;
-if (qemu_opts_foreach(&qemu_object_opts,
- user_creatable_add_opts_foreach,
- qemu_img_object_print_help, &error_fatal)) {
-goto fail;
-}
-
/* Get image size, if specified */
if (optind < argc) {
int64_t sval;
@@ -804,14 +776,9 @@ static int img_check(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) {
-return 1;
-}
-} break;
+case OPTION_OBJECT:
+user_creatable_process_cmdline(optarg);
+break;
case OPTION_IMAGE_OPTS:
image_opts = true;
break;
@@ -831,12 +798,6 @@ static int img_check(int argc, char **argv)
return 1;
}
-if (qemu_opts_foreach(&qemu_object_opts,
- user_creatable_add_opts_foreach,
- qemu_img_object_print_help, &error_fatal)) {
-return 1;
-}
-
ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
if (ret < 0) {
error_report("Invalid source cache option: %s", cache);
@@ -1034,14 +995,9 @@ static int img_commit(int argc, char **argv)
return 1;
}
break;
-case OPTION_OBJECT: {
-QemuOpts *opts;
-opts = qemu_opts_parse_noisily(&qemu_object_opts,
- optarg, true);
-if (!opts) {
-return 1;
-}
-} break;
+case OPTION_OBJECT:
+user_creatable_process_cmdline(optarg);
+break;
case OPTION_IMAGE_OPTS:
image_opts = true;
break;
@@ -1058,12 +1014,6 @@ static int img_commit(int argc, char **argv)
}
filename = argv[optind++];
-if (qemu_opts_foreach(&qemu_object_opts,
- user_creatable_add_opts_foreach,
- qemu_img_object_print_help, &error_fatal)) {
-return 1;
-}
-
flags = BDRV_O_RDWR | BDRV_O_UNMAP;
ret = bdrv_parse_cache_mode(cache, &flags, &writethrough);
if (ret < 0) {
@@ -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;
-}
-} break;
+case OPTION_OBJECT:
+user_creatable_process_cmdline(optarg);
+break;
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_he
