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