| Message ID | 20240109181717.42493-1-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | string-output-visitor: Fix (pseudo) struct handling | expand |
Kevin Wolf <kwolf@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 <aliang@redhat.com> > Signed-off-by: Kevin Wolf <kwolf@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.
Am 11.01.2024 um 12:45 hat Markus Armbruster geschrieben: > Kevin Wolf <kwolf@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. Ah, yes, I see the assertion in end_list() that you mean. That one looks like it would indeed fail if we didn't already crash on the nested start_list(): /* we can't traverse a list in a list */ assert(sov->list_mode == LM_NONE); > > 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 <aliang@redhat.com> > > Signed-off-by: Kevin Wolf <kwolf@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? They remain forbidden, we don't currently have a use case for them. Nesting inside of structs is easy to "support" because we don't actually print any of the values inside of them anyway. If you wanted to support list nesting where the value is actually meant to be printed, you'd first need to define what the output should look like and then implement that. I consider that a separate problem from what this patch fixes. > Ceterum censeo: the struct visitors need to go. But I'm *not* asking > you to do that now. I assume you mean string visitors. Kevin
On Tue, Jan 09, 2024 at 07:17:17PM +0100, Kevin Wolf wrote: > 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. 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 > > 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 <aliang@redhat.com> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > qapi/string-output-visitor.c | 46 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) Thanks for getting to this before I could: Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > 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 || > -- > 2.43.0 >
Kevin Wolf <kwolf@redhat.com> writes: > Am 11.01.2024 um 12:45 hat Markus Armbruster geschrieben: >> Kevin Wolf <kwolf@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. > > Ah, yes, I see the assertion in end_list() that you mean. That one looks > like it would indeed fail if we didn't already crash on the nested > start_list(): > > /* we can't traverse a list in a list */ > assert(sov->list_mode == LM_NONE); True. >> > 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 <aliang@redhat.com> >> > Signed-off-by: Kevin Wolf <kwolf@redhat.com> [...] >> @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? > > They remain forbidden, we don't currently have a use case for them. > > Nesting inside of structs is easy to "support" because we don't actually > print any of the values inside of them anyway. If you wanted to support > list nesting where the value is actually meant to be printed, you'd > first need to define what the output should look like and then implement > that. I consider that a separate problem from what this patch fixes. Fair enough. Mention it in the commit message? Perhaps "Lists nested within lists remain unimplemented, as we don't currently have a use case for them." >> Ceterum censeo: the struct visitors need to go. But I'm *not* asking >> you to do that now. > > I assume you mean string visitors. Yes. I blame dabbrev-expand :) Reviewed-by: Markus Armbruster <armbru@redhat.com>
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 ||
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. 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 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 <aliang@redhat.com> Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- qapi/string-output-visitor.c | 46 ++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)