[PATCH for-5.1 0/8] qemu-option: Fix corner cases and clean up

[PATCH for-5.1 0/8] qemu-option: Fix corner cases and clean up

[Markus Armbruster]

Markus Armbruster (8):
  tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()
  qemu-options: Factor out get_opt_name_value() helper
  qemu-option: Fix sloppy recognition of "id=..." after ",,"
  qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily()
  qemu-option: Fix has_help_option()'s sloppy parsing
  test-qemu-opts: Simplify test_has_help_option() after bug fix
  qemu-img: Factor out accumulate_options() helper
  qemu-option: Move is_valid_option_list() to qemu-img.c and rewrite

 include/qemu/option.h  |   1 -
 qemu-img.c             |  85 ++++++++++-------
 tests/test-qemu-opts.c |  40 +++++++-
 util/qemu-option.c     | 210 ++++++++++++++++++++---------------------
 4 files changed, 191 insertions(+), 145 deletions(-)

-- 
2.21.1

[PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()

[Markus Armbruster]

The two turn out to be inconsistent for "a,b,,help".  Test case
marked /* BUG */.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-qemu-opts.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index ef96e84aed..0efe93b45e 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -728,6 +728,43 @@ static void test_opts_parse_size(void)
     qemu_opts_reset(&opts_list_02);
 }
 
