| Message ID | 20231121173416.346610-3-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qdev array property fixes | expand |
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. > > 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. Your string_output_set() treats LM_IN_PROGRESS and LM_END the same. This could be right ;) It behaves as before in state LM_NONE: overwrite sov->string. Good. In state LM_STARTED, it overwrites sov->string. Good. In states, LM_IN_PROGRESS and LM_END, it appends to sov->string. Good. It is used for all scalar visitors except print_type_int64() and print_type_uint64(). Therefore, it makes all remaining scalars work in lists. Good. Objects and nested lists still don't work. If "info qdev" runs into a such a property, it'll crash. Not your patch's fault. I loathe the string visitors. Reviewed-by: Markus Armbruster <armbru@redhat.com> But please take care of fixing the comment in string-output-visitor.h.
On Thu, 30 Nov 2023 at 08:12, Markus Armbruster <armbru@redhat.com> wrote: > > 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. Hi Markus, I should have waited for your review. Sorry! Stefan
Stefan Hajnoczi <stefanha@gmail.com> writes: > On Thu, 30 Nov 2023 at 08:12, Markus Armbruster <armbru@redhat.com> wrote: >> >> 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. > > Hi Markus, > I should have waited for your review. Sorry! No reason to be sorry! It's a regression, and we're at rc2 already.
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
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.
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)
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. 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(-)