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

[Markus Armbruster <armbru@redhat.com>]

Peter Xu <peterx@redhat.com> writes:

> On Fri, Aug 04, 2023 at 05:48:49PM +0100, Daniel P. Berrangé wrote:
>> On Fri, Aug 04, 2023 at 12:46:18PM -0400, Peter Xu wrote:
>> > On Fri, Aug 04, 2023 at 05:29:19PM +0100, Daniel P. Berrangé wrote:
>> > > 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.
>> > 
>> > Yes, that should also work and even without worrying on nulls.  I just took
>> > a random one replacing the other.
>> > 
>> > > 
>> > > > 
>> > > > 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 ?
>> > 
>> > I think it just got overlooked when introducing tls-authz to not have added
>> > that special code in qmp_migrate_set_parameters(), the other two are fine.
>> 
>> Oh right yes, pre-existing bug.
>
> So do we really care about "null" in any form over "" (empty str) here for
> tls-* parameters?

In my opinion, the use of "" was a design mistake.  Here's my argument:

commit 01fa55982692fb51a16049b63b571651a1053989
Author: Markus Armbruster <armbru@redhat.com>
Date:   Tue Jul 18 14:42:04 2017 +0200

    migration: Use JSON null instead of "" to reset parameter to default
    
    migrate-set-parameters sets migration parameters according to is
    arguments like this:
    
    * Present means "set the parameter to this value"
    
    * Absent means "leave the parameter unchanged"
    
    * Except for parameters tls_creds and tls_hostname, "" means "reset
      the parameter to its default value
    
    The first two are perfectly normal: presence of the parameter makes
    the command do something.
    
    The third one overloads the parameter with a second meaning.  The
    overloading is *implicit*, i.e. it's not visible in the types.  Works
    here, because "" is neither a valid TLS credentials ID, nor a valid
    host name.
    
    Pressing argument values the schema accepts, but are semantically
    invalid, into service to mean "reset to default" is not general, as
    suitable invalid values need not exist.  I also find it ugly.
    
    To clean this up, we could add a separate flag argument to ask for
    "reset to default", or add a distinct value to @tls_creds and
    @tls_hostname.  This commit implements the latter: add JSON null to
    the values of @tls_creds and @tls_hostname, deprecate "".
    
    Because we're so close to the 2.10 freeze, implement it in the
    stupidest way possible: have qmp_migrate_set_parameters() rewrite null
    to "" before anything else can see the null.  The proper way to do it
    would be rewriting "" to null, but that requires fixing up code to
    work with null.  Add TODO comments for that.
    
    Signed-off-by: Markus Armbruster <armbru@redhat.com>
    Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
    Reviewed-by: Eric Blake <eblake@redhat.com>

> To fix this tls-authz bug we can add one more QTYPE_QNULL to QTYPE_QSTRING
> convertion, but I'd rather just use "str" for all tls* fields and remove
> the other two instead, if "null" is not important to anyone.

"Important" sounds too much like absolutes :)

I think we have a tradeoff here.  If perpetuating the unclean and ugly
use of "" is what it takes to de-triplicate migration parameters, we may
decide to accept that.

> In all cases, I've appended with the two patches I'm currently testing
> with.  It should also fix the tls-authz crash over 'null' by just rejecting
> that.  But I'm open to anything - the patch (more than RFC) is more for
> reference of whether we can drop the two objects in qapi/migration.
>
> Thanks,