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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, Aug 04, 2023 at 12:01:54PM -0400, Peter Xu wrote:
> 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.

I meant the other way around, such we would be using 'StrOrNull'
in all scenarios.

> 
> 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).

It shouldn't be crashing,  because qmp_migrate_set_parameters()
is turning 'null' into  "", which means the assert ought to
never fire. Did you have a local modiification that caused
this crash perhaps ?

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|