Re: [Qemu-devel] [PATCH v14 18/19] qapi: Simplify semantics of visit_next_list()

[Markus Armbruster <armbru@redhat.com>]

Markus Armbruster <armbru@redhat.com> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
> [...]
>>> diff --git a/qapi/string-input-visitor.c b/qapi/string-input-visitor.c
>>> index 797973a..77dd1a7 100644
>>> --- a/qapi/string-input-visitor.c
>>> +++ b/qapi/string-input-visitor.c
>>> @@ -25,8 +25,6 @@ struct StringInputVisitor
>>>  {
>>>      Visitor visitor;
>>>
>>> -    bool head;
>>> -
>>>      GList *ranges;
>>>      GList *cur_range;
>>>      int64_t cur;
>>> @@ -125,11 +123,21 @@ error:
>>>  }
>>>
>>>  static void
>>> -start_list(Visitor *v, const char *name, Error **errp)
>>> +start_list(Visitor *v, const char *name, GenericList **list, size_t size,
>>> +           Error **errp)
>>>  {
>>>      StringInputVisitor *siv = to_siv(v);
>>> +    Error *err = NULL;
>>>
>>> -    parse_str(siv, errp);
>>> +    /* We don't support visits without a GenericList, yet */
>>
>> without a list
>>
>> Do we plan to support them?  If not, scratch "yet".
>>
>>> +    assert(list);
>>> +
>>> +    parse_str(siv, &err);
>>> +    if (err) {
>>> +        *list = NULL;
>>> +        error_propagate(errp, err);
>>> +        return;
>>> +    }
>
> parse_str() never sets an error, and therefore your new error check is
> dead.  Just as well, because it would be wrong.
>
> parse_str() parses a complete string into a non-empty list of uint64_t
> ranges.  On success, it sets siv->ranges to this list.  On error, it
> sets it to null.  It could also set an error then, but it doesn't.
>
> If it did, then what would start_list() do with it?  Reporting it would
> be wrong, because the list members need not be integers.
>
> If they aren't, the speculative parse_str()'s failure will be ignored.
>
> If they are, parse_type_int64() will call parse_str() again, then use
> siv->ranges.
>
> If the first parse_str() succeeds, the second will do nothing, and we'll
> use the first one's siv->ranges.  Works.
>
> If the first parse_str() fails, the second will fail as well, because
> its input is the same.  We'll use the second one's failure.  Works.

No, it doesn't: failure gets interpreted as empty list.  I'll post my
test case separately.

> When used outside list context, parse_type_int64() will call parse_str()
> for the first time, and use its result.  Works.
>
> Note that opts-visitor does it differently: opts_start_list() doesn't
> parse numbers, opts_type_int64() and opts_type_uint64() do.
>
> Further note the latent bug in parse_type_int64(): we first call
> parse_str(siv, errp), and goto error if it fails, where we promptly
> error_setg(errp, ...).  If parse_str() set an error, the error_setg()
> would fail error_setv()'s assertion.
>
> Please drop parse_str()'s unused errp parameter, and add a comment to
> start_list() explaining the speculative call to parse_str() there.

Insufficient, doesn't fix the bug.  After parse_str(), we need to be
able to distinguish empty list from error.  Moving the error_set() into
parse_str() could work.  Returning succes/failure and dropping the errp
parameter could also work.

> Alternatively, change the string visitor to work like the opts visitor.
>
>>>
>>>      siv->cur_range = g_list_first(siv->ranges);
>>>      if (siv->cur_range) {
> [...]