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

[Kevin Wolf]

Am 28.01.2021 um 08:43 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > 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).
> 
> See, I can write impenetrable comments with the best of them!
> 
> The spot that talks about @name is in visitor.h:
> 
>  * The @name parameter of visit_type_FOO() describes the relation
>  * between this QAPI value and its parent container.  When visiting
>  * the root of a tree, @name is ignored; when visiting a member of an
>  * object, @name is the key associated with the value; when visiting a
>  * member of a list, @name is NULL; and when visiting the member of an
>  * alternate, @name should equal the name used for visiting the
>  * alternate.
> 
> Many contracts in the same file refer back to it like this:
> 
>  * @name expresses the relationship of this object to its parent
>  * container; see the general description of @name above.
> 
> The contract here doesn't.
> 
> >> 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.
> 
> Yes, the generated visitors always exhaust the list.  But virtual walks
> needn't.

Ah. Okay, I'll add the bool parameter then.

Kevin