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

[Markus Armbruster]

Kevin Wolf <kwolf@redhat.com> writes:

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

Right!

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

Agree.

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

You're right; I missed that.

> The more interesting point I wasn't aware of is that virtual walks don't
> need to call next_list().

visitor.h:

 * After visit_start_list() succeeds, the caller may visit its members
 * one after the other.  A real visit (where @list is non-NULL) uses
 * visit_next_list() for traversing the linked list, while a virtual
 * visit (where @list is NULL) uses other means.  For each list
 * element, call the appropriate visit_type_FOO() with name set to
 * NULL and obj set to the address of the value member of the list
 * element.  Finally, visit_end_list() needs to be called with the
 * same @list to clean up, even if intermediate visits fail.  See the
 * examples above.

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

visitor.h:

 * A visitor should be used for exactly one top-level visit_type_FOO()
 * or virtual walk; if that is successful, the caller can optionally
 * call visit_complete() (useful only for output visits, but safe to
 * call on all visits).  Then, regardless of success or failure, the
 * user should call visit_free() to clean up resources.  It is okay to
 * free the visitor without completing the visit, if some other error
 * is detected in the meantime.

The callers of property getters I can see look more or less like this:

    v = FOO_output_visitor_new(..., &ret);
    if (object_property_get(obj, name, v, errp)) {
        visit_complete(v, &ret);
    }
    visit_free(v);

Such callers don't walk anything themselves.

I think a virtual walk should be okay.