diff mbox series

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

Message ID 20210224135255.253837-26-kwolf@redhat.com (mailing list archive)
State New, archived
Headers show
Series qapi/qom: QAPIfy --object and object-add | expand

Commit Message

Kevin Wolf Feb. 24, 2021, 1:52 p.m. UTC 
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(-)

Comments

Eric Blake Feb. 26, 2021, 9:56 p.m. UTC | #1 
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 Feb. 26, 2021, 10:17 p.m. UTC | #2 
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).
>
diff mbox series

Patch

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_help, &error_fatal)) {
-        ret = 2;
-        goto out4;
-    }
-
     /* Initialize before goto out */
     qemu_progress_init(progress, 2.0);
 
@@ -1641,7 +1578,6 @@  out2:
     blk_unref(blk1);
 out3:
     qemu_progress_end();
-out4:
     return ret;
 }
 
@@ -2342,15 +2278,9 @@  static int img_convert(int argc, char **argv)
                 goto fail_getopt;
             }
             break;
-        case OPTION_OBJECT: {
-            QemuOpts *object_opts;
-            object_opts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                                  optarg, true);
-            if (!object_opts) {
-                goto fail_getopt;
-            }
+        case OPTION_OBJECT:
+            user_creatable_process_cmdline(optarg);
             break;
-        }
         case OPTION_IMAGE_OPTS:
             image_opts = true;
             break;
@@ -2378,12 +2308,6 @@  static int img_convert(int argc, char **argv)
         out_fmt = "raw";
     }
 
-    if (qemu_opts_foreach(&qemu_object_opts,
-                          user_creatable_add_opts_foreach,
-                          qemu_img_object_print_help, &error_fatal)) {
-        goto fail_getopt;
-    }
-
     if (s.compressed && s.copy_range) {
         error_report("Cannot enable copy offloading when -c is used");
         goto fail_getopt;
@@ -2971,14 +2895,9 @@  static int img_info(int argc, char **argv)
         case OPTION_BACKING_CHAIN:
             chain = 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;
@@ -2998,12 +2917,6 @@  static int img_info(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;
-    }
-
     list = collect_image_info_list(image_opts, filename, fmt, chain,
                                    force_share);
     if (!list) {
@@ -3213,14 +3126,9 @@  static int img_map(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;
@@ -3240,12 +3148,6 @@  static int img_map(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;
-    }
-
     blk = img_open(image_opts, filename, fmt, 0, false, false, force_share);
     if (!blk) {
         return 1;
@@ -3384,14 +3286,9 @@  static int img_snapshot(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;
@@ -3403,12 +3300,6 @@  static int img_snapshot(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;
-    }
-
     /* Open the image */
     blk = img_open(image_opts, filename, NULL, bdrv_oflags, false, quiet,
                    force_share);
@@ -3542,14 +3433,9 @@  static int img_rebase(int argc, char **argv)
         case 'q':
             quiet = 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;
@@ -3571,12 +3457,6 @@  static int img_rebase(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;
-    }
-
     qemu_progress_init(progress, 2.0);
     qemu_progress_print(0, 100);
 
@@ -3967,14 +3847,9 @@  static int img_resize(int argc, char **argv)
         case 'q':
             quiet = 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;
@@ -3996,12 +3871,6 @@  static int img_resize(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;
-    }
-
     /* Choose grow, shrink, or absolute resize mode */
     switch (size[0]) {
     case '+':
@@ -4181,12 +4050,7 @@  static int img_amend(int argc, char **argv)
             quiet = true;
             break;
         case OPTION_OBJECT:
-            opts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                           optarg, true);
-            if (!opts) {
-                ret = -1;
-                goto out_no_progress;
-            }
+            user_creatable_process_cmdline(optarg);
             break;
         case OPTION_IMAGE_OPTS:
             image_opts = true;
@@ -4201,13 +4065,6 @@  static int img_amend(int argc, char **argv)
         error_exit("Must specify options (-o)");
     }
 
-    if (qemu_opts_foreach(&qemu_object_opts,
-                          user_creatable_add_opts_foreach,
-                          qemu_img_object_print_help, &error_fatal)) {
-        ret = -1;
-        goto out_no_progress;
-    }
-
     if (quiet) {
         progress = false;
     }
@@ -4760,10 +4617,7 @@  static int img_bitmap(int argc, char **argv)
             merge = true;
             break;
         case OPTION_OBJECT:
-            opts = qemu_opts_parse_noisily(&qemu_object_opts, optarg, true);
-            if (!opts) {
-                goto out;
-            }
+            user_creatable_process_cmdline(optarg);
             break;
         case OPTION_IMAGE_OPTS:
             image_opts = true;
@@ -4771,12 +4625,6 @@  static int img_bitmap(int argc, char **argv)
         }
     }
 
-    if (qemu_opts_foreach(&qemu_object_opts,
-                          user_creatable_add_opts_foreach,
-                          qemu_img_object_print_help, &error_fatal)) {
-        goto out;
-    }
-
     if (QSIMPLEQ_EMPTY(&actions)) {
         error_report("Need at least one of --add, --remove, --clear, "
                      "--enable, --disable, or --merge");
@@ -5034,10 +4882,7 @@  static int img_dd(int argc, char **argv)
             force_share = true;
             break;
         case OPTION_OBJECT:
-            if (!qemu_opts_parse_noisily(&qemu_object_opts, optarg, true)) {
-                ret = -1;
-                goto out;
-            }
+            user_creatable_process_cmdline(optarg);
             break;
         case OPTION_IMAGE_OPTS:
             image_opts = true;
@@ -5084,13 +4929,6 @@  static int img_dd(int argc, char **argv)
         goto out;
     }
 
-    if (qemu_opts_foreach(&qemu_object_opts,
-                          user_creatable_add_opts_foreach,
-                          qemu_img_object_print_help, &error_fatal)) {
-        ret = -1;
-        goto out;
-    }
-
     blk1 = img_open(image_opts, in.filename, fmt, 0, false, false,
                     force_share);
 
@@ -5311,11 +5149,7 @@  static int img_measure(int argc, char **argv)
             force_share = true;
             break;
         case OPTION_OBJECT:
-            object_opts = qemu_opts_parse_noisily(&qemu_object_opts,
-                                                  optarg, true);
-            if (!object_opts) {
-                goto out;
-            }
+            user_creatable_process_cmdline(optarg);
             break;
         case OPTION_IMAGE_OPTS:
             image_opts = true;
@@ -5345,12 +5179,6 @@  static int img_measure(int argc, char **argv)
         }
     }
 
-    if (qemu_opts_foreach(&qemu_object_opts,
-                          user_creatable_add_opts_foreach,
-                          qemu_img_object_print_help, &error_fatal)) {
-        goto out;
-    }
-
     if (argc - optind > 1) {
         error_report("At most one filename argument is allowed.");
         goto out;
@@ -5490,7 +5318,6 @@  int main(int argc, char **argv)
         error_exit("Not enough arguments");
     }
 
-    qemu_add_opts(&qemu_object_opts);
     qemu_add_opts(&qemu_source_opts);
     qemu_add_opts(&qemu_trace_opts);