[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v14 18/19] qapi: Simplify semantics of visit_next_list()
Date: Fri, 22 Apr 2016 10:46:49 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)

Markus Armbruster <address@hidden> 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

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) {

reply via email to

[Prev in Thread] Current Thread [Next in Thread]