Re: [PATCH v3 14/20] migration: Use output visitor in info command

[Fabiano Rosas <farosas@suse.de>]

Markus Armbruster <armbru@redhat.com> writes:

> "Dr. David Alan Gilbert" <dave@treblig.org> writes:
>
>> * Peter Xu (peterx@redhat.com) wrote:
>>> On Thu, Sep 17, 2026 at 01:53:22PM +0200, Markus Armbruster wrote:
>>> > Fabiano Rosas <farosas@suse.de> writes:
>>> > 
>>> > > The hmp_info_migrate_parameters function currently open-codes the
>>> > > mon_printf calls for each migration parameter. As with the set command
>>> > > in the last patch, this should not be necessary as the QAPI
>>> > > infrastructure already has generated code that takes type and struct
>>> > > member names into account, including converting _ from C into the '-'
>>> > > character as part of parameter names strings.
>>> > >
>>> > > The current code is also quite painful to rebase if a series has been
>>> > > carried for a long time while parameters have been added in master.
>>> > >
>>> > > Replace all of this with a conversion from MigrationParameters to
>>> > > QDict using an output visitor and a loop over the QDict that prints
>>> > > per-QAPI-type formatted strings.
>>> > >
>>> > > Modelled after block/qapi.c:dump_qobject, but with some changes to
>>> > > keep the migration command output formatting.
>>> > >
>>> > > Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>> > 
>>> > Old output of info migrate_parameters for me:
>>> > 
>>> >     announce-initial: 50
>>> >     announce-max: 550
>>> >     announce-rounds: 5
>>> >     announce-step: 100
>>> >     throttle-trigger-threshold: 50
>>> >     cpu-throttle-initial: 20
>>> >     cpu-throttle-increment: 10
>>> >     cpu-throttle-tailslow: off
>>> >     max-cpu-throttle: 99
>>> >     tls-creds: 
>>> >     tls-hostname: 
>>> >     tls-authz: 
>>> >     max-bandwidth: 134217728
>>> >     avail-switchover-bandwidth: 0
>>> >     max-postcopy-bandwidth: 0
>>> >     downtime-limit: 300
>>> >     x-checkpoint-delay: 20000
>>> >     multifd-channels: 2
>>> >     multifd-compression: none
>>> >     zero-page-detection: multifd
>>> >     xbzrle-cache-size: 67108864
>>> >     x-vcpu-dirty-limit-period: 1000
>>> >     vcpu-dirty-limit: 1
>>> >     mode: normal
>>> >     direct-io: off
>>> >     x-rdma-chunk-size: 1048576
>>> >     cpr-exec-command:
>>> > 
>>> > New output:
>>> > 
>>> >     cpu-throttle-tailslow: off
>>> >     xbzrle-cache-size: 67108864
>>> >     cpu-throttle-initial: 20
>>> >     announce-max: 550
>>> >     direct-io: off
>>> >     avail-switchover-bandwidth: 0
>>> >     local: off
>>> >     zero-page-detection: multifd
>>> >     multifd-qatzip-level: 1
>>> >     x-rdma-chunk-size: 1048576
>>> >     multifd-channels: 2
>>> >     mode: normal
>>> >     multifd-zstd-level: 1
>>> >     announce-initial: 50
>>> >     downtime-limit: 300
>>> >     tls-authz: 
>>> >     cpr-exec-command:
>>> > 
>>> >     vcpu-dirty-limit: 1
>>> >     multifd-compression: none
>>> >     announce-rounds: 5
>>> >     announce-step: 100
>>> >     tls-creds: 
>>> >     x-vcpu-dirty-limit-period: 1000
>>> >     multifd-zlib-level: 1
>>> >     max-cpu-throttle: 99
>>> >     max-postcopy-bandwidth: 0
>>> >     tls-hostname: 
>>> >     throttle-trigger-threshold: 50
>>> >     max-bandwidth: 134217728
>>> >     x-checkpoint-delay: 20000
>>> >     cpu-throttle-increment: 10
>>> > 
>>> > New output has an unwanted blank line after "cpr-exec-command:".
>>> > 
>>> > New output adds @local, @multifd-qatzip-level, @multifd-zlib-level,
>>> > @multifd-zstd-level.  Commit message should point that out.
>>> > 
>>> > Order of parameters changes, and not for the better.  Hmm.
>>> 
>>> Good point.. it might start to be awkward to users looking for tls-*,
>>> multifd-*, announce-* or cpu-throttle*, etc. after this change, expecting
>>> them grouped.
>>> 
>>> Maybe we can sort them by name with inserting top level to a QList first
>>> from the QDict.
>>
>> Are there any filter visitors in qemu's visitor system - e.g. something that
>> sorts?
>
> No.  Visitors walk members in source order.  Source order is good as
> long as we care to make it good.  But it doesn't help us here, because
> we use the visitor only to convert to QObject, then walk that.  This
> loses order.
>
> If QDict iterated in insertion order, we'd be fine.  Python dict do.
> Could be a fun little project for someone with time to spare.
>
> The other solution is to iterate over sorted QDict keys, as Peter says.
>

Thanks, I didn't notice the output changed so drastically. The readline
completion suggestions are sorted, so I just assumed the parameters were
sorted too. I'll fix it up.