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.
>
> 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 <arm...@redhat.com>
But please take care of fixing the comment in string-output-visitor.h.
