qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v4 25/28] qapi: Make input visitors detect unvis


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH v4 25/28] qapi: Make input visitors detect unvisited list tails
Date: Fri, 03 Mar 2017 20:50:12 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux)

Eric Blake <address@hidden> writes:

> On 03/03/2017 06:32 AM, Markus Armbruster wrote:
>> Fix the design flaw demonstrated in the previous commit: new method
>> check_list() lets input visitors report that unvisited input remains
>> for a list, exactly like check_struct() lets them report that
>> unvisited input remains for a struct or union.
>> 
>> Implement the method for the qobject input visitor (straightforward),
>> and the string input visitor (less so, due to the magic list syntax
>> there).  The opts visitor's list magic is even more impenetrable, and
>> all I can do there today is a stub with a FIXME comment.  No worse
>> than before.
>> 
>> Signed-off-by: Markus Armbruster <address@hidden>
>> ---
>
> Didn't I already review this one?
>
> Ah, there's my R-b:
> https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg07614.html

Oops!  My apologies...

> But since it disappeared again, I had another look.
>
>> +++ b/qapi/qobject-input-visitor.c
>> @@ -51,7 +51,8 @@ static QObjectInputVisitor *to_qiv(Visitor *v)
>>      return container_of(v, QObjectInputVisitor, visitor);
>>  }
>>  
>> -static const char *full_name(QObjectInputVisitor *qiv, const char *name)
>> +static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name,
>> +                                 int n)
>>  {
>>      StackObject *so;
>>      char buf[32];
>> @@ -63,8 +64,10 @@ static const char *full_name(QObjectInputVisitor *qiv, 
>> const char *name)
>>      }
>>  
>>      QSLIST_FOREACH(so , &qiv->stack, node) {
>> -        if (qobject_type(so->obj) == QTYPE_QDICT) {
>> -            g_string_prepend(qiv->errname, name);
>> +        if (n) {
>> +            n--;
>> +        } else if (qobject_type(so->obj) == QTYPE_QDICT) {
>> +            g_string_prepend(qiv->errname, name ?: "<anonymous>");
>>              g_string_prepend_c(qiv->errname, '.');
>>          } else {
>>              snprintf(buf, sizeof(buf), "[%u]", so->index);
>> @@ -72,18 +75,24 @@ static const char *full_name(QObjectInputVisitor *qiv, 
>> const char *name)
>>          }
>>          name = so->name;
>>      }
>> +    assert(!n);
>
> If I'm reading this right, your use of n-- in the loop followed by the
> post-condition is to assert that QSLIST_FOREACH() iterated n times, but
> lets see what callers pass for n:

At least @n times.

>>  
>>      if (name) {
>>          g_string_prepend(qiv->errname, name);
>>      } else if (qiv->errname->str[0] == '.') {
>>          g_string_erase(qiv->errname, 0, 1);
>> -    } else {
>> +    } else if (!qiv->errname->str[0]) {
>>          return "<anonymous>";
>>      }
>>  
>>      return qiv->errname->str;
>>  }
>>  
>> +static const char *full_name(QObjectInputVisitor *qiv, const char *name)
>> +{
>> +    return full_name_nth(qiv, name, 0);
>> +}
>
> One caller passes 0,
>
>
>> +static void qobject_input_check_list(Visitor *v, Error **errp)
>> +{
>> +    QObjectInputVisitor *qiv = to_qiv(v);
>> +    StackObject *tos = QSLIST_FIRST(&qiv->stack);
>> +
>> +    assert(tos && tos->obj && qobject_type(tos->obj) == QTYPE_QLIST);
>> +
>> +    if (tos->entry) {
>> +        error_setg(errp, "Only %u list elements expected in %s",
>> +                   tos->index + 1, full_name_nth(qiv, NULL, 1));
>
> the other passes 1.  No other calls.  Did we really need an integer,
> where we use n--, or would a bool have done as well?

Since I actually use only 0 and 1, a bool would do, but would it make
the code simpler?

> At any rate, since I've already reviewed it once, you can add R-b, but
> we may want a followup to make it less confusing.

Would renaming the function to full_name_but_n() help?



reply via email to

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