Re: [PATCH] string-output-visitor: Fix (pseudo) struct handling

[Markus Armbruster <armbru@redhat.com>]

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>