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

From: Markus Armbruster <armbru@redhat.com>
To: Kevin Wolf <kwolf@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: Thu, 28 Jan 2021 08:43:25 +0100	[thread overview]
Message-ID: <87mtwtfqw2.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20210127214242.GG6090@merkur.fritz.box> (Kevin Wolf's message of "Wed, 27 Jan 2021 22:42:42 +0100")

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.  There's one in hw/ppc/spapr_drc.c:

        case FDT_PROP: {
            int i;
            prop = fdt_get_property_by_offset(fdt, fdt_offset, &prop_len);
            name = fdt_string(fdt, fdt32_to_cpu(prop->nameoff));
            if (!visit_start_list(v, name, NULL, 0, errp)) {
                return;
            }
            for (i = 0; i < prop_len; i++) {
                if (!visit_type_uint8(v, NULL, (uint8_t *)&prop->data[i],
                                      errp)) {
                    return;
                }
            }
            ok = visit_check_list(v, errp);
            visit_end_list(v, NULL);
            if (!ok) {
                return;
            }
            break;
        }

This visits @prop_len list elements.

If there are fewer, visit_type_uint8() should fail.  With the
QObjectInputVisitor, qobject_input_try_get_object() returns null to
qobject_input_get_object(), which then fails.

If there are more, visit_check_list() should fail.  With the
QObjectInputVisitor, it's the failure you challenged.

Now, this virtual walk is in a QOM getter, which should only ever be
with an output visitor.  Can't fail.  Only input visitors can.

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

See above.

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

We'd have to restrict virtual walks: if input visitor, then must exhaust
list input.  Ugh :)