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 <address@hidden>
> ---
>   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