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