On Sat, Aug 05, 2023 at 10:12:00AM +0200, Markus Armbruster wrote:
> Peter Xu <pet...@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 <pet...@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 <arm...@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 <arm...@redhat.com>
> Reviewed-by: Daniel P. Berrange <berra...@redhat.com>
> Reviewed-by: Eric Blake <ebl...@redhat.com>
I see. Personally I think as long as the interface is 100% clear I'll be
all fine (say, no possible misuse of ""). But keeping StrOrNull may
definitely be cleaner I guess.
>
> > 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.
I don't think it's a must. As Dan raised, we can convert str -> StrOrNull
for MigrationParameters. I assume it won't affect query-migrate-parameters
anyway OTOH.
I assume it means there's nothing yet obvious that we overlooked on the
whole idea. Let me propose the formal patchset early next week. It'll be
mostly the patch I attached but just add those extra logics for StrOrNull,
so the diffstat might be less attractive but hopefully still good enough to
be accepted.
Thanks,
--
Peter Xu
