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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> On 4/9/20 10:30 AM, Markus Armbruster wrote:
>> The next commits will put it to use.
>>
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>>   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 <address@hidden>

Thanks!