Markus Armbruster <arm...@redhat.com> writes:
> Markus Armbruster <arm...@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) {
> [...]
