Re: [PATCH v3 26/30] qemu-img: Use user_creatable_process_cmdline() for --object
Am 15.03.2021 um 15:15 hat Markus Armbruster geschrieben:
> Kevin Wolf writes:
>
> > Am 13.03.2021 um 13:30 hat Markus Armbruster geschrieben:
> >> Paolo Bonzini writes:
> >>
> >> > On 13/03/21 08:40, Markus Armbruster wrote:
> >> >>> +if (!user_creatable_add_from_str(optarg, _err))
> >> >>> {
> >> >>> +if (local_err) {
> >> >>> +error_report_err(local_err);
> >> >>> +exit(2);
> >> >>> +} else {
> >> >>> +/* Help was printed */
> >> >>> +exit(EXIT_SUCCESS);
> >> >>> +}
> >> >>> +}
> >> >>> +break;
> >> >>> }
> >> >>> -} break;
> >> >>> case OPTION_IMAGE_OPTS:
> >> >>> image_opts = true;
> >> >>> break;
> >> >> Why is this one different? The others all call
> >> >> user_creatable_process_cmdline().
> >> >>
> >> >>
> >> >
> >> > It's to exit with status code 2 instead of 1.
> >>
> >> I see. Worth a comment?
> >
> > There is a comment at the start of the function (which is just a few
> > lines above) that explains the exit codes:
> >
> > * Compares two images. Exit codes:
> > *
> > * 0 - Images are identical or the requested help was printed
> > * 1 - Images differ
> > * >1 - Error occurred
>
> I had in mind a comment that helps me over the "why is this not using
> user_creatable_process_cmdline()" hump. Like so:
>
> case OPTION_OBJECT:
> {
> /*
> * Can't use user_creatable_process_cmdline(), because
> * we need to exit(2) on error.
> */
> ... open-coded variation of
> user_creatable_process_cmdline() ...
> }
>
> Entirely up to you.
I see. This patch is already part of a pull request, but I wouldn't mind
a follow-up patch to add this comment if you want to send one.
Kevin
Re: [PATCH v3 26/30] qemu-img: Use user_creatable_process_cmdline() for --object
Kevin Wolf writes:
> Am 13.03.2021 um 13:30 hat Markus Armbruster geschrieben:
>> Paolo Bonzini writes:
>>
>> > On 13/03/21 08:40, Markus Armbruster wrote:
>> >>> +if (!user_creatable_add_from_str(optarg, _err)) {
>> >>> +if (local_err) {
>> >>> +error_report_err(local_err);
>> >>> +exit(2);
>> >>> +} else {
>> >>> +/* Help was printed */
>> >>> +exit(EXIT_SUCCESS);
>> >>> +}
>> >>> +}
>> >>> +break;
>> >>> }
>> >>> -} break;
>> >>> case OPTION_IMAGE_OPTS:
>> >>> image_opts = true;
>> >>> break;
>> >> Why is this one different? The others all call
>> >> user_creatable_process_cmdline().
>> >>
>> >>
>> >
>> > It's to exit with status code 2 instead of 1.
>>
>> I see. Worth a comment?
>
> There is a comment at the start of the function (which is just a few
> lines above) that explains the exit codes:
>
> * Compares two images. Exit codes:
> *
> * 0 - Images are identical or the requested help was printed
> * 1 - Images differ
> * >1 - Error occurred
I had in mind a comment that helps me over the "why is this not using
user_creatable_process_cmdline()" hump. Like so:
case OPTION_OBJECT:
{
/*
* Can't use user_creatable_process_cmdline(), because
* we need to exit(2) on error.
*/
... open-coded variation of
user_creatable_process_cmdline() ...
}
Entirely up to you.
Re: [PATCH v3 26/30] qemu-img: Use user_creatable_process_cmdline() for --object
Am 13.03.2021 um 13:30 hat Markus Armbruster geschrieben:
> Paolo Bonzini writes:
>
> > On 13/03/21 08:40, Markus Armbruster wrote:
> >>> +if (!user_creatable_add_from_str(optarg, _err)) {
> >>> +if (local_err) {
> >>> +error_report_err(local_err);
> >>> +exit(2);
> >>> +} else {
> >>> +/* Help was printed */
> >>> +exit(EXIT_SUCCESS);
> >>> +}
> >>> +}
> >>> +break;
> >>> }
> >>> -} break;
> >>> case OPTION_IMAGE_OPTS:
> >>> image_opts = true;
> >>> break;
> >> Why is this one different? The others all call
> >> user_creatable_process_cmdline().
> >>
> >>
> >
> > It's to exit with status code 2 instead of 1.
>
> I see. Worth a comment?
There is a comment at the start of the function (which is just a few
lines above) that explains the exit codes:
* Compares two images. Exit codes:
*
* 0 - Images are identical or the requested help was printed
* 1 - Images differ
* >1 - Error occurred
Kevin
Re: [PATCH v3 26/30] qemu-img: Use user_creatable_process_cmdline() for --object
Paolo Bonzini writes:
> On 13/03/21 08:40, Markus Armbruster wrote:
>>> +if (!user_creatable_add_from_str(optarg, _err)) {
>>> +if (local_err) {
>>> +error_report_err(local_err);
>>> +exit(2);
>>> +} else {
>>> +/* Help was printed */
>>> +exit(EXIT_SUCCESS);
>>> +}
>>> +}
>>> +break;
>>> }
>>> -} break;
>>> case OPTION_IMAGE_OPTS:
>>> image_opts = true;
>>> break;
>> Why is this one different? The others all call
>> user_creatable_process_cmdline().
>>
>>
>
> It's to exit with status code 2 instead of 1.
I see. Worth a comment?
Re: [PATCH v3 26/30] qemu-img: Use user_creatable_process_cmdline() for --object
On 13/03/21 08:40, Markus Armbruster wrote:
+if (!user_creatable_add_from_str(optarg, _err)) {
+if (local_err) {
+error_report_err(local_err);
+exit(2);
+} else {
+/* Help was printed */
+exit(EXIT_SUCCESS);
+}
+}
+break;
}
-} break;
case OPTION_IMAGE_OPTS:
image_opts = true;
break;
Why is this one different? The others all call
user_creatable_process_cmdline().
It's to exit with status code 2 instead of 1.
Paolo
Re: [PATCH v3 26/30] qemu-img: Use user_creatable_process_cmdline() for --object
Kevin Wolf writes:
> 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
> Acked-by: Peter Krempa
> ---
> qemu-img.c | 251 ++---
> 1 file changed, 45 insertions(+), 206 deletions(-)
>
> diff --git a/qemu-img.c b/qemu-img.c
> index e2952fe955..babb5573ab 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(_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(_object_opts,
> - user_creatable_add_opts_foreach,
> - qemu_img_object_print_help, _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(_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(_object_opts,
> - user_creatable_add_opts_foreach,
> - qemu_img_object_print_help, _fatal)) {
> -return 1;
> -}
> -
> ret = bdrv_parse_cache_mode(cache, , );
> 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(_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(_object_opts,
> - user_creatable_add_opts_foreach,
> - qemu_img_object_print_help, _fatal)) {
> -return 1;
> -}
> -
> flags = BDRV_O_RDWR | BDRV_O_UNMAP;
> ret = bdrv_parse_cache_mode(cache, , );
> if (ret < 0) {
> @@ -1353,7 +1303,7 @@ static int check_empty_sectors(BlockBackend *blk,
> int64_t offset,
> /*
> * Compares two images. Exit codes:
> *
> - * 0 - Images are identical
> + * 0 - Images are identical or the requested help was printed
> * 1 - Images differ
> * >1 - Error occurred
> */
> @@ -1423,15 +1373,21 @@ static int img_compare(int argc, char **argv)
> case 'U':
> force_share = true;
> break;
> -case OPTION_OBJECT: {
> -QemuOpts *opts;
> -opts = qemu_opts_parse_noisily(_object_opts,
> - optarg, true);
> -if (!opts) {
> -ret = 2;
> -
Re: [PATCH v3 26/30] qemu-img: Use user_creatable_process_cmdline() for --object
On 3/8/21 10:54 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
> Acked-by: Peter Krempa
> ---
> qemu-img.c | 251 ++---
> 1 file changed, 45 insertions(+), 206 deletions(-)
>
> @@ -1353,7 +1303,7 @@ static int check_empty_sectors(BlockBackend *blk,
> int64_t offset,
> /*
> * Compares two images. Exit codes:
> *
> - * 0 - Images are identical
> + * 0 - Images are identical or the requested help was printed
Nice, but does the user-facing documentation need updating to match?
> * 1 - Images differ
> * >1 - Error occurred
> */
> @@ -1423,15 +1373,21 @@ static int img_compare(int argc, char **argv)
> case 'U':
> force_share = true;
> break;
> -case OPTION_OBJECT: {
> -QemuOpts *opts;
> -opts = qemu_opts_parse_noisily(_object_opts,
> - optarg, true);
> -if (!opts) {
> -ret = 2;
> -goto out4;
> +case OPTION_OBJECT:
> +{
> +Error *local_err = NULL;
> +
> +if (!user_creatable_add_from_str(optarg, _err)) {
> +if (local_err) {
> +error_report_err(local_err);
> +exit(2);
> +} else {
> +/* Help was printed */
> +exit(EXIT_SUCCESS);
> +}
The commit message needs to be updated to call out that this bug fix was
intentional, preferably mentioning the commit where we broke it
(334c43e2c3).
The code is fine, though, so with an improved commit message (and maybe
some matching doc tweaks),
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[PATCH v3 26/30] 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
Acked-by: Peter Krempa
---
qemu-img.c | 251 ++---
1 file changed, 45 insertions(+), 206 deletions(-)
diff --git a/qemu-img.c b/qemu-img.c
index e2952fe955..babb5573ab 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(_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(_object_opts,
- user_creatable_add_opts_foreach,
- qemu_img_object_print_help, _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(_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(_object_opts,
- user_creatable_add_opts_foreach,
- qemu_img_object_print_help, _fatal)) {
-return 1;
-}
-
ret = bdrv_parse_cache_mode(cache, , );
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(_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(_object_opts,
- user_creatable_add_opts_foreach,
- qemu_img_object_print_help, _fatal)) {
-return 1;
-}
-
flags = BDRV_O_RDWR | BDRV_O_UNMAP;
ret = bdrv_parse_cache_mode(cache, , );
if (ret < 0) {
@@ -1353,7 +1303,7 @@ static int check_empty_sectors(BlockBackend *blk, int64_t
offset,
/*
* Compares two images. Exit codes:
*
- * 0 - Images are identical
+ * 0 - Images are identical or the requested help was printed
* 1 - Images differ
* >1 - Error occurred
*/
@@ -1423,15 +1373,21 @@ static int img_compare(int argc, char **argv)
case 'U':
force_share = true;
break;
-case OPTION_OBJECT: {
-QemuOpts *opts;
-opts = qemu_opts_parse_noisily(_object_opts,
- optarg, true);
-if (!opts) {
-ret = 2;
-goto out4;
+case OPTION_OBJECT:
+{
+Error *local_err = NULL;
+
+if (!user_creatable_add_from_str(optarg, _err)) {
+if (local_err) {
+error_report_err(local_err);
+exit(2);
+}
