Re: [PATCH for-8.2 2/2] string-output-visitor: Support lists for non-integer types

From: Kevin Wolf <kwolf@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: qemu-devel@nongnu.org, thuth@redhat.com, philmd@linaro.org,
	peter.maydell@linaro.org, Stefan Hajnoczi <stefanha@redhat.com>
Subject: Re: [PATCH for-8.2 2/2] string-output-visitor: Support lists for non-integer types
Date: Thu, 30 Nov 2023 15:00:54 +0100	[thread overview]
Message-ID: <ZWiVliL7igJIv3-j@redhat.com> (raw)
In-Reply-To: <87bkbb9yht.fsf@pond.sub.org>

Am 30.11.2023 um 14:11 hat Markus Armbruster geschrieben:
> I understand Stefan already took this patch.  I'm looking at it anyway,
> because experience has taught me to be very afraid of the string
> visitors.
> 
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > With the introduction of list-based array properties in qdev, the string
> > output visitor has to deal with lists of non-integer elements now ('info
> > qtree' prints all properties with the string output visitor).
> >
> > Currently there is no explicit support for such lists, and the resulting
> > output is only the last element because string_output_set() always
> > replaces the output with the latest value.
> 
> Yes.
> 
> The string visitors were created just for QOM's object_property_parse()
> and object_property_print().  At the time, QOM properties were limited
> to scalars, and the new visitors implemented just enough of the visitor
> API to be usable with scalars.  This was a Really Bad Idea(tm).
> 
> Commit a020f9809cf (qapi: add string-based visitors)
>    and b2cd7dee86f (qom: add generic string parsing/printing).
> 
> When we wanted a QOM property for "set of NUMA node number", we extended
> the visitors to support integer lists.  With fancy range syntax.  Except
> for 'size'.  This was another Really Bad Idea(tm).
> 
> Commit 659268ffbff (qapi: make string input visitor parse int list)
>    and 69e255635d0 (qapi: make string output visitor parse int list)
> 
> All the visitor stuff was scandalously under-documented (that's not even
> a bad idea, just a Really Bad Habit(tm)).  When we added documentation
> much later, we missed the lack of support for lists with elements other
> than integers.  We later fixed that oversight for the input visitor
> only.
> 
> Commit adfb264c9ed (qapi: Document visitor interfaces, add assertions)
>    and c9fba9de89d (qapi: Rewrite string-input-visitor's integer and list parsing)
> 
> Your patch extends the string output visitor to support lists of
> arbitrary scalars.
> 
> >                                            Instead of replacing the old
> > value, append comma separated values in list context.
> >
> > The difference can be observed in 'info qtree' with a 'rocker' device
> > that has a 'ports' list with more than one element.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  qapi/string-output-visitor.c | 24 ++++++++++++++++++++----
> >  1 file changed, 20 insertions(+), 4 deletions(-)
> 
> Missing: update of string-output-visitor.h's comment
> 
>  * The string output visitor does not implement support for visiting
>  * QAPI structs, alternates, null, or arbitrary QTypes.  It also
>  * requires a non-null list argument to visit_start_list().
> 
> It is wrong before the patch: most lists do not work.  After the patch,
> only lists of scalars work.  Document that, please.  Maybe:
> 
>  * The string output visitor does not implement support for visiting
>  * QAPI structs, alternates, null, or arbitrary QTypes.  Only flat lists
>  * are supported.  It also requires a non-null list argument to
>  * visit_start_list().
> 
> Stolen from string-input-visitor.h's comment.
> 
> Could instead use "Only lists of scalars are supported."
> 
> Follow-up patch would be fine.

I guess I'm lucky that the comment I missed already failed to point out
the limitations before, so at least I didn't make anything worse!

Adding a sentence makes sense to me. I find "list of scalars" easier to
understand than "flat lists" (in particular, I would have considered a
list of structs to be flat), so I'd prefer that wording.

> >
> > diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> > index 71ddc92b7b..c0cb72dbe4 100644
> > --- a/qapi/string-output-visitor.c
> > +++ b/qapi/string-output-visitor.c
> > @@ -74,11 +74,27 @@ static StringOutputVisitor *to_sov(Visitor *v)
> >  
> >  static void string_output_set(StringOutputVisitor *sov, char *string)
> >  {
> > -    if (sov->string) {
> > -        g_string_free(sov->string, true);
> > +    switch (sov->list_mode) {
> > +    case LM_STARTED:
> > +        sov->list_mode = LM_IN_PROGRESS;
> > +        /* fall through */
> > +    case LM_NONE:
> > +        if (sov->string) {
> > +            g_string_free(sov->string, true);
> > +        }
> > +        sov->string = g_string_new(string);
> > +        g_free(string);
> > +        break;
> > +
> > +    case LM_IN_PROGRESS:
> > +    case LM_END:
> > +        g_string_append(sov->string, ", ");
> > +        g_string_append(sov->string, string);
> > +        break;
> > +
> > +    default:
> > +        abort();
> >      }
> > -    sov->string = g_string_new(string);
> > -    g_free(string);
> >  }
> >  
> >  static void string_output_append(StringOutputVisitor *sov, int64_t a)
> 
> The ->list_mode state machine was designed for parsing integer lists
> with fancy range syntax.  Let me try to figure out how it works.
> 
> Initial state is LM_NONE.
> 
> On start_list():
>     LM_NONE -> LM_STARTED.
> 
> On end_list():
>     any -> LM_NONE. 
> 
> On next_list():
>     any -> LM_END.
> 
> On print_type_int64():
>     LM_STARTED -> LM_IN_PROGRESS
>     LM_IN_PROGRESS -> LM_IN_PROGRESS
>     LM_END -> LM_END
> 
> The two states LM_SIGNED_INTERVAL and LM_UNSIGNED_INTERVAL have never
> been used.  Copy-pasta from opts-visitor.c.
> 
> Only real walks call next_list(), virtual walks do not.  In a real walk,
> print_type_int64() executes its LM_END case for non-first elements.  In
> a virtual walk, it executes its LM_IN_PROGRESS case.  This can't be
> right.
> 
> What a load of confused crap.

I won't try to argue that the string visitor isn't a load of confused
crap, but I don't see how LM_END is non-first elements? It only gets set
in next_list() for the last element.

The more interesting point I wasn't aware of is that virtual walks don't
need to call next_list(). If we can fix the string visitor, doing a
virtual walk might have made more sense for the array property getter
than construction a temporary real list?

Or can't you mix virtual and real with the same visitor? Because I
assume the callers of property getters are doing a real walk.

Kevin