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.
  */
-static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name,
-                                 int n)
+static const char *full_name_so(QObjectInputVisitor *qiv, const char *name,
+                                StackObject *so)
 {
-    StackObject *so;
     char buf[32];
 
     if (qiv->errname) {
@@ -109,10 +105,13 @@ static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name,
         qiv->errname = g_string_new("");
     }
 
-    QSLIST_FOREACH(so , &qiv->stack, node) {
-        if (n) {
-            n--;
-        } else if (qobject_type(so->obj) == QTYPE_QDICT) {
+    if (!name && so) {
+        name = so->name;
+        so = QSLIST_NEXT(so, node);
+    }
+
+    for (; so; so = QSLIST_NEXT(so, node)) {
+        if (qobject_type(so->obj) == QTYPE_QDICT) {
             g_string_prepend(qiv->errname, name ?: "<anonymous>");
             g_string_prepend_c(qiv->errname, '.');
         } else {
@@ -123,7 +122,6 @@ static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name,
         }
         name = so->name;
     }
-    assert(!n);
 
     if (name) {
         g_string_prepend(qiv->errname, name);
@@ -138,7 +136,9 @@ static const char *full_name_nth(QObjectInputVisitor *qiv, const char *name,
 
 static const char *full_name(QObjectInputVisitor *qiv, const char *name)
 {
-    return full_name_nth(qiv, name, 0);
+    StackObject *tos = QSLIST_FIRST(&qiv->stack);
+
+    return full_name_so(qiv, name, tos);
 }
 
 static QObject *qobject_input_try_get_object(QObjectInputVisitor *qiv,
@@ -473,7 +473,7 @@ static bool qobject_input_check_list(Visitor *v, Error **errp)
 
     if (tos->entry) {
         error_setg(errp, "Only %u list elements expected in %s",
-                   tos->index + 1, full_name_nth(qiv, NULL, 1));
+                   tos->index + 1, full_name_so(qiv, NULL, tos));
         return false;
     }
     return true;
-- 
2.28.0

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

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.

[...]

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

> 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

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 :)

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