+static void test_has_help_option(void)
+{
+    static const struct {
+        const char *params;
+        /* expected value of has_help_option() */
+        bool expect_has_help_option;
+        /* expected value of qemu_opt_has_help_opt() with implied=false */
+        bool expect_opt_has_help_opt;
+        /* expected value of qemu_opt_has_help_opt() with implied=true */
+        bool expect_opt_has_help_opt_implied;
+    } test[] = {
+        { "help", true, true, false },
+        { "helpme", false, false, false },
+        { "a,help", true, true, true },
+        { "a=0,help,b", true, true, true },
+        { "help,b=1", true, true, false },
+        { "a,b,,help", false /* BUG */, true, true },
+    };
+    int i;
+    QemuOpts *opts;
+
+    for (i = 0; i < ARRAY_SIZE(test); i++) {
+        g_assert_cmpint(has_help_option(test[i].params),
+                        ==, test[i].expect_has_help_option);
+        opts = qemu_opts_parse(&opts_list_03, test[i].params, false,
+                               &error_abort);
+        g_assert_cmpint(qemu_opt_has_help_opt(opts),
+                        ==, test[i].expect_opt_has_help_opt);
+        qemu_opts_del(opts);
+        opts = qemu_opts_parse(&opts_list_03, test[i].params, true,
+                               &error_abort);
+        g_assert_cmpint(qemu_opt_has_help_opt(opts),
+                        ==, test[i].expect_opt_has_help_opt_implied);
+        qemu_opts_del(opts);
+    }
+}
+
 static void append_verify_list_01(QemuOptDesc *desc, bool with_overlapping)
 {
     int i = 0;
@@ -990,6 +1027,7 @@ int main(int argc, char *argv[])
     g_test_add_func("/qemu-opts/opts_parse/bool", test_opts_parse_bool);
     g_test_add_func("/qemu-opts/opts_parse/number", test_opts_parse_number);
     g_test_add_func("/qemu-opts/opts_parse/size", test_opts_parse_size);
+    g_test_add_func("/qemu-opts/has_help_option", test_has_help_option);
     g_test_add_func("/qemu-opts/append_to_null", test_opts_append_to_null);
     g_test_add_func("/qemu-opts/append", test_opts_append);
     g_test_add_func("/qemu-opts/to_qdict/basic", test_opts_to_qdict_basic);
-- 
2.21.1

Re: [PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()

[Eric Blake]

On 4/9/20 10:30 AM, Markus Armbruster wrote:
> The two turn out to be inconsistent for "a,b,,help".  Test case
> marked /* BUG */.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/test-qemu-opts.c | 38 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 38 insertions(+)
> 

> +static void test_has_help_option(void)
> +{
> +    static const struct {
> +        const char *params;
> +        /* expected value of has_help_option() */
> +        bool expect_has_help_option;
> +        /* expected value of qemu_opt_has_help_opt() with implied=false */
> +        bool expect_opt_has_help_opt;
> +        /* expected value of qemu_opt_has_help_opt() with implied=true */
> +        bool expect_opt_has_help_opt_implied;
> +    } test[] = {
> +        { "help", true, true, false },
> +        { "helpme", false, false, false },
> +        { "a,help", true, true, true },
> +        { "a=0,help,b", true, true, true },
> +        { "help,b=1", true, true, false },
> +        { "a,b,,help", false /* BUG */, true, true },

So which way are you calling the bug?  Without looking at the code but 
going off my intuition, I parse this as option 'a' and option 'b,help'. 
The latter is not a normal option name because it contains a ',', but is 
a valid option value.

I agree that we have a bug, but I'm not yet sure in which direction the 
bug lies (should has_help_option be fixed to report true, in which case 
the substring ",help" has precedence over ',,' escaping; or should 
qemu_opt_has_help_opt be fixed to report false, due to treating 'b,help' 
after ',,' escape removal as an invalid option name).  So the placement 
of the /* BUG */ comment matters - where you placed it, I'm presuming 
that later in the series you change has_help_option to return true, even 
though that goes against my intuitive parse.

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

Re: [PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 4/9/20 10:30 AM, Markus Armbruster wrote:
>> The two turn out to be inconsistent for "a,b,,help".  Test case
>> marked /* BUG */.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   tests/test-qemu-opts.c | 38 ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 38 insertions(+)
>>
>
>> +static void test_has_help_option(void)
>> +{
>> +    static const struct {
>> +        const char *params;
>> +        /* expected value of has_help_option() */
>> +        bool expect_has_help_option;
>> +        /* expected value of qemu_opt_has_help_opt() with implied=false */
>> +        bool expect_opt_has_help_opt;
>> +        /* expected value of qemu_opt_has_help_opt() with implied=true */
>> +        bool expect_opt_has_help_opt_implied;
>> +    } test[] = {
>> +        { "help", true, true, false },
>> +        { "helpme", false, false, false },
>> +        { "a,help", true, true, true },
>> +        { "a=0,help,b", true, true, true },
>> +        { "help,b=1", true, true, false },
>> +        { "a,b,,help", false /* BUG */, true, true },
>
> So which way are you calling the bug?  Without looking at the code but
> going off my intuition, I parse this as option 'a' and option
> 'b,help'. The latter is not a normal option name because it contains a
> ',', but is a valid option value.
>
> I agree that we have a bug, but I'm not yet sure in which direction
> the bug lies (should has_help_option be fixed to report true, in which
> case the substring ",help" has precedence over ',,' escaping; or
> should qemu_opt_has_help_opt be fixed to report false, due to treating
> 'b,help' after ',,' escape removal as an invalid option name).  So the
> placement of the /* BUG */ comment matters - where you placed it, I'm
> presuming that later in the series you change has_help_option to
> return true, even though that goes against my intuitive parse.

In addition to the canonical QemuOpts parser opts_do_parse(), we have
several more, and of course they all differ from the canonical one for
corner cases.

I treat the canonical one as correct, and fix the others by eliminating
the extra parsers.

The others are:

* has_help_option()

  Fixed in PATCH 5 by reusing the guts of opts_do_parse().

* is_valid_option_list()

  Fixed in PATCH 8 by not parsing.

* "id" extraction in opts_parse()

  Lazy hack.  Fixed in PATCH 3 by reusing the guts of opts_do_parse().

Back to your question: the value of has_help_option() differs from the
value of qemu_opt_has_help_opt().  The latter uses the canonical parser,
the former is one of the other parsers.  I therefore judge the latter
right and the former wrong.

Clear now?

Re: [PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()

[Kevin Wolf]

Am 14.04.2020 um 11:10 hat Markus Armbruster geschrieben:
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 4/9/20 10:30 AM, Markus Armbruster wrote:
> >> The two turn out to be inconsistent for "a,b,,help".  Test case
> >> marked /* BUG */.
> >>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>   tests/test-qemu-opts.c | 38 ++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 38 insertions(+)
> >>
> >
> >> +static void test_has_help_option(void)
> >> +{
> >> +    static const struct {
> >> +        const char *params;
> >> +        /* expected value of has_help_option() */
> >> +        bool expect_has_help_option;
> >> +        /* expected value of qemu_opt_has_help_opt() with implied=false */
> >> +        bool expect_opt_has_help_opt;
> >> +        /* expected value of qemu_opt_has_help_opt() with implied=true */
> >> +        bool expect_opt_has_help_opt_implied;
> >> +    } test[] = {
> >> +        { "help", true, true, false },

While we're talking about unintuitive, I feel the result for
implied=true is confusing, too. Never noticed it before, but are we
really sure that it is the best possible behaviour that '-chardev help'
and '-chardev id=foo,help' print two entirely different help texts?

I'm not requesting to change anything about this in this series, but
just making the point that maybe sometimes the existing behaviour is
questionable.

> >> +        { "helpme", false, false, false },
> >> +        { "a,help", true, true, true },
> >> +        { "a=0,help,b", true, true, true },
> >> +        { "help,b=1", true, true, false },
> >> +        { "a,b,,help", false /* BUG */, true, true },
> >
> > So which way are you calling the bug?  Without looking at the code but
> > going off my intuition, I parse this as option 'a' and option
> > 'b,help'. The latter is not a normal option name because it contains a
> > ',', but is a valid option value.
> >
> > I agree that we have a bug, but I'm not yet sure in which direction
> > the bug lies (should has_help_option be fixed to report true, in which
> > case the substring ",help" has precedence over ',,' escaping; or
> > should qemu_opt_has_help_opt be fixed to report false, due to treating
> > 'b,help' after ',,' escape removal as an invalid option name).  So the
> > placement of the /* BUG */ comment matters - where you placed it, I'm
> > presuming that later in the series you change has_help_option to
> > return true, even though that goes against my intuitive parse.
> 
> In addition to the canonical QemuOpts parser opts_do_parse(), we have
> several more, and of course they all differ from the canonical one for
> corner cases.
> 
> I treat the canonical one as correct, and fix the others by eliminating
> the extra parsers.
> 
> The others are:
> 
> * has_help_option()
> 
>   Fixed in PATCH 5 by reusing the guts of opts_do_parse().
> 
> * is_valid_option_list()
> 
>   Fixed in PATCH 8 by not parsing.
> 
> * "id" extraction in opts_parse()
> 
>   Lazy hack.  Fixed in PATCH 3 by reusing the guts of opts_do_parse().
> 
> Back to your question: the value of has_help_option() differs from the
> value of qemu_opt_has_help_opt().  The latter uses the canonical parser,
> the former is one of the other parsers.  I therefore judge the latter
> right and the former wrong.

Shouldn't we also consider what users would reasonably expect?

Getting it parsed as an empty option name (I assume with a default value
of "on"?) certainly looks like something that would surprise most users
and, as you can see, even some QEMU developers.

Kevin

Re: [PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 14.04.2020 um 11:10 hat Markus Armbruster geschrieben:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>> > On 4/9/20 10:30 AM, Markus Armbruster wrote:
>> >> The two turn out to be inconsistent for "a,b,,help".  Test case
>> >> marked /* BUG */.
>> >>
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >>   tests/test-qemu-opts.c | 38 ++++++++++++++++++++++++++++++++++++++
>> >>   1 file changed, 38 insertions(+)
>> >>
>> >
>> >> +static void test_has_help_option(void)
>> >> +{
>> >> +    static const struct {
>> >> +        const char *params;
>> >> +        /* expected value of has_help_option() */
>> >> +        bool expect_has_help_option;
>> >> +        /* expected value of qemu_opt_has_help_opt() with implied=false */
>> >> +        bool expect_opt_has_help_opt;
>> >> +        /* expected value of qemu_opt_has_help_opt() with implied=true */
>> >> +        bool expect_opt_has_help_opt_implied;
>> >> +    } test[] = {
>> >> +        { "help", true, true, false },
>
> While we're talking about unintuitive, I feel the result for
> implied=true is confusing, too. Never noticed it before, but are we
> really sure that it is the best possible behaviour that '-chardev help'
> and '-chardev id=foo,help' print two entirely different help texts?
>
> I'm not requesting to change anything about this in this series, but
> just making the point that maybe sometimes the existing behaviour is
> questionable.

The QEMU CLI may sometimes be confusing?!?  I'm shocked, sir, I'm
shocked!

>> >> +        { "helpme", false, false, false },
>> >> +        { "a,help", true, true, true },
>> >> +        { "a=0,help,b", true, true, true },
>> >> +        { "help,b=1", true, true, false },
>> >> +        { "a,b,,help", false /* BUG */, true, true },
>> >
>> > So which way are you calling the bug?  Without looking at the code but
>> > going off my intuition, I parse this as option 'a' and option
>> > 'b,help'. The latter is not a normal option name because it contains a
>> > ',', but is a valid option value.
>> >
>> > I agree that we have a bug, but I'm not yet sure in which direction
>> > the bug lies (should has_help_option be fixed to report true, in which
>> > case the substring ",help" has precedence over ',,' escaping; or
>> > should qemu_opt_has_help_opt be fixed to report false, due to treating
>> > 'b,help' after ',,' escape removal as an invalid option name).  So the
>> > placement of the /* BUG */ comment matters - where you placed it, I'm
>> > presuming that later in the series you change has_help_option to
>> > return true, even though that goes against my intuitive parse.
>> 
>> In addition to the canonical QemuOpts parser opts_do_parse(), we have
>> several more, and of course they all differ from the canonical one for
>> corner cases.
>> 
>> I treat the canonical one as correct, and fix the others by eliminating
>> the extra parsers.
>> 
>> The others are:
>> 
>> * has_help_option()
>> 
>>   Fixed in PATCH 5 by reusing the guts of opts_do_parse().
>> 
>> * is_valid_option_list()
>> 
>>   Fixed in PATCH 8 by not parsing.
>> 
>> * "id" extraction in opts_parse()
>> 
>>   Lazy hack.  Fixed in PATCH 3 by reusing the guts of opts_do_parse().
>> 
>> Back to your question: the value of has_help_option() differs from the
>> value of qemu_opt_has_help_opt().  The latter uses the canonical parser,
>> the former is one of the other parsers.  I therefore judge the latter
>> right and the former wrong.
>
> Shouldn't we also consider what users would reasonably expect?

Of course we should consider reasonable user expectations.

Grumpy aside: when I do, I commonly run into objections that users
reasonably expect things not to change.

> Getting it parsed as an empty option name (I assume with a default value
> of "on"?) certainly looks like something that would surprise most users
> and, as you can see, even some QEMU developers.

My preferred way to address QemuOpts parsing madness is replacing it
wholesale by keyval.c, but that's some time off, I'm afraid.

This series merely aims for more method to the same old madness.

Re: [PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()

[Kevin Wolf]

Am 14.04.2020 um 15:36 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> > Am 14.04.2020 um 11:10 hat Markus Armbruster geschrieben:
> >> Eric Blake <eblake@redhat.com> writes:
> >> > On 4/9/20 10:30 AM, Markus Armbruster wrote:
> >> >> +        { "helpme", false, false, false },
> >> >> +        { "a,help", true, true, true },
> >> >> +        { "a=0,help,b", true, true, true },
> >> >> +        { "help,b=1", true, true, false },
> >> >> +        { "a,b,,help", false /* BUG */, true, true },
> >> >
> >> > So which way are you calling the bug?  Without looking at the code but
> >> > going off my intuition, I parse this as option 'a' and option
> >> > 'b,help'. The latter is not a normal option name because it contains a
> >> > ',', but is a valid option value.
> >> >
> >> > I agree that we have a bug, but I'm not yet sure in which direction
> >> > the bug lies (should has_help_option be fixed to report true, in which
> >> > case the substring ",help" has precedence over ',,' escaping; or
> >> > should qemu_opt_has_help_opt be fixed to report false, due to treating
> >> > 'b,help' after ',,' escape removal as an invalid option name).  So the
> >> > placement of the /* BUG */ comment matters - where you placed it, I'm
> >> > presuming that later in the series you change has_help_option to
> >> > return true, even though that goes against my intuitive parse.
> >> 
> >> In addition to the canonical QemuOpts parser opts_do_parse(), we have
> >> several more, and of course they all differ from the canonical one for
> >> corner cases.
> >> 
> >> I treat the canonical one as correct, and fix the others by eliminating
> >> the extra parsers.
> >> 
> >> The others are:
> >> 
> >> * has_help_option()
> >> 
> >>   Fixed in PATCH 5 by reusing the guts of opts_do_parse().
> >> 
> >> * is_valid_option_list()
> >> 
> >>   Fixed in PATCH 8 by not parsing.
> >> 
> >> * "id" extraction in opts_parse()
> >> 
> >>   Lazy hack.  Fixed in PATCH 3 by reusing the guts of opts_do_parse().
> >> 
> >> Back to your question: the value of has_help_option() differs from the
> >> value of qemu_opt_has_help_opt().  The latter uses the canonical parser,
> >> the former is one of the other parsers.  I therefore judge the latter
> >> right and the former wrong.
> >
> > Shouldn't we also consider what users would reasonably expect?
> 
> Of course we should consider reasonable user expectations.
> 
> Grumpy aside: when I do, I commonly run into objections that users
> reasonably expect things not to change.

Fair point. It's not always easy to tell whether something should be
considered a bug in the external interface (and consequently be fixed)
or just an idiosyncrasy that people may have get used to (and therefore
requires deprecation before improving it).

In this specific case, I'm not aware of empty option names actually
doing anything useful anywhere, so I think it might be clearer in this
case that it's indeed a bug.

> > Getting it parsed as an empty option name (I assume with a default value
> > of "on"?) certainly looks like something that would surprise most users
> > and, as you can see, even some QEMU developers.
> 
> My preferred way to address QemuOpts parsing madness is replacing it
> wholesale by keyval.c, but that's some time off, I'm afraid.
> 
> This series merely aims for more method to the same old madness.

I understand. Though I think replacing with keyval will be potentially
less problematic if QemuOpts already behaved more similar.

If I were writing the code, I think I would use existing bugs and
inconsistencies as an excuse to make QemuOpts behave more like what
keyval can easily handle by declaring whatever is closest to keyval as
the correct interpretation.

Kevin

Re: [PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 14.04.2020 um 15:36 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> > Am 14.04.2020 um 11:10 hat Markus Armbruster geschrieben:
>> >> Eric Blake <eblake@redhat.com> writes:
>> >> > On 4/9/20 10:30 AM, Markus Armbruster wrote:
>> >> >> +        { "helpme", false, false, false },
>> >> >> +        { "a,help", true, true, true },
>> >> >> +        { "a=0,help,b", true, true, true },
>> >> >> +        { "help,b=1", true, true, false },
>> >> >> +        { "a,b,,help", false /* BUG */, true, true },
>> >> >
>> >> > So which way are you calling the bug?  Without looking at the code but
>> >> > going off my intuition, I parse this as option 'a' and option
>> >> > 'b,help'. The latter is not a normal option name because it contains a
>> >> > ',', but is a valid option value.
>> >> >
>> >> > I agree that we have a bug, but I'm not yet sure in which direction
>> >> > the bug lies (should has_help_option be fixed to report true, in which
>> >> > case the substring ",help" has precedence over ',,' escaping; or
>> >> > should qemu_opt_has_help_opt be fixed to report false, due to treating
>> >> > 'b,help' after ',,' escape removal as an invalid option name).  So the
>> >> > placement of the /* BUG */ comment matters - where you placed it, I'm
>> >> > presuming that later in the series you change has_help_option to
>> >> > return true, even though that goes against my intuitive parse.
>> >> 
>> >> In addition to the canonical QemuOpts parser opts_do_parse(), we have
>> >> several more, and of course they all differ from the canonical one for
>> >> corner cases.
>> >> 
>> >> I treat the canonical one as correct, and fix the others by eliminating
>> >> the extra parsers.
>> >> 
>> >> The others are:
>> >> 
>> >> * has_help_option()
>> >> 
>> >>   Fixed in PATCH 5 by reusing the guts of opts_do_parse().
>> >> 
>> >> * is_valid_option_list()
>> >> 
>> >>   Fixed in PATCH 8 by not parsing.
>> >> 
>> >> * "id" extraction in opts_parse()
>> >> 
>> >>   Lazy hack.  Fixed in PATCH 3 by reusing the guts of opts_do_parse().
>> >> 
>> >> Back to your question: the value of has_help_option() differs from the
>> >> value of qemu_opt_has_help_opt().  The latter uses the canonical parser,
>> >> the former is one of the other parsers.  I therefore judge the latter
>> >> right and the former wrong.
>> >
>> > Shouldn't we also consider what users would reasonably expect?
>> 
>> Of course we should consider reasonable user expectations.
>> 
>> Grumpy aside: when I do, I commonly run into objections that users
>> reasonably expect things not to change.
>
> Fair point. It's not always easy to tell whether something should be
> considered a bug in the external interface (and consequently be fixed)
> or just an idiosyncrasy that people may have get used to (and therefore
> requires deprecation before improving it).
>
> In this specific case, I'm not aware of empty option names actually
> doing anything useful anywhere, so I think it might be clearer in this
> case that it's indeed a bug.

You're right in that backward compatibility is not a convincing argument
for stuff that has no known productive uses, and is bonkers to boot.

>> > Getting it parsed as an empty option name (I assume with a default value
>> > of "on"?) certainly looks like something that would surprise most users
>> > and, as you can see, even some QEMU developers.
>> 
>> My preferred way to address QemuOpts parsing madness is replacing it
>> wholesale by keyval.c, but that's some time off, I'm afraid.
>> 
>> This series merely aims for more method to the same old madness.
>
> I understand. Though I think replacing with keyval will be potentially
> less problematic if QemuOpts already behaved more similar.
>
> If I were writing the code, I think I would use existing bugs and
> inconsistencies as an excuse to make QemuOpts behave more like what
> keyval can easily handle by declaring whatever is closest to keyval as
> the correct interpretation.

Fair enough.

However,

(1) is_valid_option_list()'s and opts_do_parse()'s parse of "a,b,,help"
    are equidistant from keyval_parse()'s:

    opts_do_parse() splits it into four parts:

        "a"     (desugared to a=on)
        "b"     (desugared to b=on)
        ""      (desugared to =on)
        "help"  (desugared to help=on)

    has_help_option() splits it into two:

        "a"
        "b,help"

    keyval_parse() fails:

        Expected '=' after parameter 'a'

    If I it implemented boolean sugar, then it would fail at the third
    comma, just like ",help" fails now:

        Invalid parameter ''

    Fails because ",help" does not start with a valid name.

    Thus, the answer to the question which of the two functions covered
    by the test are wrong would be "both".

(2) This series tries hard not to write QemuOpts parsing code.  It
    throws away QemuOpts parsing code.

Re: [PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()

[Kevin Wolf]

Am 14.04.2020 um 22:13 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Am 14.04.2020 um 15:36 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> > Am 14.04.2020 um 11:10 hat Markus Armbruster geschrieben:
> >> >> Eric Blake <eblake@redhat.com> writes:
> >> >> > On 4/9/20 10:30 AM, Markus Armbruster wrote:
> >> >> >> +        { "helpme", false, false, false },
> >> >> >> +        { "a,help", true, true, true },
> >> >> >> +        { "a=0,help,b", true, true, true },
> >> >> >> +        { "help,b=1", true, true, false },
> >> >> >> +        { "a,b,,help", false /* BUG */, true, true },
> >> >> >
> >> >> > So which way are you calling the bug?  Without looking at the code but
> >> >> > going off my intuition, I parse this as option 'a' and option
> >> >> > 'b,help'. The latter is not a normal option name because it contains a
> >> >> > ',', but is a valid option value.
> >> >> >
> >> >> > I agree that we have a bug, but I'm not yet sure in which direction
> >> >> > the bug lies (should has_help_option be fixed to report true, in which
> >> >> > case the substring ",help" has precedence over ',,' escaping; or
> >> >> > should qemu_opt_has_help_opt be fixed to report false, due to treating
> >> >> > 'b,help' after ',,' escape removal as an invalid option name).  So the
> >> >> > placement of the /* BUG */ comment matters - where you placed it, I'm
> >> >> > presuming that later in the series you change has_help_option to
> >> >> > return true, even though that goes against my intuitive parse.
> >> >> 
> >> >> In addition to the canonical QemuOpts parser opts_do_parse(), we have
> >> >> several more, and of course they all differ from the canonical one for
> >> >> corner cases.
> >> >> 
> >> >> I treat the canonical one as correct, and fix the others by eliminating
> >> >> the extra parsers.
> >> >> 
> >> >> The others are:
> >> >> 
> >> >> * has_help_option()
> >> >> 
> >> >>   Fixed in PATCH 5 by reusing the guts of opts_do_parse().
> >> >> 
> >> >> * is_valid_option_list()
> >> >> 
> >> >>   Fixed in PATCH 8 by not parsing.
> >> >> 
> >> >> * "id" extraction in opts_parse()
> >> >> 
> >> >>   Lazy hack.  Fixed in PATCH 3 by reusing the guts of opts_do_parse().
> >> >> 
> >> >> Back to your question: the value of has_help_option() differs from the
> >> >> value of qemu_opt_has_help_opt().  The latter uses the canonical parser,
> >> >> the former is one of the other parsers.  I therefore judge the latter
> >> >> right and the former wrong.
> >> >
> >> > Shouldn't we also consider what users would reasonably expect?
> >> 
> >> Of course we should consider reasonable user expectations.
> >> 
> >> Grumpy aside: when I do, I commonly run into objections that users
> >> reasonably expect things not to change.
> >
> > Fair point. It's not always easy to tell whether something should be
> > considered a bug in the external interface (and consequently be fixed)
> > or just an idiosyncrasy that people may have get used to (and therefore
> > requires deprecation before improving it).
> >
> > In this specific case, I'm not aware of empty option names actually
> > doing anything useful anywhere, so I think it might be clearer in this
> > case that it's indeed a bug.
> 
> You're right in that backward compatibility is not a convincing argument
> for stuff that has no known productive uses, and is bonkers to boot.
> 
> >> > Getting it parsed as an empty option name (I assume with a default value
> >> > of "on"?) certainly looks like something that would surprise most users
> >> > and, as you can see, even some QEMU developers.
> >> 
> >> My preferred way to address QemuOpts parsing madness is replacing it
> >> wholesale by keyval.c, but that's some time off, I'm afraid.
> >> 
> >> This series merely aims for more method to the same old madness.
> >
> > I understand. Though I think replacing with keyval will be potentially
> > less problematic if QemuOpts already behaved more similar.
> >
> > If I were writing the code, I think I would use existing bugs and
> > inconsistencies as an excuse to make QemuOpts behave more like what
> > keyval can easily handle by declaring whatever is closest to keyval as
> > the correct interpretation.
> 
> Fair enough.
> 
> However,
> 
> (1) is_valid_option_list()'s and opts_do_parse()'s parse of "a,b,,help"
>     are equidistant from keyval_parse()'s:
> 
>     opts_do_parse() splits it into four parts:
> 
>         "a"     (desugared to a=on)
>         "b"     (desugared to b=on)
>         ""      (desugared to =on)
>         "help"  (desugared to help=on)
> 
>     has_help_option() splits it into two:
> 
>         "a"
>         "b,help"
> 
>     keyval_parse() fails:
> 
>         Expected '=' after parameter 'a'
> 
>     If I it implemented boolean sugar, then it would fail at the third
>     comma, just like ",help" fails now:
> 
>         Invalid parameter ''
> 
>     Fails because ",help" does not start with a valid name.
> 
>     Thus, the answer to the question which of the two functions covered
>     by the test are wrong would be "both".

opts_do_parse() can return an error. So maybe what we should do is
rejecting empty option names there?

> (2) This series tries hard not to write QemuOpts parsing code.  It
>     throws away QemuOpts parsing code.

Arguably, an additional error is writing QemuOpts parsing code, but
maybe little enough that it's tolerable?

Kevin

Re: [PATCH for-5.1 1/8] tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 14.04.2020 um 22:13 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > Am 14.04.2020 um 15:36 hat Markus Armbruster geschrieben:
>> >> Kevin Wolf <kwolf@redhat.com> writes:
>> >> > Am 14.04.2020 um 11:10 hat Markus Armbruster geschrieben:
>> >> >> Eric Blake <eblake@redhat.com> writes:
>> >> >> > On 4/9/20 10:30 AM, Markus Armbruster wrote:
>> >> >> >> +        { "helpme", false, false, false },
>> >> >> >> +        { "a,help", true, true, true },
>> >> >> >> +        { "a=0,help,b", true, true, true },
>> >> >> >> +        { "help,b=1", true, true, false },
>> >> >> >> +        { "a,b,,help", false /* BUG */, true, true },
>> >> >> >
>> >> >> > So which way are you calling the bug?  Without looking at the code but
>> >> >> > going off my intuition, I parse this as option 'a' and option
>> >> >> > 'b,help'. The latter is not a normal option name because it contains a
>> >> >> > ',', but is a valid option value.
>> >> >> >
>> >> >> > I agree that we have a bug, but I'm not yet sure in which direction
>> >> >> > the bug lies (should has_help_option be fixed to report true, in which
>> >> >> > case the substring ",help" has precedence over ',,' escaping; or
>> >> >> > should qemu_opt_has_help_opt be fixed to report false, due to treating
>> >> >> > 'b,help' after ',,' escape removal as an invalid option name).  So the
>> >> >> > placement of the /* BUG */ comment matters - where you placed it, I'm
>> >> >> > presuming that later in the series you change has_help_option to
>> >> >> > return true, even though that goes against my intuitive parse.
>> >> >> 
>> >> >> In addition to the canonical QemuOpts parser opts_do_parse(), we have
>> >> >> several more, and of course they all differ from the canonical one for
>> >> >> corner cases.
>> >> >> 
>> >> >> I treat the canonical one as correct, and fix the others by eliminating
>> >> >> the extra parsers.
>> >> >> 
>> >> >> The others are:
>> >> >> 
>> >> >> * has_help_option()
>> >> >> 
>> >> >>   Fixed in PATCH 5 by reusing the guts of opts_do_parse().
>> >> >> 
>> >> >> * is_valid_option_list()
>> >> >> 
>> >> >>   Fixed in PATCH 8 by not parsing.
>> >> >> 
>> >> >> * "id" extraction in opts_parse()
>> >> >> 
>> >> >>   Lazy hack.  Fixed in PATCH 3 by reusing the guts of opts_do_parse().
>> >> >> 
>> >> >> Back to your question: the value of has_help_option() differs from the
>> >> >> value of qemu_opt_has_help_opt().  The latter uses the canonical parser,
>> >> >> the former is one of the other parsers.  I therefore judge the latter
>> >> >> right and the former wrong.
>> >> >
>> >> > Shouldn't we also consider what users would reasonably expect?
>> >> 
>> >> Of course we should consider reasonable user expectations.
>> >> 
>> >> Grumpy aside: when I do, I commonly run into objections that users
>> >> reasonably expect things not to change.
>> >
>> > Fair point. It's not always easy to tell whether something should be
>> > considered a bug in the external interface (and consequently be fixed)
>> > or just an idiosyncrasy that people may have get used to (and therefore
>> > requires deprecation before improving it).
>> >
>> > In this specific case, I'm not aware of empty option names actually
>> > doing anything useful anywhere, so I think it might be clearer in this
>> > case that it's indeed a bug.
>> 
>> You're right in that backward compatibility is not a convincing argument
>> for stuff that has no known productive uses, and is bonkers to boot.
>> 
>> >> > Getting it parsed as an empty option name (I assume with a default value
>> >> > of "on"?) certainly looks like something that would surprise most users
>> >> > and, as you can see, even some QEMU developers.
>> >> 
>> >> My preferred way to address QemuOpts parsing madness is replacing it
>> >> wholesale by keyval.c, but that's some time off, I'm afraid.
>> >> 
>> >> This series merely aims for more method to the same old madness.
>> >
>> > I understand. Though I think replacing with keyval will be potentially
>> > less problematic if QemuOpts already behaved more similar.
>> >
>> > If I were writing the code, I think I would use existing bugs and
>> > inconsistencies as an excuse to make QemuOpts behave more like what
>> > keyval can easily handle by declaring whatever is closest to keyval as
>> > the correct interpretation.
>> 
>> Fair enough.
>> 
>> However,
>> 
>> (1) is_valid_option_list()'s and opts_do_parse()'s parse of "a,b,,help"
>>     are equidistant from keyval_parse()'s:
>> 
>>     opts_do_parse() splits it into four parts:
>> 
>>         "a"     (desugared to a=on)
>>         "b"     (desugared to b=on)
>>         ""      (desugared to =on)
>>         "help"  (desugared to help=on)
>> 
>>     has_help_option() splits it into two:
>> 
>>         "a"
>>         "b,help"
>> 
>>     keyval_parse() fails:
>> 
>>         Expected '=' after parameter 'a'
>> 
>>     If I it implemented boolean sugar, then it would fail at the third
>>     comma, just like ",help" fails now:
>> 
>>         Invalid parameter ''
>> 
>>     Fails because ",help" does not start with a valid name.
>> 
>>     Thus, the answer to the question which of the two functions covered
>>     by the test are wrong would be "both".
>
> opts_do_parse() can return an error. So maybe what we should do is
> rejecting empty option names there?
>
>> (2) This series tries hard not to write QemuOpts parsing code.  It
>>     throws away QemuOpts parsing code.
>
> Arguably, an additional error is writing QemuOpts parsing code, but
> maybe little enough that it's tolerable?

The patch would be simple enough, but I dread writing the commit
message.

I permitted myself to get sidetracked into this series, I'm desperate to
get it out of the way and return to the work I actually want to do.

[PATCH for-5.1 2/8] qemu-options: Factor out get_opt_name_value() helper

[Markus Armbruster]

The next commits will put it to use.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/qemu-option.c | 102 +++++++++++++++++++++++++--------------------
 1 file changed, 56 insertions(+), 46 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index 97172b5eaa..f08f4bc458 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -805,61 +805,71 @@ void qemu_opts_print(QemuOpts *opts, const char *separator)
     }
 }
 
+static const char *get_opt_name_value(const char *params,
+                                      const char *firstname,
+                                      char **name, char **value)
+{
+    const char *p, *pe, *pc;
+
+    pe = strchr(params, '=');
+    pc = strchr(params, ',');
+
+    if (!pe || (pc && pc < pe)) {
+        /* found "foo,more" */
+        if (firstname) {
+            /* implicitly named first option */
+            *name = g_strdup(firstname);
+            p = get_opt_value(params, value);
+        } else {
+            /* option without value, must be a flag */
+            p = get_opt_name(params, name, ',');
+            if (strncmp(*name, "no", 2) == 0) {
+                memmove(*name, *name + 2, strlen(*name + 2) + 1);
+                *value = g_strdup("off");
+            } else {
+                *value = g_strdup("on");
+            }
+        }
+    } else {
+        /* found "foo=bar,more" */
+        p = get_opt_name(params, name, '=');
+        assert(*p == '=');
+        p++;
+        p = get_opt_value(p, value);
+    }
+
+    assert(!*p || *p == ',');
+    if (*p == ',') {
+        p++;
+    }
+    return p;
+}
+
 static void opts_do_parse(QemuOpts *opts, const char *params,
                           const char *firstname, bool prepend,
                           bool *invalidp, Error **errp)
 {
-    char *option = NULL;
-    char *value = NULL;
-    const char *p,*pe,*pc;
     Error *local_err = NULL;
+    char *option, *value;
+    const char *p;
 
-    for (p = params; *p != '\0'; p++) {
-        pe = strchr(p, '=');
-        pc = strchr(p, ',');
-        if (!pe || (pc && pc < pe)) {
-            /* found "foo,more" */
-            if (p == params && firstname) {
-                /* implicitly named first option */
-                option = g_strdup(firstname);
-                p = get_opt_value(p, &value);
-            } else {
-                /* option without value, probably a flag */
-                p = get_opt_name(p, &option, ',');
-                if (strncmp(option, "no", 2) == 0) {
-                    memmove(option, option+2, strlen(option+2)+1);
-                    value = g_strdup("off");
-                } else {
-                    value = g_strdup("on");
-                }
-            }
-        } else {
-            /* found "foo=bar,more" */
-            p = get_opt_name(p, &option, '=');
-            assert(*p == '=');
-            p++;
-            p = get_opt_value(p, &value);
-        }
-        if (strcmp(option, "id") != 0) {
-            /* store and parse */
-            opt_set(opts, option, value, prepend, invalidp, &local_err);
-            value = NULL;
-            if (local_err) {
-                error_propagate(errp, local_err);
-                goto cleanup;
-            }
-        }
-        if (*p != ',') {
-            break;
+    for (p = params; *p;) {
+        p = get_opt_name_value(p, firstname, &option, &value);
+        firstname = NULL;
+
+        if (!strcmp(option, "id")) {
+            g_free(option);
+            g_free(value);
+            continue;
         }
+
+        opt_set(opts, option, value, prepend, invalidp, &local_err);
         g_free(option);
-        g_free(value);
-        option = value = NULL;
+        if (local_err) {
+            error_propagate(errp, local_err);
+            return;
+        }
     }
-
- cleanup:
-    g_free(option);
-    g_free(value);
 }
 
 /**
-- 
2.21.1

Re: [PATCH for-5.1 2/8] qemu-options: Factor out get_opt_name_value() helper

[Eric Blake]

On 4/9/20 10:30 AM, Markus Armbruster wrote:
> The next commits will put it to use.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   util/qemu-option.c | 102 +++++++++++++++++++++++++--------------------
>   1 file changed, 56 insertions(+), 46 deletions(-)
> 

> +static const char *get_opt_name_value(const char *params,
> +                                      const char *firstname,
> +                                      char **name, char **value)
> +{
> +    const char *p, *pe, *pc;
> +
> +    pe = strchr(params, '=');
> +    pc = strchr(params, ',');
> +
> +    if (!pe || (pc && pc < pe)) {
> +        /* found "foo,more" */
> +        if (firstname) {
> +            /* implicitly named first option */
> +            *name = g_strdup(firstname);
> +            p = get_opt_value(params, value);

Is this correct even when params is "foo,,more"?  But...

>   static void opts_do_parse(QemuOpts *opts, const char *params,
>                             const char *firstname, bool prepend,
>                             bool *invalidp, Error **errp)
>   {
> -    char *option = NULL;
> -    char *value = NULL;
> -    const char *p,*pe,*pc;
>       Error *local_err = NULL;
> +    char *option, *value;
> +    const char *p;
>   
> -    for (p = params; *p != '\0'; p++) {
> -        pe = strchr(p, '=');
> -        pc = strchr(p, ',');
> -        if (!pe || (pc && pc < pe)) {
> -            /* found "foo,more" */
> -            if (p == params && firstname) {
> -                /* implicitly named first option */
> -                option = g_strdup(firstname);
> -                p = get_opt_value(p, &value);

...in this patch, it is just code motion, so if it is a bug, it's 
pre-existing and worth a separate fix.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH for-5.1 2/8] qemu-options: Factor out get_opt_name_value() helper

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 4/9/20 10:30 AM, Markus Armbruster wrote:
>> The next commits will put it to use.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   util/qemu-option.c | 102 +++++++++++++++++++++++++--------------------
>>   1 file changed, 56 insertions(+), 46 deletions(-)
>>
>
>> +static const char *get_opt_name_value(const char *params,
>> +                                      const char *firstname,
>> +                                      char **name, char **value)
>> +{
>> +    const char *p, *pe, *pc;
>> +
>> +    pe = strchr(params, '=');
>> +    pc = strchr(params, ',');
>> +
>> +    if (!pe || (pc && pc < pe)) {
>> +        /* found "foo,more" */
>> +        if (firstname) {
>> +            /* implicitly named first option */
>> +            *name = g_strdup(firstname);
>> +            p = get_opt_value(params, value);
>
> Is this correct even when params is "foo,,more"?  But...
>
>>   static void opts_do_parse(QemuOpts *opts, const char *params,
>>                             const char *firstname, bool prepend,
>>                             bool *invalidp, Error **errp)
>>   {
>> -    char *option = NULL;
>> -    char *value = NULL;
>> -    const char *p,*pe,*pc;
>>       Error *local_err = NULL;
>> +    char *option, *value;
>> +    const char *p;
>>   -    for (p = params; *p != '\0'; p++) {
>> -        pe = strchr(p, '=');
>> -        pc = strchr(p, ',');
>> -        if (!pe || (pc && pc < pe)) {
>> -            /* found "foo,more" */
>> -            if (p == params && firstname) {
>> -                /* implicitly named first option */
>> -                option = g_strdup(firstname);
>> -                p = get_opt_value(p, &value);
>
> ...in this patch, it is just code motion, so if it is a bug, it's
> pre-existing and worth a separate fix.

If @p points to "foo,,more", possibly followed by additional characters,
then we have pc && pc < pe, and enter this conditional.  This means that

    foo,,more=42

gets parsed as two name=value, either as

    name        value
    -----------------------
    @firstname  "foo,more"
    ""          "42"

if @firstname is non-null, or else as

    name        value
    -----------------
    "foo,,more" "on"
    ""          "42"

Has always been that way; see commit e27c88fe9e "QemuOpts: framework for
storing and parsing options.", v0.12.0.

You might expect

    name        value
    -----------------
    "foo,,more" "42"

or

    name        value
    -----------------
    "foo,more"  "42"

instead.  Close, but no cigar.

A language without syntax errors is bound to surprise.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

Re: [PATCH for-5.1 2/8] qemu-options: Factor out get_opt_name_value() helper

[Kevin Wolf]

Am 14.04.2020 um 11:42 hat Markus Armbruster geschrieben:
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 4/9/20 10:30 AM, Markus Armbruster wrote:
> >> The next commits will put it to use.
> >>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >> ---
> >>   util/qemu-option.c | 102 +++++++++++++++++++++++++--------------------
> >>   1 file changed, 56 insertions(+), 46 deletions(-)
> >>
> >
> >> +static const char *get_opt_name_value(const char *params,
> >> +                                      const char *firstname,
> >> +                                      char **name, char **value)
> >> +{
> >> +    const char *p, *pe, *pc;
> >> +
> >> +    pe = strchr(params, '=');
> >> +    pc = strchr(params, ',');
> >> +
> >> +    if (!pe || (pc && pc < pe)) {
> >> +        /* found "foo,more" */
> >> +        if (firstname) {
> >> +            /* implicitly named first option */
> >> +            *name = g_strdup(firstname);
> >> +            p = get_opt_value(params, value);
> >
> > Is this correct even when params is "foo,,more"?  But...
> >
> >>   static void opts_do_parse(QemuOpts *opts, const char *params,
> >>                             const char *firstname, bool prepend,
> >>                             bool *invalidp, Error **errp)
> >>   {
> >> -    char *option = NULL;
> >> -    char *value = NULL;
> >> -    const char *p,*pe,*pc;
> >>       Error *local_err = NULL;
> >> +    char *option, *value;
> >> +    const char *p;
> >>   -    for (p = params; *p != '\0'; p++) {
> >> -        pe = strchr(p, '=');
> >> -        pc = strchr(p, ',');
> >> -        if (!pe || (pc && pc < pe)) {
> >> -            /* found "foo,more" */
> >> -            if (p == params && firstname) {
> >> -                /* implicitly named first option */
> >> -                option = g_strdup(firstname);
> >> -                p = get_opt_value(p, &value);
> >
> > ...in this patch, it is just code motion, so if it is a bug, it's
> > pre-existing and worth a separate fix.
> 
> If @p points to "foo,,more", possibly followed by additional characters,
> then we have pc && pc < pe, and enter this conditional.  This means that
> 
>     foo,,more=42
> 
> gets parsed as two name=value, either as
> 
>     name        value
>     -----------------------
>     @firstname  "foo,more"
>     ""          "42"
> 
> if @firstname is non-null

Hm, how that? get_opt_value() doesn't stop at '=', so shouldn't you get
a single option @firstname with a value of "foo,more=42"?

     name        value
     -----------------------
     @firstname  "foo,more=42"

> or else as
> 
>     name        value
>     -----------------
>     "foo,,more" "on"
>     ""          "42"

Here get_opt_name() stops at the first comma. You get a positive flag
"foo" and the remaing option string starts with a comma, so the
condition will still be true for the next loop iteration. Now you get a
positive flag with an empty name "". Finally, the rest is parsed as an
option, so we get:

     name        value
     -----------------------
     "foo"       "on"
     ""          "on"
     "more"      "42"

Actually, at this point I thought I could just check, and gdb confirms
my reading for both cases.

This is still crazy, but perhaps less so than the interpretations you
suggested.

Kevin

Re: [PATCH for-5.1 2/8] qemu-options: Factor out get_opt_name_value() helper

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 14.04.2020 um 11:42 hat Markus Armbruster geschrieben:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>> > On 4/9/20 10:30 AM, Markus Armbruster wrote:
>> >> The next commits will put it to use.
>> >>
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >> ---
>> >>   util/qemu-option.c | 102 +++++++++++++++++++++++++--------------------
>> >>   1 file changed, 56 insertions(+), 46 deletions(-)
>> >>
>> >
>> >> +static const char *get_opt_name_value(const char *params,
>> >> +                                      const char *firstname,
>> >> +                                      char **name, char **value)
>> >> +{
>> >> +    const char *p, *pe, *pc;
>> >> +
>> >> +    pe = strchr(params, '=');
>> >> +    pc = strchr(params, ',');
>> >> +
>> >> +    if (!pe || (pc && pc < pe)) {
>> >> +        /* found "foo,more" */
>> >> +        if (firstname) {
>> >> +            /* implicitly named first option */
>> >> +            *name = g_strdup(firstname);
>> >> +            p = get_opt_value(params, value);
>> >
>> > Is this correct even when params is "foo,,more"?  But...
>> >
>> >>   static void opts_do_parse(QemuOpts *opts, const char *params,
>> >>                             const char *firstname, bool prepend,
>> >>                             bool *invalidp, Error **errp)
>> >>   {
>> >> -    char *option = NULL;
>> >> -    char *value = NULL;
>> >> -    const char *p,*pe,*pc;
>> >>       Error *local_err = NULL;
>> >> +    char *option, *value;
>> >> +    const char *p;
>> >>   -    for (p = params; *p != '\0'; p++) {
>> >> -        pe = strchr(p, '=');
>> >> -        pc = strchr(p, ',');
>> >> -        if (!pe || (pc && pc < pe)) {
>> >> -            /* found "foo,more" */
>> >> -            if (p == params && firstname) {
>> >> -                /* implicitly named first option */
>> >> -                option = g_strdup(firstname);
>> >> -                p = get_opt_value(p, &value);
>> >
>> > ...in this patch, it is just code motion, so if it is a bug, it's
>> > pre-existing and worth a separate fix.
>> 
>> If @p points to "foo,,more", possibly followed by additional characters,
>> then we have pc && pc < pe, and enter this conditional.  This means that
>> 
>>     foo,,more=42
>> 
>> gets parsed as two name=value, either as
>> 
>>     name        value
>>     -----------------------
>>     @firstname  "foo,more"
>>     ""          "42"
>> 
>> if @firstname is non-null
>
> Hm, how that? get_opt_value() doesn't stop at '=', so shouldn't you get
> a single option @firstname with a value of "foo,more=42"?
>
>      name        value
>      -----------------------
>      @firstname  "foo,more=42"
>
>> or else as
>> 
>>     name        value
>>     -----------------
>>     "foo,,more" "on"
>>     ""          "42"
>
> Here get_opt_name() stops at the first comma. You get a positive flag
> "foo" and the remaing option string starts with a comma, so the
> condition will still be true for the next loop iteration. Now you get a
> positive flag with an empty name "". Finally, the rest is parsed as an
> option, so we get:
>
>      name        value
>      -----------------------
>      "foo"       "on"
>      ""          "on"
>      "more"      "42"
>
> Actually, at this point I thought I could just check, and gdb confirms
> my reading for both cases.

You're right.  I should know better than trying to predict what the
QemuOpts parser does.

> This is still crazy, but perhaps less so than the interpretations you
> suggested.

Permitting anti-social characters in the NAME part of NAME=VALUE is
crazy.  To findout which kind of crazy exactly, run the code and
observe.  Do not blindly trust the maintainer's explanations, because
he's quite possibly just as confused as everybody else.

Re: [PATCH for-5.1 2/8] qemu-options: Factor out get_opt_name_value() helper

[Kevin Wolf]

Am 09.04.2020 um 17:30 hat Markus Armbruster geschrieben:
> The next commits will put it to use.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[PATCH for-5.1 3/8] qemu-option: Fix sloppy recognition of "id=..." after ", , "

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-qemu-opts.c |  4 ++--
 util/qemu-option.c     | 27 +++++++++++++++++++--------
 2 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 0efe93b45e..27c24bb1a2 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -500,10 +500,10 @@ static void test_opts_parse(void)
     g_assert(!opts);
     /* TODO Cover .merge_lists = true */
 
-    /* Buggy ID recognition */
+    /* Buggy ID recognition (fixed) */
     opts = qemu_opts_parse(&opts_list_03, "x=,,id=bar", false, &error_abort);
     g_assert_cmpuint(opts_count(opts), ==, 1);
-    g_assert_cmpstr(qemu_opts_id(opts), ==, "bar"); /* BUG */
+    g_assert(!qemu_opts_id(opts));
     g_assert_cmpstr(qemu_opt_get(opts, "x"), ==, ",id=bar");
 
     /* Anti-social ID */
diff --git a/util/qemu-option.c b/util/qemu-option.c
index f08f4bc458..d2956082bd 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -872,6 +872,24 @@ static void opts_do_parse(QemuOpts *opts, const char *params,
     }
 }
 
+static char *opts_parse_id(const char *params)
+{
+    const char *p;
+    char *name, *value;
+
+    for (p = params; *p;) {
+        p = get_opt_name_value(p, NULL, &name, &value);
+        if (!strcmp(name, "id")) {
+            g_free(name);
+            return value;
+        }
+        g_free(name);
+        g_free(value);
+    }
+
+    return NULL;
+}
+
 /**
  * Store options parsed from @params into @opts.
  * If @firstname is non-null, the first key=value in @params may omit
@@ -889,20 +907,13 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
                             bool *invalidp, Error **errp)
 {
     const char *firstname;
-    char *id = NULL;
-    const char *p;
+    char *id = opts_parse_id(params);
     QemuOpts *opts;
     Error *local_err = NULL;
 
     assert(!permit_abbrev || list->implied_opt_name);
     firstname = permit_abbrev ? list->implied_opt_name : NULL;
 
-    if (strncmp(params, "id=", 3) == 0) {
-        get_opt_value(params + 3, &id);
-    } else if ((p = strstr(params, ",id=")) != NULL) {
-        get_opt_value(p + 4, &id);
-    }
-
     /*
      * This code doesn't work for defaults && !list->merge_lists: when
      * params has no id=, and list has an element with !opts->id, it
-- 
2.21.1

Re: [PATCH for-5.1 3/8] qemu-option: Fix sloppy recognition of "id=..." after ", , "

[Eric Blake]

On 4/9/20 10:30 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/test-qemu-opts.c |  4 ++--
>   util/qemu-option.c     | 27 +++++++++++++++++++--------
>   2 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
> index 0efe93b45e..27c24bb1a2 100644
> --- a/tests/test-qemu-opts.c
> +++ b/tests/test-qemu-opts.c
> @@ -500,10 +500,10 @@ static void test_opts_parse(void)
>       g_assert(!opts);
>       /* TODO Cover .merge_lists = true */
>   
> -    /* Buggy ID recognition */
> +    /* Buggy ID recognition (fixed) */
>       opts = qemu_opts_parse(&opts_list_03, "x=,,id=bar", false, &error_abort);
>       g_assert_cmpuint(opts_count(opts), ==, 1);
> -    g_assert_cmpstr(qemu_opts_id(opts), ==, "bar"); /* BUG */
> +    g_assert(!qemu_opts_id(opts));
>       g_assert_cmpstr(qemu_opt_get(opts, "x"), ==, ",id=bar");

Yay - that matches my intuition better about ,, handling.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH for-5.1 3/8] qemu-option: Fix sloppy recognition of "id=..." after ",,"

[Kevin Wolf]

Am 09.04.2020 um 17:30 hat Markus Armbruster geschrieben:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[PATCH for-5.1 4/8] qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily()

[Markus Armbruster]

When opts_parse() sets @invalidp to true, qemu_opts_parse_noisily()
uses has_help_option() to decide whether to print help.  This parses
the input string a second time, in a slightly different way.

Easy to avoid: replace @invalidp by @help_wanted.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 util/qemu-option.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/util/qemu-option.c b/util/qemu-option.c
index d2956082bd..6403e521fc 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -539,7 +539,7 @@ int qemu_opt_unset(QemuOpts *opts, const char *name)
 }
 
 static void opt_set(QemuOpts *opts, const char *name, char *value,
-                    bool prepend, bool *invalidp, Error **errp)
+                    bool prepend, bool *help_wanted, Error **errp)
 {
     QemuOpt *opt;
     const QemuOptDesc *desc;
@@ -549,8 +549,8 @@ static void opt_set(QemuOpts *opts, const char *name, char *value,
     if (!desc && !opts_accepts_any(opts)) {
         g_free(value);
         error_setg(errp, QERR_INVALID_PARAMETER, name);
-        if (invalidp) {
-            *invalidp = true;
+        if (help_wanted && is_help_option(name)) {
+            *help_wanted = true;
         }
         return;
     }
@@ -847,7 +847,7 @@ static const char *get_opt_name_value(const char *params,
 
 static void opts_do_parse(QemuOpts *opts, const char *params,
                           const char *firstname, bool prepend,
-                          bool *invalidp, Error **errp)
+                          bool *help_wanted, Error **errp)
 {
     Error *local_err = NULL;
     char *option, *value;
@@ -863,7 +863,7 @@ static void opts_do_parse(QemuOpts *opts, const char *params,
             continue;
         }
 
-        opt_set(opts, option, value, prepend, invalidp, &local_err);
+        opt_set(opts, option, value, prepend, help_wanted, &local_err);
         g_free(option);
         if (local_err) {
             error_propagate(errp, local_err);
@@ -904,7 +904,7 @@ void qemu_opts_do_parse(QemuOpts *opts, const char *params,
 
 static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
                             bool permit_abbrev, bool defaults,
-                            bool *invalidp, Error **errp)
+                            bool *help_wanted, Error **errp)
 {
     const char *firstname;
     char *id = opts_parse_id(params);
@@ -929,7 +929,7 @@ static QemuOpts *opts_parse(QemuOptsList *list, const char *params,
         return NULL;
     }
 
-    opts_do_parse(opts, params, firstname, defaults, invalidp, &local_err);
+    opts_do_parse(opts, params, firstname, defaults, help_wanted, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         qemu_opts_del(opts);
@@ -965,11 +965,11 @@ QemuOpts *qemu_opts_parse_noisily(QemuOptsList *list, const char *params,
 {
     Error *err = NULL;
     QemuOpts *opts;
-    bool invalidp = false;
+    bool help_wanted = false;
 
-    opts = opts_parse(list, params, permit_abbrev, false, &invalidp, &err);
+    opts = opts_parse(list, params, permit_abbrev, false, &help_wanted, &err);
     if (err) {
-        if (invalidp && has_help_option(params)) {
+        if (help_wanted) {
             qemu_opts_print_help(list, true);
             error_free(err);
         } else {
-- 
2.21.1

Re: [PATCH for-5.1 4/8] qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily()

[Eric Blake]

On 4/9/20 10:30 AM, Markus Armbruster wrote:
> When opts_parse() sets @invalidp to true, qemu_opts_parse_noisily()
> uses has_help_option() to decide whether to print help.  This parses
> the input string a second time, in a slightly different way.
> 
> Easy to avoid: replace @invalidp by @help_wanted.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   util/qemu-option.c | 20 ++++++++++----------
>   1 file changed, 10 insertions(+), 10 deletions(-)
> 

> -    opts = opts_parse(list, params, permit_abbrev, false, &invalidp, &err);
> +    opts = opts_parse(list, params, permit_abbrev, false, &help_wanted, &err);
>       if (err) {
> -        if (invalidp && has_help_option(params)) {
> +        if (help_wanted) {

Yep, that is cleaner.  Should there be testsuite coverage changing as a 
result of this?

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH for-5.1 4/8] qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily()

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 4/9/20 10:30 AM, Markus Armbruster wrote:
>> When opts_parse() sets @invalidp to true, qemu_opts_parse_noisily()
>> uses has_help_option() to decide whether to print help.  This parses
>> the input string a second time, in a slightly different way.
>>
>> Easy to avoid: replace @invalidp by @help_wanted.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   util/qemu-option.c | 20 ++++++++++----------
>>   1 file changed, 10 insertions(+), 10 deletions(-)
>>
>
>> -    opts = opts_parse(list, params, permit_abbrev, false, &invalidp, &err);
>> +    opts = opts_parse(list, params, permit_abbrev, false, &help_wanted, &err);
>>       if (err) {
>> -        if (invalidp && has_help_option(params)) {
>> +        if (help_wanted) {
>
> Yep, that is cleaner.  Should there be testsuite coverage changing as
> a result of this?

Hmm.  I guess the actual question is whether this patch changes
behavior.

@invalidp gets set to true when opt_set() runs into an unknown @name.

Aside: can't happend when opts_accepts_any(); such options can't rely on
qemu_opts_parse_noisily() providing help.

One reason for unknown @name is the user asking for help.  We want to
provide it then.  To find out, we use has_help_option(), which parses
certain corner cases differently.  PATCH 1 demonstrates it can return
false incorrectly for certain corner cases.  This patch fixes
qemu_opts_parse_noisily() to provide help as it should even for these
corner cases.

What about this:

* I put PATCH 5 "qemu-option: Fix has_help_option()'s sloppy parsing"
  before this one, so that this one doesn't change behavior.

* I adjust this one's commit message accordingly: scratch ", in a
  slightly different way".

Do we care enough to further document the bug fix in PATCH 5's commit
message?  Even find an actual CLI option that reproduces the bug?  I
doubt it.  This is all about silly corner cases involving ','.

Perhaps has_help_option() can also return true incorrectly.  I doubt we
care.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks!

[PATCH for-5.1 5/8] qemu-option: Fix has_help_option()'s sloppy parsing

[Markus Armbruster]

has_help_option() uses its own parser.  It's inconsistent with
qemu_opts_parse(), as demonstrated by test-qemu-opts case
/qemu-opts/has_help_option.  Fix by reusing the common parser.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-qemu-opts.c |  2 +-
 util/qemu-option.c     | 39 +++++++++++++++++++--------------------
 2 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 27c24bb1a2..58a4ea2408 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -744,7 +744,7 @@ static void test_has_help_option(void)
         { "a,help", true, true, true },
         { "a=0,help,b", true, true, true },
         { "help,b=1", true, true, false },
-        { "a,b,,help", false /* BUG */, true, true },
+        { "a,b,,help", true, true, true },
     };
     int i;
     QemuOpts *opts;
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 6403e521fc..279f1b3fb3 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -165,26 +165,6 @@ void parse_option_size(const char *name, const char *value,
     *ret = size;
 }
 
-bool has_help_option(const char *param)
-{
-    const char *p = param;
-    bool result = false;
-
-    while (*p && !result) {
-        char *value;
-
-        p = get_opt_value(p, &value);
-        if (*p) {
-            p++;
-        }
-
-        result = is_help_option(value);
-        g_free(value);
-    }
-
-    return result;
-}
-
 bool is_valid_option_list(const char *p)
 {
     char *value = NULL;
@@ -890,6 +870,25 @@ static char *opts_parse_id(const char *params)
     return NULL;
 }
 
+bool has_help_option(const char *params)
+{
+    const char *p;
+    char *name, *value;
+    bool ret;
+
+    for (p = params; *p;) {
+        p = get_opt_name_value(p, NULL, &name, &value);
+        ret = !strcmp(name, "help");
+        g_free(name);
+        g_free(value);
+        if (ret) {
+            return true;
+        }
+    }
+
+    return false;
+}
+
 /**
  * Store options parsed from @params into @opts.
  * If @firstname is non-null, the first key=value in @params may omit
-- 
2.21.1

Re: [PATCH for-5.1 5/8] qemu-option: Fix has_help_option()'s sloppy parsing

[Eric Blake]

On 4/9/20 10:30 AM, Markus Armbruster wrote:
> has_help_option() uses its own parser.  It's inconsistent with
> qemu_opts_parse(), as demonstrated by test-qemu-opts case
> /qemu-opts/has_help_option.  Fix by reusing the common parser.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/test-qemu-opts.c |  2 +-
>   util/qemu-option.c     | 39 +++++++++++++++++++--------------------
>   2 files changed, 20 insertions(+), 21 deletions(-)
> 
> diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
> index 27c24bb1a2..58a4ea2408 100644
> --- a/tests/test-qemu-opts.c
> +++ b/tests/test-qemu-opts.c
> @@ -744,7 +744,7 @@ static void test_has_help_option(void)
>           { "a,help", true, true, true },
>           { "a=0,help,b", true, true, true },
>           { "help,b=1", true, true, false },
> -        { "a,b,,help", false /* BUG */, true, true },
> +        { "a,b,,help", true, true, true },

Okay, this revisits my question from 1/8.

I guess the argument is that since 'b,help' is NOT a valid option name 
(only as an option value), that we are instead parsing three separate 
options 'b', '', and 'help', and whether or not the empty option is 
valid, the face that 'help' is valid is what makes this return true?


> +++ b/util/qemu-option.c
> @@ -165,26 +165,6 @@ void parse_option_size(const char *name, const char *value,
>       *ret = size;
>   }
>   
> -bool has_help_option(const char *param)
> -{
> -    const char *p = param;
> -    bool result = false;
> -
> -    while (*p && !result) {
> -        char *value;
> -
> -        p = get_opt_value(p, &value);
> -        if (*p) {
> -            p++;
> -        }
> -
> -        result = is_help_option(value);

Old code: both 'help' and '?' are accepted.

> +bool has_help_option(const char *params)
> +{
> +    const char *p;
> +    char *name, *value;
> +    bool ret;
> +
> +    for (p = params; *p;) {
> +        p = get_opt_name_value(p, NULL, &name, &value);
> +        ret = !strcmp(name, "help");

New code: only 'help' is accepted.  Is the loss of '?' intentional?

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

Re: [PATCH for-5.1 5/8] qemu-option: Fix has_help_option()'s sloppy parsing

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 4/9/20 10:30 AM, Markus Armbruster wrote:
>> has_help_option() uses its own parser.  It's inconsistent with
>> qemu_opts_parse(), as demonstrated by test-qemu-opts case
>> /qemu-opts/has_help_option.  Fix by reusing the common parser.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   tests/test-qemu-opts.c |  2 +-
>>   util/qemu-option.c     | 39 +++++++++++++++++++--------------------
>>   2 files changed, 20 insertions(+), 21 deletions(-)
>>
>> diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
>> index 27c24bb1a2..58a4ea2408 100644
>> --- a/tests/test-qemu-opts.c
>> +++ b/tests/test-qemu-opts.c
>> @@ -744,7 +744,7 @@ static void test_has_help_option(void)
>>           { "a,help", true, true, true },
>>           { "a=0,help,b", true, true, true },
>>           { "help,b=1", true, true, false },
>> -        { "a,b,,help", false /* BUG */, true, true },
>> +        { "a,b,,help", true, true, true },
>
> Okay, this revisits my question from 1/8.
>
> I guess the argument is that since 'b,help' is NOT a valid option name
> (only as an option value), that we are instead parsing three separate
> options 'b', '', and 'help', and whether or not the empty option is
> valid, the face that 'help' is valid is what makes this return true?

Parsing is oblivious of which option names are valid.  It's actually
oblivious of the entire QemuOpts definition.

Desugaring may depend on the QemuOpts definition, however.

"a,b,,help" gets parsed as four option parameters:

    "a",    which gets desugared to either "a=on" or "firstname=a"
    "b",    which gets desugared to "b=on"
    "" ,    which gets desugared to "=on"
    "help", which gets desugared to "help=on"

A user of the parse that supports help should clue on the last one,
throw away the parse, and provide help.

The first desugaring is one that depends on the QemuOpts definition.

>> +++ b/util/qemu-option.c
>> @@ -165,26 +165,6 @@ void parse_option_size(const char *name, const char *value,
>>       *ret = size;
>>   }
>>   -bool has_help_option(const char *param)
>> -{
>> -    const char *p = param;
>> -    bool result = false;
>> -
>> -    while (*p && !result) {
>> -        char *value;
>> -
>> -        p = get_opt_value(p, &value);
>> -        if (*p) {
>> -            p++;
>> -        }
>> -
>> -        result = is_help_option(value);
>
> Old code: both 'help' and '?' are accepted.
>
>> +bool has_help_option(const char *params)
>> +{
>> +    const char *p;
>> +    char *name, *value;
>> +    bool ret;
>> +
>> +    for (p = params; *p;) {
>> +        p = get_opt_name_value(p, NULL, &name, &value);
>> +        ret = !strcmp(name, "help");
>
> New code: only 'help' is accepted.  Is the loss of '?' intentional?

No.  Will fix, thanks!

Re: [PATCH for-5.1 5/8] qemu-option: Fix has_help_option()'s sloppy parsing

[Kevin Wolf]

Am 14.04.2020 um 12:16 hat Markus Armbruster geschrieben:
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 4/9/20 10:30 AM, Markus Armbruster wrote:
> >> has_help_option() uses its own parser.  It's inconsistent with
> >> qemu_opts_parse(), as demonstrated by test-qemu-opts case
> >> /qemu-opts/has_help_option.  Fix by reusing the common parser.
> >>
> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>

> >> @@ -165,26 +165,6 @@ void parse_option_size(const char *name, const char *value,
> >>       *ret = size;
> >>   }
> >>   -bool has_help_option(const char *param)
> >> -{
> >> -    const char *p = param;
> >> -    bool result = false;
> >> -
> >> -    while (*p && !result) {
> >> -        char *value;
> >> -
> >> -        p = get_opt_value(p, &value);
> >> -        if (*p) {
> >> -            p++;
> >> -        }
> >> -
> >> -        result = is_help_option(value);
> >
> > Old code: both 'help' and '?' are accepted.
> >
> >> +bool has_help_option(const char *params)
> >> +{
> >> +    const char *p;
> >> +    char *name, *value;
> >> +    bool ret;
> >> +
> >> +    for (p = params; *p;) {
> >> +        p = get_opt_name_value(p, NULL, &name, &value);
> >> +        ret = !strcmp(name, "help");
> >
> > New code: only 'help' is accepted.  Is the loss of '?' intentional?
> 
> No.  Will fix, thanks!

Please also add some '?' test cases while you're at it.

Kevin

Re: [PATCH for-5.1 5/8] qemu-option: Fix has_help_option()'s sloppy parsing

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 14.04.2020 um 12:16 hat Markus Armbruster geschrieben:
>> Eric Blake <eblake@redhat.com> writes:
>> 
>> > On 4/9/20 10:30 AM, Markus Armbruster wrote:
>> >> has_help_option() uses its own parser.  It's inconsistent with
>> >> qemu_opts_parse(), as demonstrated by test-qemu-opts case
>> >> /qemu-opts/has_help_option.  Fix by reusing the common parser.
>> >>
>> >> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
>> >> @@ -165,26 +165,6 @@ void parse_option_size(const char *name, const char *value,
>> >>       *ret = size;
>> >>   }
>> >>   -bool has_help_option(const char *param)
>> >> -{
>> >> -    const char *p = param;
>> >> -    bool result = false;
>> >> -
>> >> -    while (*p && !result) {
>> >> -        char *value;
>> >> -
>> >> -        p = get_opt_value(p, &value);
>> >> -        if (*p) {
>> >> -            p++;
>> >> -        }
>> >> -
>> >> -        result = is_help_option(value);
>> >
>> > Old code: both 'help' and '?' are accepted.
>> >
>> >> +bool has_help_option(const char *params)
>> >> +{
>> >> +    const char *p;
>> >> +    char *name, *value;
>> >> +    bool ret;
>> >> +
>> >> +    for (p = params; *p;) {
>> >> +        p = get_opt_name_value(p, NULL, &name, &value);
>> >> +        ret = !strcmp(name, "help");
>> >
>> > New code: only 'help' is accepted.  Is the loss of '?' intentional?
>> 
>> No.  Will fix, thanks!
>
> Please also add some '?' test cases while you're at it.

Okay.

[PATCH for-5.1 6/8] test-qemu-opts: Simplify test_has_help_option() after bug fix

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 tests/test-qemu-opts.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/tests/test-qemu-opts.c b/tests/test-qemu-opts.c
index 58a4ea2408..909118d617 100644
--- a/tests/test-qemu-opts.c
+++ b/tests/test-qemu-opts.c
@@ -732,35 +732,33 @@ static void test_has_help_option(void)
 {
     static const struct {
         const char *params;
-        /* expected value of has_help_option() */
-        bool expect_has_help_option;
         /* expected value of qemu_opt_has_help_opt() with implied=false */
-        bool expect_opt_has_help_opt;
+        bool expect;
         /* expected value of qemu_opt_has_help_opt() with implied=true */
-        bool expect_opt_has_help_opt_implied;
+        bool expect_implied;
     } test[] = {
-        { "help", true, true, false },
-        { "helpme", false, false, false },
-        { "a,help", true, true, true },
-        { "a=0,help,b", true, true, true },
-        { "help,b=1", true, true, false },
-        { "a,b,,help", true, true, true },
+        { "help", true, false },
+        { "helpme", false, false },
+        { "a,help", true, true },
+        { "a=0,help,b", true, true },
+        { "help,b=1", true, false },
+        { "a,b,,help", true, true },
     };
     int i;
     QemuOpts *opts;
 
     for (i = 0; i < ARRAY_SIZE(test); i++) {
         g_assert_cmpint(has_help_option(test[i].params),
-                        ==, test[i].expect_has_help_option);
+                        ==, test[i].expect);
         opts = qemu_opts_parse(&opts_list_03, test[i].params, false,
                                &error_abort);
         g_assert_cmpint(qemu_opt_has_help_opt(opts),
-                        ==, test[i].expect_opt_has_help_opt);
+                        ==, test[i].expect);
         qemu_opts_del(opts);
         opts = qemu_opts_parse(&opts_list_03, test[i].params, true,
                                &error_abort);
         g_assert_cmpint(qemu_opt_has_help_opt(opts),
-                        ==, test[i].expect_opt_has_help_opt_implied);
+                        ==, test[i].expect_implied);
         qemu_opts_del(opts);
     }
 }
-- 
2.21.1

Re: [PATCH for-5.1 6/8] test-qemu-opts: Simplify test_has_help_option() after bug fix

[Eric Blake]

On 4/9/20 10:30 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   tests/test-qemu-opts.c | 24 +++++++++++-------------
>   1 file changed, 11 insertions(+), 13 deletions(-)
> 

Assuming that the interpretation of a,b,,help is intended to match the 
interpretation you chose in 5/8 (and not the one I questioned in 1/8), 
this is fine.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH for-5.1 6/8] test-qemu-opts: Simplify test_has_help_option() after bug fix

[Kevin Wolf]

Am 09.04.2020 um 17:30 hat Markus Armbruster geschrieben:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[PATCH for-5.1 7/8] qemu-img: Factor out accumulate_options() helper

[Markus Armbruster]

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 qemu-img.c | 59 +++++++++++++++++++++---------------------------------
 1 file changed, 23 insertions(+), 36 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index b167376bd7..28b27b738a 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -223,6 +223,25 @@ static bool qemu_img_object_print_help(const char *type, QemuOpts *opts)
     return true;
 }
 
+static int accumulate_options(char **options, char *optarg)
+{
+    char *new_options;
+
+    if (!is_valid_option_list(optarg)) {
+        error_report("Invalid option list: %s", optarg);
+        return -1;
+    }
+
+    if (!*options) {
+        *options = g_strdup(optarg);
+    } else {
+        new_options = g_strdup_printf("%s,%s", *options, optarg);
+        g_free(*options);
+        *options = new_options;
+    }
+    return 0;
+}
+
 static QemuOptsList qemu_source_opts = {
     .name = "source",
     .implied_opt_name = "file",
@@ -482,17 +501,9 @@ static int img_create(int argc, char **argv)
             fmt = optarg;
             break;
         case 'o':
-            if (!is_valid_option_list(optarg)) {
-                error_report("Invalid option list: %s", optarg);
+            if (accumulate_options(&options, optarg) < 0) {
                 goto fail;
             }
-            if (!options) {
-                options = g_strdup(optarg);
-            } else {
-                char *old_options = options;
-                options = g_strdup_printf("%s,%s", options, optarg);
-                g_free(old_options);
-            }
             break;
         case 'q':
             quiet = true;
@@ -2127,17 +2138,9 @@ static int img_convert(int argc, char **argv)
             s.compressed = true;
             break;
         case 'o':
-            if (!is_valid_option_list(optarg)) {
-                error_report("Invalid option list: %s", optarg);
+            if (accumulate_options(&options, optarg) < 0) {
                 goto fail_getopt;
             }
-            if (!options) {
-                options = g_strdup(optarg);
-            } else {
-                char *old_options = options;
-                options = g_strdup_printf("%s,%s", options, optarg);
-                g_free(old_options);
-            }
             break;
         case 'l':
             if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
@@ -3953,18 +3956,10 @@ static int img_amend(int argc, char **argv)
             help();
             break;
         case 'o':
-            if (!is_valid_option_list(optarg)) {
-                error_report("Invalid option list: %s", optarg);
+            if (accumulate_options(&options, optarg) < 0) {
                 ret = -1;
                 goto out_no_progress;
             }
-            if (!options) {
-                options = g_strdup(optarg);
-            } else {
-                char *old_options = options;
-                options = g_strdup_printf("%s,%s", options, optarg);
-                g_free(old_options);
-            }
             break;
         case 'f':
             fmt = optarg;
@@ -4855,17 +4850,9 @@ static int img_measure(int argc, char **argv)
             out_fmt = optarg;
             break;
         case 'o':
-            if (!is_valid_option_list(optarg)) {
-                error_report("Invalid option list: %s", optarg);
+            if (accumulate_options(&options, optarg) < 0) {
                 goto out;
             }
-            if (!options) {
-                options = g_strdup(optarg);
-            } else {
-                char *old_options = options;
-                options = g_strdup_printf("%s,%s", options, optarg);
-                g_free(old_options);
-            }
             break;
         case 'l':
             if (strstart(optarg, SNAPSHOT_OPT_BASE, NULL)) {
-- 
2.21.1

Re: [PATCH for-5.1 7/8] qemu-img: Factor out accumulate_options() helper

[Eric Blake]

On 4/9/20 10:30 AM, Markus Armbruster wrote:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   qemu-img.c | 59 +++++++++++++++++++++---------------------------------
>   1 file changed, 23 insertions(+), 36 deletions(-)
> 
Nice.
Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH for-5.1 7/8] qemu-img: Factor out accumulate_options() helper

[Kevin Wolf]

Am 09.04.2020 um 17:30 hat Markus Armbruster geschrieben:
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

[PATCH for-5.1 8/8] qemu-option: Move is_valid_option_list() to qemu-img.c and rewrite

[Markus Armbruster]

is_valid_option_list()'s purpose is ensuring qemu-img.c's can safely
join multiple parameter strings separated by ',' like this:

        g_strdup_printf("%s,%s", params1, params2);

How it does that is anything but obvious.  A close reading of the code
reveals that it fails exactly when its argument starts with ',' or
ends with an odd number of ','.  Makes sense, actually, because when
the argument starts with ',', a separating ',' preceding it would get
escaped, and when it ends with an odd number of ',', a separating ','
following it would get escaped.

Move it to qemu-img.c and rewrite it the obvious way.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 include/qemu/option.h |  1 -
 qemu-img.c            | 26 ++++++++++++++++++++++++++
 util/qemu-option.c    | 22 ----------------------
 3 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/include/qemu/option.h b/include/qemu/option.h
index 844587cab3..eb4097889d 100644
--- a/include/qemu/option.h
+++ b/include/qemu/option.h
@@ -33,7 +33,6 @@ const char *get_opt_value(const char *p, char **value);
 void parse_option_size(const char *name, const char *value,
                        uint64_t *ret, Error **errp);
 bool has_help_option(const char *param);
-bool is_valid_option_list(const char *param);
 
 enum QemuOptType {
     QEMU_OPT_STRING = 0,  /* no parsing (use string as-is)                        */
diff --git a/qemu-img.c b/qemu-img.c
index 28b27b738a..d8ca8a73e2 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -223,6 +223,32 @@ static bool qemu_img_object_print_help(const char *type, QemuOpts *opts)
     return true;
 }
 
+/*
+ * Is @optarg safe for accumulate_options()?
+ * It is when multiple of them can be joined together separated by ','.
+ * To make that work, @optarg must not start with ',' (or else a
+ * separating ',' preceding it gets escaped), and it must not end with
+ * an odd number of ',' (or else a separating ',' following it gets
+ * escaped).
+ */
+static bool is_valid_option_list(const char *optarg)
+{
+    size_t len = strlen(optarg);
+    size_t i;
+
+    if (optarg[0] == ',') {
+        return false;
+    }
+
+    for (i = len; i > 0 && optarg[i - 1] == ','; i--) {
+    }
+    if ((len - i) % 2) {
+        return false;
+    }
+
+    return true;
+}
+
 static int accumulate_options(char **options, char *optarg)
 {
     char *new_options;
diff --git a/util/qemu-option.c b/util/qemu-option.c
index 279f1b3fb3..6f80807738 100644
--- a/util/qemu-option.c
+++ b/util/qemu-option.c
@@ -165,28 +165,6 @@ void parse_option_size(const char *name, const char *value,
     *ret = size;
 }
 
-bool is_valid_option_list(const char *p)
-{
-    char *value = NULL;
-    bool result = false;
-
-    while (*p) {
-        p = get_opt_value(p, &value);
-        if ((*p && !*++p) ||
-            (!*value || *value == ',')) {
-            goto out;
-        }
-
-        g_free(value);
-        value = NULL;
-    }
-
-    result = true;
-out:
-    g_free(value);
-    return result;
-}
-
 static const char *opt_type_to_string(enum QemuOptType type)
 {
     switch (type) {
-- 
2.21.1

Re: [PATCH for-5.1 8/8] qemu-option: Move is_valid_option_list() to qemu-img.c and rewrite

[Eric Blake]

On 4/9/20 10:30 AM, Markus Armbruster wrote:
> is_valid_option_list()'s purpose is ensuring qemu-img.c's can safely
> join multiple parameter strings separated by ',' like this:
> 
>          g_strdup_printf("%s,%s", params1, params2);
> 
> How it does that is anything but obvious.  A close reading of the code
> reveals that it fails exactly when its argument starts with ',' or
> ends with an odd number of ','.  Makes sense, actually, because when
> the argument starts with ',', a separating ',' preceding it would get
> escaped, and when it ends with an odd number of ',', a separating ','
> following it would get escaped.
> 
> Move it to qemu-img.c and rewrite it the obvious way.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>   include/qemu/option.h |  1 -
>   qemu-img.c            | 26 ++++++++++++++++++++++++++
>   util/qemu-option.c    | 22 ----------------------
>   3 files changed, 26 insertions(+), 23 deletions(-)
> 

> +++ b/qemu-img.c
> @@ -223,6 +223,32 @@ static bool qemu_img_object_print_help(const char *type, QemuOpts *opts)
>       return true;
>   }
>   
> +/*
> + * Is @optarg safe for accumulate_options()?
> + * It is when multiple of them can be joined together separated by ','.
> + * To make that work, @optarg must not start with ',' (or else a
> + * separating ',' preceding it gets escaped), and it must not end with
> + * an odd number of ',' (or else a separating ',' following it gets
> + * escaped).
> + */
> +static bool is_valid_option_list(const char *optarg)
> +{
> +    size_t len = strlen(optarg);
> +    size_t i;
> +
> +    if (optarg[0] == ',') {
> +        return false;
> +    }
> +
> +    for (i = len; i > 0 && optarg[i - 1] == ','; i--) {
> +    }
> +    if ((len - i) % 2) {
> +        return false;
> +    }
> +
> +    return true;

Okay, that's easy to read.  Note that is_valid_option_list("") returns 
true.  But proving it replaces the obtuse mess is harder...

> +++ b/util/qemu-option.c
> @@ -165,28 +165,6 @@ void parse_option_size(const char *name, const char *value,
>       *ret = size;
>   }
>   
> -bool is_valid_option_list(const char *p)
> -{
> -    char *value = NULL;
> -    bool result = false;
> -
> -    while (*p) {
> -        p = get_opt_value(p, &value);

Refreshing myself on what get_opt_value() does:

> const char *get_opt_value(const char *p, char **value)
> {
>     size_t capacity = 0, length;
>     const char *offset;
> 
>     *value = NULL;

start with p pointing to the tail of a string from which to extract a value,

>     while (1) {
>         offset = qemu_strchrnul(p, ',');

find the potential end of the value: either the first comma (not yet 
sure if it is ',,' or the end of this value), or the end of the string,

>         length = offset - p;
>         if (*offset != '\0' && *(offset + 1) == ',') {
>             length++;
>         }

if we found a comma and it was doubled, we are going to unescape the comma,

>         *value = g_renew(char, *value, capacity + length + 1);
>         strncpy(*value + capacity, p, length);

copy bytes into the tail of value,

>         (*value)[capacity + length] = '\0';
>         capacity += length;
>         if (*offset == '\0' ||
>             *(offset + 1) != ',') {
>             break;
>         }

if we hit the end of the string or the ',' before the next option, we 
are done,

> 
>         p += (offset - p) + 2;

otherwise we unescaped a ',,' and continue appending to the current value.

>     }
> 
>     return offset;
> }

and the resulting return value is a substring of p: either the NUL byte 
ending p, or the last ',' in the first odd run of commas (the next byte 
after the return is then NUL if the parser would next see an empty 
option name, or non-NUL if the parser would start processing the next 
option name).

Some interesting cases:
get_opt_value("", &value) returns "" with value set to ""
get_opt_value(",", &value) returns "," with value set to ""
get_opt_value(",,", &value) returns "" with value set to ","
get_opt_value(",,,", &value) returns "," with value set to ","
get_opt_value(",,a", &value) returns "" with value set to ",a"
get_opt_value("a,,", &value) returns "" with value set to "a,"
get_opt_value("a,b", &value) returns ",b" with value set to "a"
get_opt_value("a,,b", &value) returns "" with value set to "a,b"

With that detour out of the way:

> -        if ((*p && !*++p) ||

If *p, then we know '*p' == ','; checking !*++p moves past the comma and 
determines if we have the empty string as the next potential option name 
(if so, we ended in an odd number of commas) or anything else (if so, 
repeat the loop, just past the comma).  Oddly, this means that 
is_valid_option_list("a,,,b") returns true (first pass on "a,,,b" 
returned ",b", second pass on "b" returned "").  But I agree that !*++p 
only fires on the final iteration through our loop of get_opt_value() 
calls, matching your rewrite to check for an odd number of trailing 
commas (regardless of even or odd pairing of commas in the middle).

> -            (!*value || *value == ',')) {

Focusing on "*value == ','", on the first iteration, *value can be 
comma; on later iterations, we are guaranteed that *value will not be 
comma (because later iterations can only occur if the first iteration 
returned p pointing to a final comma of a run, but we always start the 
next iteration beyond that comma).  So this matches your rewrite to 
check for a leading comma.

Focusing on "!*value": the loop is never entered to begin with on 
is_valid_option_list("") (that returns true in the old implementation as 
well as yours); otherwise, value will only be empty if p originally 
started with an unpaired comma (but your new code catches that with its 
check for a leading comma).

Note that is_valid_option_list("") returning true has some odd effects:

$ qemu-img create -f qcow2 -o '' xyz 1
Formatting 'xyz', fmt=qcow2 size=1 cluster_size=65536 lazy_refcounts=off 
refcount_bits=16
$ qemu-img create -f qcow2 -o '' -o '' xyz 1
qemu-img: xyz: Invalid parameter ''
$ qemu-img create -f qcow2 -o 'help' -o '' -o '' xyz 1
qemu-img: xyz: Invalid parameter 'help'
$ qemu-img create -f qcow2 -o 'help' -o '' xyz 1 | head -n1
Supported options:

but as I can't see any substantial differences between the old algorithm 
and your new one, I don't see that changing here.  Perhaps you want 
another patch to make is_valid_option_list() return false for "".


> -            goto out;
> -        }
> -
> -        g_free(value);
> -        value = NULL;
> -    }
> -
> -    result = true;
> -out:
> -    g_free(value);
> -    return result;
> -}

At any rate, your new code is a LOT more efficient and legible than the 
old.  I'll restate the "why" in my own words: is_valid_option_list() is 
only used by our code base to concatenate multiple -o strings into one 
string, where each -o individually parses into one or more options; and 
where the concatenated string must parse back to the same number of 
embedded options.  As both -o ",help" and -o "help," are invalid option 
lists on their own (there is no valid empty option for -o), it makes no 
sense to append that -o to other option lists to produce a single string 
(where the appending would become confusing due to creating escaped ',,' 
that were not present in either original -o).

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH for-5.1 8/8] qemu-option: Move is_valid_option_list() to qemu-img.c and rewrite

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 4/9/20 10:30 AM, Markus Armbruster wrote:
>> is_valid_option_list()'s purpose is ensuring qemu-img.c's can safely
>> join multiple parameter strings separated by ',' like this:
>>
>>          g_strdup_printf("%s,%s", params1, params2);
>>
>> How it does that is anything but obvious.  A close reading of the code
>> reveals that it fails exactly when its argument starts with ',' or
>> ends with an odd number of ','.  Makes sense, actually, because when
>> the argument starts with ',', a separating ',' preceding it would get
>> escaped, and when it ends with an odd number of ',', a separating ','
>> following it would get escaped.
>>
>> Move it to qemu-img.c and rewrite it the obvious way.
>>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>   include/qemu/option.h |  1 -
>>   qemu-img.c            | 26 ++++++++++++++++++++++++++
>>   util/qemu-option.c    | 22 ----------------------
>>   3 files changed, 26 insertions(+), 23 deletions(-)
>>
>
>> +++ b/qemu-img.c
>> @@ -223,6 +223,32 @@ static bool qemu_img_object_print_help(const char *type, QemuOpts *opts)
>>       return true;
>>   }
>>   +/*
>> + * Is @optarg safe for accumulate_options()?
>> + * It is when multiple of them can be joined together separated by ','.
>> + * To make that work, @optarg must not start with ',' (or else a
>> + * separating ',' preceding it gets escaped), and it must not end with
>> + * an odd number of ',' (or else a separating ',' following it gets
>> + * escaped).
>> + */
>> +static bool is_valid_option_list(const char *optarg)
>> +{
>> +    size_t len = strlen(optarg);
>> +    size_t i;
>> +
>> +    if (optarg[0] == ',') {
>> +        return false;
>> +    }
>> +
>> +    for (i = len; i > 0 && optarg[i - 1] == ','; i--) {
>> +    }
>> +    if ((len - i) % 2) {
>> +        return false;
>> +    }
>> +
>> +    return true;
>
> Okay, that's easy to read.  Note that is_valid_option_list("") returns
> true.

Hmm, that's a bug:

    $ qemu-img create -f qcow2 -o backing_file=a -o "" -o backing_fmt=raw,size=1M new.qcow2
    qemu-img: warning: Could not verify backing image. This may become an error in future versions.
    Could not open 'a,backing_fmt=raw': No such file or directory
    Formatting 'new.qcow2', fmt=qcow2 size=1048576 backing_file=a,,backing_fmt=raw cluster_size=65536 lazy_refcounts=off refcount_bits=16
    $ qemu-img info new.qcow2 
    image: new.qcow2
    file format: qcow2
    virtual size: 1 MiB (1048576 bytes)
    disk size: 196 KiB
    cluster_size: 65536
--> backing file: a,backing_fmt=raw
    Format specific information:
        compat: 1.1
        lazy refcounts: false
        refcount bits: 16
        corrupt: false

My rewrite preserves this bug.  Will fix in v2.

>        But proving it replaces the obtuse mess is harder...
>
>> +++ b/util/qemu-option.c
>> @@ -165,28 +165,6 @@ void parse_option_size(const char *name, const char *value,
>>       *ret = size;
>>   }
>>   -bool is_valid_option_list(const char *p)
>> -{
>> -    char *value = NULL;
>> -    bool result = false;
>> -
>> -    while (*p) {
>> -        p = get_opt_value(p, &value);
>
> Refreshing myself on what get_opt_value() does:
>
>> const char *get_opt_value(const char *p, char **value)
>> {
>>     size_t capacity = 0, length;
>>     const char *offset;
>>
>>     *value = NULL;
>
> start with p pointing to the tail of a string from which to extract a value,
>
>>     while (1) {
>>         offset = qemu_strchrnul(p, ',');
>
> find the potential end of the value: either the first comma (not yet
> sure if it is ',,' or the end of this value), or the end of the
> string,
>
>>         length = offset - p;
>>         if (*offset != '\0' && *(offset + 1) == ',') {
>>             length++;
>>         }
>
> if we found a comma and it was doubled, we are going to unescape the comma,
>
>>         *value = g_renew(char, *value, capacity + length + 1);
>>         strncpy(*value + capacity, p, length);
>
> copy bytes into the tail of value,
>
>>         (*value)[capacity + length] = '\0';
>>         capacity += length;
>>         if (*offset == '\0' ||
>>             *(offset + 1) != ',') {
>>             break;
>>         }
>
> if we hit the end of the string or the ',' before the next option, we
> are done,
>
>>
>>         p += (offset - p) + 2;
>
> otherwise we unescaped a ',,' and continue appending to the current value.
>
>>     }
>>
>>     return offset;
>> }
>
> and the resulting return value is a substring of p: either the NUL
> byte ending p, or the last ',' in the first odd run of commas (the
> next byte after the return is then NUL if the parser would next see an
> empty option name, or non-NUL if the parser would start processing the
> next option name).

Yes.

> Some interesting cases:
> get_opt_value("", &value) returns "" with value set to ""
> get_opt_value(",", &value) returns "," with value set to ""
> get_opt_value(",,", &value) returns "" with value set to ","
> get_opt_value(",,,", &value) returns "," with value set to ","
> get_opt_value(",,a", &value) returns "" with value set to ",a"
> get_opt_value("a,,", &value) returns "" with value set to "a,"
> get_opt_value("a,b", &value) returns ",b" with value set to "a"
> get_opt_value("a,,b", &value) returns "" with value set to "a,b"

QemuOpts is a language without syntax errors.

> With that detour out of the way:
>
>> -        if ((*p && !*++p) ||
>
> If *p, then we know '*p' == ','; checking !*++p moves past the comma
> and determines if we have the empty string as the next potential
> option name (if so, we ended in an odd number of commas) or anything
> else (if so, repeat the loop, just past the comma).  Oddly, this means
> that is_valid_option_list("a,,,b") returns true (first pass on "a,,,b"
> returned ",b", second pass on "b" returned "").  But I agree that
> !*++p only fires on the final iteration through our loop of
> get_opt_value() calls, matching your rewrite to check for an odd
> number of trailing commas (regardless of even or odd pairing of commas
> in the middle).
>
>> -            (!*value || *value == ',')) {
>
> Focusing on "*value == ','", on the first iteration, *value can be
> comma; on later iterations, we are guaranteed that *value will not be
> comma (because later iterations can only occur if the first iteration
> returned p pointing to a final comma of a run, but we always start the
> next iteration beyond that comma).  So this matches your rewrite to
> check for a leading comma.
>
> Focusing on "!*value": the loop is never entered to begin with on
> is_valid_option_list("") (that returns true in the old implementation
> as well as yours); otherwise, value will only be empty if p originally
> started with an unpaired comma (but your new code catches that with
> its check for a leading comma).
>
> Note that is_valid_option_list("") returning true has some odd effects:
>
> $ qemu-img create -f qcow2 -o '' xyz 1
> Formatting 'xyz', fmt=qcow2 size=1 cluster_size=65536
> lazy_refcounts=off refcount_bits=16
> $ qemu-img create -f qcow2 -o '' -o '' xyz 1
> qemu-img: xyz: Invalid parameter ''
> $ qemu-img create -f qcow2 -o 'help' -o '' -o '' xyz 1
> qemu-img: xyz: Invalid parameter 'help'
> $ qemu-img create -f qcow2 -o 'help' -o '' xyz 1 | head -n1
> Supported options:
>
> but as I can't see any substantial differences between the old
> algorithm and your new one, I don't see that changing here.  Perhaps
> you want another patch to make is_valid_option_list() return false for
> "".

I do.

>> -            goto out;
>> -        }
>> -
>> -        g_free(value);
>> -        value = NULL;
>> -    }
>> -
>> -    result = true;
>> -out:
>> -    g_free(value);
>> -    return result;
>> -}
>
> At any rate, your new code is a LOT more efficient and legible than
> the old.  I'll restate the "why" in my own words:
> is_valid_option_list() is only used by our code base to concatenate
> multiple -o strings into one string, where each -o individually parses
> into one or more options;

Correct.

>                           and where the concatenated string must parse
> back to the same number of embedded options.

We want the concatenated string to parse into the concatenation of the
parse of its parts.  Whether "same number" implies that is an
interesting puzzle, but not one worth solving today :)

>                                               As both -o ",help" and
> -o "help," are invalid option lists on their own (there is no valid
> empty option for -o),

Yes, it's *semantically* invalid.  It parses as sugar for "help=on,=on",
except "help=on" doesn't get you help, only "help" does.

>                       it makes no sense to append that -o to other
> option lists to produce a single string (where the appending would
> become confusing due to creating escaped ',,' that were not present in
> either original -o).

Yes.  If the empty option existed, concatenation would not do.  It
doesn't now, and I expect it to stay that way.

> Reviewed-by: Eric Blake <eblake@redhat.com>

Thanks for your thorough review!

Re: [PATCH for-5.1 8/8] qemu-option: Move is_valid_option_list() to qemu-img.c and rewrite

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Eric Blake <eblake@redhat.com> writes:
>
>> On 4/9/20 10:30 AM, Markus Armbruster wrote:
>>> is_valid_option_list()'s purpose is ensuring qemu-img.c's can safely
>>> join multiple parameter strings separated by ',' like this:
>>>
>>>          g_strdup_printf("%s,%s", params1, params2);
>>>
>>> How it does that is anything but obvious.  A close reading of the code
>>> reveals that it fails exactly when its argument starts with ',' or
>>> ends with an odd number of ','.  Makes sense, actually, because when
>>> the argument starts with ',', a separating ',' preceding it would get
>>> escaped, and when it ends with an odd number of ',', a separating ','
>>> following it would get escaped.
>>>
>>> Move it to qemu-img.c and rewrite it the obvious way.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>>   include/qemu/option.h |  1 -
>>>   qemu-img.c            | 26 ++++++++++++++++++++++++++
>>>   util/qemu-option.c    | 22 ----------------------
>>>   3 files changed, 26 insertions(+), 23 deletions(-)
>>>
>>
>>> +++ b/qemu-img.c
>>> @@ -223,6 +223,32 @@ static bool qemu_img_object_print_help(const char *type, QemuOpts *opts)
>>>       return true;
>>>   }
>>>   +/*
>>> + * Is @optarg safe for accumulate_options()?
>>> + * It is when multiple of them can be joined together separated by ','.
>>> + * To make that work, @optarg must not start with ',' (or else a
>>> + * separating ',' preceding it gets escaped), and it must not end with
>>> + * an odd number of ',' (or else a separating ',' following it gets
>>> + * escaped).
>>> + */
>>> +static bool is_valid_option_list(const char *optarg)
>>> +{
>>> +    size_t len = strlen(optarg);
>>> +    size_t i;
>>> +
>>> +    if (optarg[0] == ',') {
>>> +        return false;
>>> +    }
>>> +
>>> +    for (i = len; i > 0 && optarg[i - 1] == ','; i--) {
>>> +    }
>>> +    if ((len - i) % 2) {
>>> +        return false;
>>> +    }
>>> +
>>> +    return true;
>>
>> Okay, that's easy to read.  Note that is_valid_option_list("") returns
>> true.
>
> Hmm, that's a bug:
>
>     $ qemu-img create -f qcow2 -o backing_file=a -o "" -o backing_fmt=raw,size=1M new.qcow2
>     qemu-img: warning: Could not verify backing image. This may become an error in future versions.
>     Could not open 'a,backing_fmt=raw': No such file or directory
>     Formatting 'new.qcow2', fmt=qcow2 size=1048576 backing_file=a,,backing_fmt=raw cluster_size=65536 lazy_refcounts=off refcount_bits=16
>     $ qemu-img info new.qcow2 
>     image: new.qcow2
>     file format: qcow2
>     virtual size: 1 MiB (1048576 bytes)
>     disk size: 196 KiB
>     cluster_size: 65536
> --> backing file: a,backing_fmt=raw
>     Format specific information:
>         compat: 1.1
>         lazy refcounts: false
>         refcount bits: 16
>         corrupt: false
>
> My rewrite preserves this bug.  Will fix in v2.

Kevin, two obvious fixes:

* Make is_valid_option_list() reject -o ""

* Make accumulate_options(options, "") return options.

Got a preference?

[...]

Re: [PATCH for-5.1 8/8] qemu-option: Move is_valid_option_list() to qemu-img.c and rewrite

[Kevin Wolf]

Am 14.04.2020 um 16:34 hat Markus Armbruster geschrieben:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Eric Blake <eblake@redhat.com> writes:
> >
> >> On 4/9/20 10:30 AM, Markus Armbruster wrote:
> >>> is_valid_option_list()'s purpose is ensuring qemu-img.c's can safely
> >>> join multiple parameter strings separated by ',' like this:
> >>>
> >>>          g_strdup_printf("%s,%s", params1, params2);
> >>>
> >>> How it does that is anything but obvious.  A close reading of the code
> >>> reveals that it fails exactly when its argument starts with ',' or
> >>> ends with an odd number of ','.  Makes sense, actually, because when
> >>> the argument starts with ',', a separating ',' preceding it would get
> >>> escaped, and when it ends with an odd number of ',', a separating ','
> >>> following it would get escaped.
> >>>
> >>> Move it to qemu-img.c and rewrite it the obvious way.
> >>>
> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> >>> ---
> >>>   include/qemu/option.h |  1 -
> >>>   qemu-img.c            | 26 ++++++++++++++++++++++++++
> >>>   util/qemu-option.c    | 22 ----------------------
> >>>   3 files changed, 26 insertions(+), 23 deletions(-)
> >>>
> >>
> >>> +++ b/qemu-img.c
> >>> @@ -223,6 +223,32 @@ static bool qemu_img_object_print_help(const char *type, QemuOpts *opts)
> >>>       return true;
> >>>   }
> >>>   +/*
> >>> + * Is @optarg safe for accumulate_options()?
> >>> + * It is when multiple of them can be joined together separated by ','.
> >>> + * To make that work, @optarg must not start with ',' (or else a
> >>> + * separating ',' preceding it gets escaped), and it must not end with
> >>> + * an odd number of ',' (or else a separating ',' following it gets
> >>> + * escaped).
> >>> + */
> >>> +static bool is_valid_option_list(const char *optarg)
> >>> +{
> >>> +    size_t len = strlen(optarg);
> >>> +    size_t i;
> >>> +
> >>> +    if (optarg[0] == ',') {
> >>> +        return false;
> >>> +    }
> >>> +
> >>> +    for (i = len; i > 0 && optarg[i - 1] == ','; i--) {
> >>> +    }
> >>> +    if ((len - i) % 2) {
> >>> +        return false;
> >>> +    }
> >>> +
> >>> +    return true;
> >>
> >> Okay, that's easy to read.  Note that is_valid_option_list("") returns
> >> true.
> >
> > Hmm, that's a bug:
> >
> >     $ qemu-img create -f qcow2 -o backing_file=a -o "" -o backing_fmt=raw,size=1M new.qcow2
> >     qemu-img: warning: Could not verify backing image. This may become an error in future versions.
> >     Could not open 'a,backing_fmt=raw': No such file or directory
> >     Formatting 'new.qcow2', fmt=qcow2 size=1048576 backing_file=a,,backing_fmt=raw cluster_size=65536 lazy_refcounts=off refcount_bits=16
> >     $ qemu-img info new.qcow2 
> >     image: new.qcow2
> >     file format: qcow2
> >     virtual size: 1 MiB (1048576 bytes)
> >     disk size: 196 KiB
> >     cluster_size: 65536
> > --> backing file: a,backing_fmt=raw
> >     Format specific information:
> >         compat: 1.1
> >         lazy refcounts: false
> >         refcount bits: 16
> >         corrupt: false
> >
> > My rewrite preserves this bug.  Will fix in v2.
> 
> Kevin, two obvious fixes:
> 
> * Make is_valid_option_list() reject -o ""
> 
> * Make accumulate_options(options, "") return options.
> 
> Got a preference?

In other words, the choice is between reporting an error and ignoring it
silently. I think reporting an error makes more sense.

Kevin

Re: [PATCH for-5.1 8/8] qemu-option: Move is_valid_option_list() to qemu-img.c and rewrite

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

> Am 14.04.2020 um 16:34 hat Markus Armbruster geschrieben:
>> Markus Armbruster <armbru@redhat.com> writes:
>> 
>> > Eric Blake <eblake@redhat.com> writes:
>> >
>> >> On 4/9/20 10:30 AM, Markus Armbruster wrote:
>> >>> is_valid_option_list()'s purpose is ensuring qemu-img.c's can safely
>> >>> join multiple parameter strings separated by ',' like this:
>> >>>
>> >>>          g_strdup_printf("%s,%s", params1, params2);
>> >>>
>> >>> How it does that is anything but obvious.  A close reading of the code
>> >>> reveals that it fails exactly when its argument starts with ',' or
>> >>> ends with an odd number of ','.  Makes sense, actually, because when
>> >>> the argument starts with ',', a separating ',' preceding it would get
>> >>> escaped, and when it ends with an odd number of ',', a separating ','
>> >>> following it would get escaped.
>> >>>
>> >>> Move it to qemu-img.c and rewrite it the obvious way.
>> >>>
>> >>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> >>> ---
>> >>>   include/qemu/option.h |  1 -
>> >>>   qemu-img.c            | 26 ++++++++++++++++++++++++++
>> >>>   util/qemu-option.c    | 22 ----------------------
>> >>>   3 files changed, 26 insertions(+), 23 deletions(-)
>> >>>
>> >>
>> >>> +++ b/qemu-img.c
>> >>> @@ -223,6 +223,32 @@ static bool qemu_img_object_print_help(const char *type, QemuOpts *opts)
>> >>>       return true;
>> >>>   }
>> >>>   +/*
>> >>> + * Is @optarg safe for accumulate_options()?
>> >>> + * It is when multiple of them can be joined together separated by ','.
>> >>> + * To make that work, @optarg must not start with ',' (or else a
>> >>> + * separating ',' preceding it gets escaped), and it must not end with
>> >>> + * an odd number of ',' (or else a separating ',' following it gets
>> >>> + * escaped).
>> >>> + */
>> >>> +static bool is_valid_option_list(const char *optarg)
>> >>> +{
>> >>> +    size_t len = strlen(optarg);
>> >>> +    size_t i;
>> >>> +
>> >>> +    if (optarg[0] == ',') {
>> >>> +        return false;
>> >>> +    }
>> >>> +
>> >>> +    for (i = len; i > 0 && optarg[i - 1] == ','; i--) {
>> >>> +    }
>> >>> +    if ((len - i) % 2) {
>> >>> +        return false;
>> >>> +    }
>> >>> +
>> >>> +    return true;
>> >>
>> >> Okay, that's easy to read.  Note that is_valid_option_list("") returns
>> >> true.
>> >
>> > Hmm, that's a bug:
>> >
>> >     $ qemu-img create -f qcow2 -o backing_file=a -o "" -o backing_fmt=raw,size=1M new.qcow2
>> >     qemu-img: warning: Could not verify backing image. This may become an error in future versions.
>> >     Could not open 'a,backing_fmt=raw': No such file or directory
>> >     Formatting 'new.qcow2', fmt=qcow2 size=1048576 backing_file=a,,backing_fmt=raw cluster_size=65536 lazy_refcounts=off refcount_bits=16
>> >     $ qemu-img info new.qcow2 
>> >     image: new.qcow2
>> >     file format: qcow2
>> >     virtual size: 1 MiB (1048576 bytes)
>> >     disk size: 196 KiB
>> >     cluster_size: 65536
>> > --> backing file: a,backing_fmt=raw
>> >     Format specific information:
>> >         compat: 1.1
>> >         lazy refcounts: false
>> >         refcount bits: 16
>> >         corrupt: false
>> >
>> > My rewrite preserves this bug.  Will fix in v2.
>> 
>> Kevin, two obvious fixes:
>> 
>> * Make is_valid_option_list() reject -o ""
>> 
>> * Make accumulate_options(options, "") return options.
>> 
>> Got a preference?
>
> In other words, the choice is between reporting an error and ignoring it
> silently. I think reporting an error makes more sense.

Thanks!

Re: [PATCH for-5.1 0/8] qemu-option: Fix corner cases and clean up

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200409153041.17576-1-armbru@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [PATCH for-5.1 0/8] qemu-option: Fix corner cases and clean up
Message-id: 20200409153041.17576-1-armbru@redhat.com
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20200409164954.36902-1-peterx@redhat.com -> patchew/20200409164954.36902-1-peterx@redhat.com
Switched to a new branch 'test'
505f5f3 qemu-option: Move is_valid_option_list() to qemu-img.c and rewrite
b164930 qemu-img: Factor out accumulate_options() helper
b7fcaae test-qemu-opts: Simplify test_has_help_option() after bug fix
6845c29 qemu-option: Fix has_help_option()'s sloppy parsing
5c1b2db qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily()
b3630a3 qemu-option: Fix sloppy recognition of "id=..." after ", , "
8bb805d qemu-options: Factor out get_opt_name_value() helper
2e00310 tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt()

=== OUTPUT BEGIN ===
1/8 Checking commit 2e003109273b (tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt())
WARNING: Block comments use a leading /* on a separate line
#37: FILE: tests/test-qemu-opts.c:747:
+        { "a,b,,help", false /* BUG */, true, true },

total: 0 errors, 1 warnings, 50 lines checked

Patch 1/8 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
2/8 Checking commit 8bb805dd3730 (qemu-options: Factor out get_opt_name_value() helper)
3/8 Checking commit b3630a346906 (qemu-option: Fix sloppy recognition of "id=..." after ", , ")
4/8 Checking commit 5c1b2db0b7ad (qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily())
5/8 Checking commit 6845c29bee11 (qemu-option: Fix has_help_option()'s sloppy parsing)
6/8 Checking commit b7fcaaeeeb6f (test-qemu-opts: Simplify test_has_help_option() after bug fix)
7/8 Checking commit b164930d4c8d (qemu-img: Factor out accumulate_options() helper)
8/8 Checking commit 505f5f389855 (qemu-option: Move is_valid_option_list() to qemu-img.c and rewrite)
ERROR: suspect code indent for conditional statements (4, 4)
#61: FILE: qemu-img.c:243:
+    for (i = len; i > 0 && optarg[i - 1] == ','; i--) {
+    }

total: 1 errors, 0 warnings, 67 lines checked

Patch 8/8 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200409153041.17576-1-armbru@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH for-5.1 0/8] qemu-option: Fix corner cases and clean up

[Eric Blake]

On 4/9/20 12:09 PM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/20200409153041.17576-1-armbru@redhat.com/
> 
> 
> 
> Hi,
> 
> This series seems to have some coding style problems. See output below for
> more information:
> 

> === OUTPUT BEGIN ===
> 1/8 Checking commit 2e003109273b (tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt())
> WARNING: Block comments use a leading /* on a separate line
> #37: FILE: tests/test-qemu-opts.c:747:
> +        { "a,b,,help", false /* BUG */, true, true },

Annoying, but I don't mind ignoring it (especially since we fix the bug 
later in the series).

> 
> total: 0 errors, 1 warnings, 50 lines checked
> 
> Patch 1/8 has style problems, please review.  If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> 2/8 Checking commit 8bb805dd3730 (qemu-options: Factor out get_opt_name_value() helper)
> 3/8 Checking commit b3630a346906 (qemu-option: Fix sloppy recognition of "id=..." after ", , ")
> 4/8 Checking commit 5c1b2db0b7ad (qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily())
> 5/8 Checking commit 6845c29bee11 (qemu-option: Fix has_help_option()'s sloppy parsing)
> 6/8 Checking commit b7fcaaeeeb6f (test-qemu-opts: Simplify test_has_help_option() after bug fix)
> 7/8 Checking commit b164930d4c8d (qemu-img: Factor out accumulate_options() helper)
> 8/8 Checking commit 505f5f389855 (qemu-option: Move is_valid_option_list() to qemu-img.c and rewrite)
> ERROR: suspect code indent for conditional statements (4, 4)
> #61: FILE: qemu-img.c:243:
> +    for (i = len; i > 0 && optarg[i - 1] == ','; i--) {
> +    }

False positive. Maybe you can shut up the checker by:
     for (...) {
         /* nothing further to do */
     }

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

Re: [PATCH for-5.1 0/8] qemu-option: Fix corner cases and clean up

[Markus Armbruster]

Eric Blake <eblake@redhat.com> writes:

> On 4/9/20 12:09 PM, no-reply@patchew.org wrote:
>> Patchew URL: https://patchew.org/QEMU/20200409153041.17576-1-armbru@redhat.com/
>>
>>
>>
>> Hi,
>>
>> This series seems to have some coding style problems. See output below for
>> more information:
>>
>
>> === OUTPUT BEGIN ===
>> 1/8 Checking commit 2e003109273b (tests-qemu-opts: Cover has_help_option(), qemu_opt_has_help_opt())
>> WARNING: Block comments use a leading /* on a separate line
>> #37: FILE: tests/test-qemu-opts.c:747:
>> +        { "a,b,,help", false /* BUG */, true, true },
>
> Annoying, but I don't mind ignoring it (especially since we fix the
> bug later in the series).
>
>>
>> total: 0 errors, 1 warnings, 50 lines checked
>>
>> Patch 1/8 has style problems, please review.  If any of these errors
>> are false positives report them to the maintainer, see
>> CHECKPATCH in MAINTAINERS.
>> 2/8 Checking commit 8bb805dd3730 (qemu-options: Factor out get_opt_name_value() helper)
>> 3/8 Checking commit b3630a346906 (qemu-option: Fix sloppy recognition of "id=..." after ", , ")
>> 4/8 Checking commit 5c1b2db0b7ad (qemu-option: Avoid has_help_option() in qemu_opts_parse_noisily())
>> 5/8 Checking commit 6845c29bee11 (qemu-option: Fix has_help_option()'s sloppy parsing)
>> 6/8 Checking commit b7fcaaeeeb6f (test-qemu-opts: Simplify test_has_help_option() after bug fix)
>> 7/8 Checking commit b164930d4c8d (qemu-img: Factor out accumulate_options() helper)
>> 8/8 Checking commit 505f5f389855 (qemu-option: Move is_valid_option_list() to qemu-img.c and rewrite)
>> ERROR: suspect code indent for conditional statements (4, 4)
>> #61: FILE: qemu-img.c:243:
>> +    for (i = len; i > 0 && optarg[i - 1] == ','; i--) {
>> +    }
>
> False positive. Maybe you can shut up the checker by:
>     for (...) {
>         /* nothing further to do */
>     }

I don't like to add code to work around checkpatch bugs.  Rare
checkpatch bugs can be ignored, non-rare ones should be fixed.