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

[Eric Blake]

On 04/22/2016 05:35 AM, Markus Armbruster wrote:

>>>>
>>>>  static void
>>>> -start_list(Visitor *v, const char *name, Error **errp)
>>>> +start_list(Visitor *v, const char *name, GenericList **list, size_t size,

>>>> +
>>>> +    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.

parse_str() is only ever called for start_list and parse_type_int64 - so
the real question is do we ever support the string input visitor on
anything other than an integral list.  Per the comments I'm adding
earlier in the series:

 * The string input visitor does not implement support for visiting
 * QAPI structs, alternates, null, or arbitrary QTypes.

so it sounds like the only lists it supports ARE integer lists.

>>
>> 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.

I like the approach used in opts-visitor (start_list should only check
if a list is present, but save the parsing for when the items are
actually consumed off the list).  But opts-visitor also handles structs,
unlike the string visitor, which forces the separation (at start_list,
we don't know what the list element will be, unlike the string visitor
where we know the list element is integral).

>>
>> 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.

I'm playing with these ideas, but will get the bug fixed (along with
your testsuite addition) as a prerequisite to the list refactoring (to
make sure that the refactored list still passes the fixed test).

> 
>> Alternatively, change the string visitor to work like the opts visitor.

Trickier, but may be my only option if the other approaches don't work.
 Thanks again for spotting yet another ugly corner case worth fixing
(this series seems to have been a never-ending source of them...)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature