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 $ 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, loc = {kind = (unknown: 0x557f08f0), num = 21845, ptr = 0x55555578f020 <qemu_object_opts>, prev = 0x0}, head = { tqh_first = 0x0, tqh_circ = {tql_next = 0x0, tql_prev = 0x0}}, next = { tqe_next = 0x55555583b500, tqe_circ = {tql_next = 0x55555583b500, tql_prev = 0x55555583b528}}} (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