Re: [PATCH 01/21] migration: Normalize tls arguments

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

On Tue, Jul 01, 2025 at 09:08:13AM +0200, Markus Armbruster wrote:
> Fabiano Rosas <farosas@suse.de> writes:
> 
> > Markus Armbruster <armbru@redhat.com> writes:
> >
> >> Fabiano Rosas <farosas@suse.de> writes:
> >>
> >>> Markus Armbruster <armbru@redhat.com> writes:
> >>>
> >>>> Fabiano Rosas <farosas@suse.de> writes:
> >>>>
> >>>>> Markus Armbruster <armbru@redhat.com> writes:
> >>>>>
> >>>>>> Fabiano Rosas <farosas@suse.de> writes:
> 
> [...]
> 
> >> There more than one way to skin this cat.  I like to keep state
> >> normalized.
> >>
> >> State is an optional StrOrNull.  Possible values:
> >>
> >> * NULL
> >>
> >> * QNull, i.e. non-NULL, ->type is QTYPE_QNULL
> >>
> >> * Empty string, i.e. non-NULL, ->type is QTYPE_QSTRING, ->u.s is ""
> >>
> >> * Non-empty string, i.e. non-NULL, -> type is QTYPE_QSTRING, ->u.s is
> >>   not "" (and cannot be NULL)
> >>
> >> As far as I understand, we have just two cases semantically:
> >>
> >> * Set, value is a non-empty string (empty makes no sense)
> >>
> >> * Unset
> >>
> >> I'd normalize the state to "either NULL, or (non-empty) string".
> >>
> >
> > This is what I wanted to do (in the next version), but it results in
> > more complex and less readable code:
> 
> [...]
> 
> > If we instead normalize to "either non-empty string or empty string"
> > then:
> 
> [...]
> 
> > The query methods get simpler because s->parameters already contains
> > data in the format they expect, we can normalize earlier in [2], which
> > means data is always in the same format throughout
> > qmp_migrate_set_parameters() and lastly, we already have the getter
> > methods [1] which can expose "abc"|NULL to the rest of the code anyway.
> 
> I'd like the possible states to be clearly visible, and suggest to guard
> them with assertions.  Details, such as how exactly the states are
> encoded, are up to you.  You're in a better position to judge them than
> I am.
> 
> >> When writing state, we need to normalize.
> >>
> >> When reading state, we can rely on it being normalized.  Asserting it is
> >> seems prudent, and should help readers.
> >>
> >
> > My main concern is that reading can rely on it being normalized, but the
> > query methods cannot, so they need to do an "extra conversion", which
> > from the reader's POV, will look nonsensical. It's not as simple as
> > using a ternary because the StrOrNull object needs to be allocated.
> 
> [...]
> 
> >>> There are two external interfaces actually.
> >>>
> >>> -global migration.some_compat_option=on (stored in MigrationState):
> >>>
> >>> seems intentional and I believe we'd lose the ability to get out of some
> >>> tricky situations if we ditched it.
> >>>
> >>> -global migation.some_random_option=on (stored in MigrationParameters):
> >>>
> >>> has become a debugging *feature*, which I personally don't use, but
> >>> others do. And worse: we don't know if anyone uses it in production.
> >>
> >> Accidental external interface.
> >>
> >>> We also arbitrarily put x- in front of options for some reason. There is
> >>> an argument to drop those because x- is scary and no one should be using
> >>> them.
> >>
> >> We pretty much ditched the x- convention in the QAPI schema.
> >> docs/devel/qapi-code-gen.rst:
> >>
> >>     Names beginning with ``x-`` used to signify "experimental".  This
> >>     convention has been replaced by special feature "unstable".
> >>
> >> Goes back to
> >>
> >> commit a3c45b3e62962f99338716b1347cfb0d427cea44
> >> Author: Markus Armbruster <armbru@redhat.com>
> >> Date:   Thu Oct 28 12:25:12 2021 +0200
> >>
> >>     qapi: New special feature flag "unstable"
> >>     
> >>     By convention, names starting with "x-" are experimental.  The parts
> >>     of external interfaces so named may be withdrawn or changed
> >>     incompatibly in future releases.
> >
> > This allows dropping about half of the parameters we expose. Deprecate
> > the other half, move the remaining legitimate compat options into
> > MigrationParameters, (which can be set by migrate-set-parameters) and
> > maybe we can remove the TYPE_DEVICE from MigrationState anytime this
> > decade.
> 
> I'd love to get rid of the pseudo-device.
> 
> > Moving all qdev properties to their own TYPE_DEVICE object and putting
> > it under --enable-debug is also an idea.
> >
> > I'm willing to do the work if we ever reach a consensus about this.
> 
> I'd like migration to work more like other long-running tasks: pass the
> entire configuration with the command starting it, provide commands and
> events to manage the task while it runs.
> 
> This is advice, not a demand.  I'm not going to block change the
> migration maintainers want.  I may ask you to do the QAPI schema part in
> certain ways, but that's detail.

This should be doable as IIUC at the end of this series, the QMP
'migrate-incoming' command parameters can express all the required
data for accepting an incoming migration. As such, that parameter
struct could be exposed on the CLI with the CLI json syntax to
accept complex args in a way that wasn't possible historically
with -incoming.

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 :|