Am 27.01.2021 um 14:56 hat Markus Armbruster geschrieben:
> Kevin Wolf <kw...@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 <kw...@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
