Kevin Wolf <kw...@redhat.com> writes:
> Am 11.01.2024 um 12:45 hat Markus Armbruster geschrieben:
>> 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.
>
> 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 <ali...@redhat.com>
>> > Signed-off-by: Kevin Wolf <kw...@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 <arm...@redhat.com>
