Kevin Wolf <kw...@redhat.com> writes: > 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).
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 :)