Re: [PATCH 3/6] qapi: Simplify full_name_nth() in qobject-input-visitor

From: Kevin Wolf <kwolf@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: jsnow@redhat.com, qemu-devel@nongnu.org
Subject: Re: [PATCH 3/6] qapi: Simplify full_name_nth() in qobject-input-visitor
Date: Wed, 27 Jan 2021 22:42:42 +0100	[thread overview]
Message-ID: <20210127214242.GG6090@merkur.fritz.box> (raw)
In-Reply-To: <87ft2mpjon.fsf@dusky.pond.sub.org>

Am 27.01.2021 um 14:56 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > Instead of counting how many elements from the top of the stack we need
> > to ignore until we find the thing we're interested in, we can just
> > directly pass the StackObject pointer because all callers already know
> > it.
> >
> > We only need a different way now to tell if we want to know the name of
> > something contained in the given StackObject or of the StackObject
> > itself. Passing name = NULL is the obvious way to request the latter.
> >
> > This simplifies the interface and makes it easier to use in cases where
> > we have the StackObject, but don't know how many steps down the stack it
> > is.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qapi/qobject-input-visitor.c | 38 ++++++++++++++++++------------------
> >  1 file changed, 19 insertions(+), 19 deletions(-)
> >
> > diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
> > index a00ac32682..1415561828 100644
> > --- a/qapi/qobject-input-visitor.c
> > +++ b/qapi/qobject-input-visitor.c
> > @@ -87,20 +87,16 @@ static QObjectInputVisitor *to_qiv(Visitor *v)
> >  }
> >  
> >  /*
> > - * Find the full name of something @qiv is currently visiting.
> > - * @qiv is visiting something named @name in the stack of containers
> > - * @qiv->stack.
> > - * If @n is zero, return its full name.
> > - * If @n is positive, return the full name of the @n-th container
> > - * counting from the top.  The stack of containers must have at least
> > - * @n elements.
> > - * The returned string is valid until the next full_name_nth(@v) or
> > - * destruction of @v.
> > + * Find the full name of something named @name in @so which @qiv is
> > + * currently visiting.  If @name is NULL, find the full name of @so
> > + * itself.
> > + *
> > + * The returned string is valid until the next full_name_so(@qiv) or
> > + * destruction of @qiv.
> 
> How can this distinguish between a list and its member?
> 
> Before the patch:
> 
> * list member: n = 0, name = NULL
> * list: n = 1, name = NULL

Oh. These two lines were more helpful than the whole function comment
before this patch (which doesn't talk about name = NULL at all).

> Afterwards?
> 
> Checking... yes, regression.  Test case:
> 
>     {"execute": "blockdev-add", "arguments": {"driver": "file", "node-name": "blk0", "filename": "tmp.img"}}
>     {"return": {}}
>     {"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "node-name": "blk1", "image": "blk0", "take-child-perms": [0]}}
>     {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'take-child-perms', expected: string"}}
> 
> The second command's reply changes from
> 
>     {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'take-child-perms[0]', expected: string"}}
> 
> to
> 
>     {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'take-child-perms', expected: string"}}
> 
> The idea of using @so instead of @n may be salvagable.

I can always add a bool parameter that tells (independently from @name)
whether we want the name of a member or of the container.

Though do we really need the name of the container anywhere? The n = 1
case exists in qobject_input_check_list(), but is this a case that can
fail? The pattern how lists are intended to be visited seems to be
calling visit_next_list() until it returns NULL.

The only place where this pattern isn't followed and visit_next_list()
is called outside such a loop, so that we can actually run into the
error in qobject_input_check_list(), is a test case specifically for
this error path.

So should we just declare not visiting all list elements a programming
error and assert instead of constructing an error message that users
will never see?

Kevin