Kevin Wolf <kw...@redhat.com> writes:
> Commit ff32bb53 tried to get minimal struct support into the string
> output visitor by just making it return "<omitted>". Unfortunately, it
> forgot that the caller will still make more visitor calls for the
> content of the struct.
>
> If the struct is contained in a list, such as IOThreadVirtQueueMapping,
> in the better case its fields show up as separate list entries. In the
> worse case, it contains another list, and the string output visitor
> doesn't support nested lists and asserts that this doesn't happen.
What it actually asserts, or rather tries to assert is this constraint
from visit_end_list()'s contract:
* @list must match what was passed to the paired visit_start_list().
Since it's not prepared for nested lists, it actually asserts "match
what was passed the last visit_start_list() for this visitor", which is
correct only as long as there is no nesting.
I'm not sure whether this is relevant enough to justify tweaking your
commit message.
> doesn't support nested lists and asserts that this doesn't happen. So as
> soon as the optional "vqs" field in IOThreadVirtQueueMapping is
> specified, we get a crash.
>
> This can be reproduced with the following command line:
>
> echo "info qtree" | ./qemu-system-x86_64 \
> -object iothread,id=t0 \
> -blockdev null-co,node-name=disk \
> -device '{"driver": "virtio-blk-pci", "drive": "disk",
> "iothread-vq-mapping": [{"iothread": "t0", "vqs": [0]}]}' \
> -monitor stdio
Appreciate the easy reproducer.
> Fix the problem by counting the nesting level of structs and ignoring
> any visitor calls for values (apart from start/end_struct) while we're
> not on the top level.
>
> Fixes: ff32bb53476539d352653f4ed56372dced73a388
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2069
> Reported-by: Aihua Liang <ali...@redhat.com>
> Signed-off-by: Kevin Wolf <kw...@redhat.com>
> ---
> qapi/string-output-visitor.c | 46 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 46 insertions(+)
>
> diff --git a/qapi/string-output-visitor.c b/qapi/string-output-visitor.c
> index f0c1dea89e..5115536b15 100644
> --- a/qapi/string-output-visitor.c
> +++ b/qapi/string-output-visitor.c
> @@ -65,6 +65,7 @@ struct StringOutputVisitor
> } range_start, range_end;
> GList *ranges;
> void *list; /* Only needed for sanity checking the caller */
> + unsigned int struct_nesting;
> };
>
> static StringOutputVisitor *to_sov(Visitor *v)
> @@ -144,6 +145,10 @@ static bool print_type_int64(Visitor *v, const char
> *name, int64_t *obj,
> StringOutputVisitor *sov = to_sov(v);
> GList *l;
>
> + if (sov->struct_nesting) {
> + return true;
> + }
> +
> switch (sov->list_mode) {
> case LM_NONE:
> string_output_append(sov, *obj);
> @@ -231,6 +236,10 @@ static bool print_type_size(Visitor *v, const char
> *name, uint64_t *obj,
> uint64_t val;
> char *out, *psize;
>
> + if (sov->struct_nesting) {
> + return true;
> + }
> +
> if (!sov->human) {
> out = g_strdup_printf("%"PRIu64, *obj);
> string_output_set(sov, out);
> @@ -250,6 +259,11 @@ static bool print_type_bool(Visitor *v, const char
> *name, bool *obj,
> Error **errp)
> {
> StringOutputVisitor *sov = to_sov(v);
> +
> + if (sov->struct_nesting) {
> + return true;
> + }
> +
> string_output_set(sov, g_strdup(*obj ? "true" : "false"));
> return true;
> }
> @@ -260,6 +274,10 @@ static bool print_type_str(Visitor *v, const char *name,
> char **obj,
> StringOutputVisitor *sov = to_sov(v);
> char *out;
>
> + if (sov->struct_nesting) {
> + return true;
> + }
> +
> if (sov->human) {
> out = *obj ? g_strdup_printf("\"%s\"", *obj) : g_strdup("<null>");
> } else {
> @@ -273,6 +291,11 @@ static bool print_type_number(Visitor *v, const char
> *name, double *obj,
> Error **errp)
> {
> StringOutputVisitor *sov = to_sov(v);
> +
> + if (sov->struct_nesting) {
> + return true;
> + }
> +
> string_output_set(sov, g_strdup_printf("%.17g", *obj));
> return true;
> }
> @@ -283,6 +306,10 @@ static bool print_type_null(Visitor *v, const char
> *name, QNull **obj,
> StringOutputVisitor *sov = to_sov(v);
> char *out;
>
> + if (sov->struct_nesting) {
> + return true;
> + }
> +
> if (sov->human) {
> out = g_strdup("<null>");
> } else {
> @@ -295,6 +322,9 @@ static bool print_type_null(Visitor *v, const char *name,
> QNull **obj,
> static bool start_struct(Visitor *v, const char *name, void **obj,
> size_t size, Error **errp)
> {
> + StringOutputVisitor *sov = to_sov(v);
> +
> + sov->struct_nesting++;
> return true;
> }
>
> @@ -302,6 +332,10 @@ static void end_struct(Visitor *v, void **obj)
> {
> StringOutputVisitor *sov = to_sov(v);
>
> + if (--sov->struct_nesting) {
> + return;
> + }
> +
> /* TODO actually print struct fields */
> string_output_set(sov, g_strdup("<omitted>"));
> }
> @@ -312,6 +346,10 @@ start_list(Visitor *v, const char *name, GenericList
> **list, size_t size,
> {
> StringOutputVisitor *sov = to_sov(v);
>
> + if (sov->struct_nesting) {
> + return true;
> + }
> +
> /* we can't traverse a list in a list */
> assert(sov->list_mode == LM_NONE);
> /* We don't support visits without a list */
> @@ -329,6 +367,10 @@ static GenericList *next_list(Visitor *v, GenericList
> *tail, size_t size)
> StringOutputVisitor *sov = to_sov(v);
> GenericList *ret = tail->next;
>
> + if (sov->struct_nesting) {
> + return ret;
> + }
> +
> if (ret && !ret->next) {
> sov->list_mode = LM_END;
> }
> @@ -339,6 +381,10 @@ static void end_list(Visitor *v, void **obj)
> {
> StringOutputVisitor *sov = to_sov(v);
>
> + if (sov->struct_nesting) {
> + return;
> + }
> +
> assert(sov->list == obj);
> assert(sov->list_mode == LM_STARTED ||
> sov->list_mode == LM_END ||
@struct_nesting is what its name suggests: the *struct* nesting level.
The patch's idea is to turn all methods into no-ops inside a struct. To
make that work, start_struct() and end_struct() aren't actually no-ops;
they track the nesting level.
What about nested lists that are not inside any struct?
Ceterum censeo: the struct visitors need to go. But I'm *not* asking
you to do that now.
