Re: [PATCH for-8.2 v2 1/2] qapi/migration: Deduplicate migration parameter field comments

[Peter Xu]

On Fri, Aug 04, 2023 at 02:59:07PM +0100, Daniel P. Berrangé wrote:
> On Fri, Aug 04, 2023 at 02:28:05PM +0200, Markus Armbruster wrote:
> > Peter Xu <peterx@redhat.com> writes:
> > 
> > > We used to have three objects that have always the same list of parameters
> > 
> > We have!
> > 
> > > and comments are always duplicated:
> > >
> > >   - @MigrationParameter
> > >   - @MigrationParameters
> > >   - @MigrateSetParameters
> > >
> > > Before we can deduplicate the code, it's fairly straightforward to
> > > deduplicate the comments first, so for each time we add a new migration
> > > parameter we don't need to copy the same paragraphs three times.
> > 
> > De-duplicating the code would be nice, but we haven't done so in years,
> > which suggests it's hard enough not to be worth the trouble.
> 
> The "MigrationParameter" enumeration isn't actually used in
> QMP at all.
> 
> It is only used in HMP for hmp_migrate_set_parameter and
> hmp_info_migrate_parameters. So it is questionable documenting
> that enum in the QMP reference docs at all.
> 
> 1c1
> < { 'struct': 'MigrationParameters',
> ---
> > { 'struct': 'MigrateSetParameters',
> 14,16c14,16
> <             '*tls-creds': 'str',
> <             '*tls-hostname': 'str',
> <             '*tls-authz': 'str',
> ---
> >             '*tls-creds': 'StrOrNull',
> >             '*tls-hostname': 'StrOrNull',
> >             '*tls-authz': 'StrOrNull',
> 
> Is it not valid to use StrOrNull in both cases and thus
> delete the duplication here ?

I tested removing MigrateSetParameters by replacing it with
MigrationParameters and it looks all fine here... I manually tested qmp/hmp
on set/query parameters, and qtests are all happy.

The only thing I see that may affect it is we used to logically allow
taking things like '"tls-authz": null' in the json input, but now we won't
allow that because we'll be asking for a string type only.

Since we have query-qmp-schema I suppose we're all fine, because logically
the mgmt app (libvirt?) will still query that to understand the protocol,
so now we'll have (response of query-qmp-schema):

        {
            "arg-type": "144",
            "meta-type": "command",
            "name": "migrate-set-parameters",
            "ret-type": "0"
        },

Where 144 can start to point to MigrationParameters, rather than
MigrateSetParameters.

Ok, then what if the mgmt app doesn't care and just used "null" in tls-*
fields when setting?  Funnily I tried it and actually anything that does
migrate-set-parameters with a "null" passed over to tls-* fields will
already crash qemu...

./migration/options.c:1333: migrate_params_apply: Assertion 
`params->tls_authz->type == QTYPE_QSTRING' failed.

#0  0x00007f72f4b2a844 in __pthread_kill_implementation () at /lib64/libc.so.6
#1  0x00007f72f4ad9abe in raise () at /lib64/libc.so.6
#2  0x00007f72f4ac287f in abort () at /lib64/libc.so.6
#3  0x00007f72f4ac279b in _nl_load_domain.cold () at /lib64/libc.so.6
#4  0x00007f72f4ad2147 in  () at /lib64/libc.so.6
#5  0x00005573308740e6 in migrate_params_apply (params=0x7ffc74fd09d0, 
errp=0x7ffc74fd0998) at ../migration/options.c:1333
#6  0x0000557330874591 in qmp_migrate_set_parameters (params=0x7ffc74fd09d0, 
errp=0x7ffc74fd0998) at ../migration/options.c:1433
#7  0x0000557330cb9132 in qmp_marshal_migrate_set_parameters 
(args=0x7f72e00036d0, ret=0x7f72f133cd98, errp=0x7f72f133cd90) at 
qapi/qapi-commands-migration.c:214
#8  0x0000557330d07fab in do_qmp_dispatch_bh (opaque=0x7f72f133ce30) at 
../qapi/qmp-dispatch.c:128
#9  0x0000557330d33bbb in aio_bh_call (bh=0x5573337d7920) at ../util/async.c:169
#10 0x0000557330d33cd8 in aio_bh_poll (ctx=0x55733356e7d0) at 
../util/async.c:216
#11 0x0000557330d17a19 in aio_dispatch (ctx=0x55733356e7d0) at 
../util/aio-posix.c:423
#12 0x0000557330d34117 in aio_ctx_dispatch (source=0x55733356e7d0, 
callback=0x0, user_data=0x0) at ../util/async.c:358
#13 0x00007f72f5a8848c in g_main_context_dispatch () at /lib64/libglib-2.0.so.0
#14 0x0000557330d358d4 in glib_pollfds_poll () at ../util/main-loop.c:290
#15 0x0000557330d35951 in os_host_main_loop_wait (timeout=0) at 
../util/main-loop.c:313
#16 0x0000557330d35a5f in main_loop_wait (nonblocking=0) at 
../util/main-loop.c:592
#17 0x000055733083aee0 in qemu_main_loop () at ../softmmu/runstate.c:732
#18 0x0000557330b0921b in qemu_default_main () at ../softmmu/main.c:37
#19 0x0000557330b09251 in main (argc=35, argv=0x7ffc74fd0ec8) at 
../softmmu/main.c:48

Then I suppose it means all mgmt apps are not using "null" anyway, and it
makes more sense to me to just remove MigrateSetParameters (by replacing it
with MigrationParameters).

Then if we can also replace MigrationParameter enum with an internal enum
(alongside with a _str[] array for it) it seems we're all fine to dedup the
3 objects into 1 in qapi schema.

Thanks,

-- 
Peter Xu