[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/6] qapi: Simplify full_name_nth() in qobject-input-visitor
From: |
Markus Armbruster |
Subject: |
Re: [PATCH 3/6] qapi: Simplify full_name_nth() in qobject-input-visitor |
Date: |
Thu, 28 Jan 2021 08:43:25 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
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 :)