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

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3511 bytes --]

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 <armbru@redhat.com>
> ---

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

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:

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

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.

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]