Fabiano Rosas <faro...@suse.de> writes:
> Markus Armbruster <arm...@redhat.com> writes:
>
> Markus, sorry for the delay here. I had vacations and holidays, plus a
> pile of patches to review.
No problem. Hope you enjoyed your time off!
>> Fabiano Rosas <faro...@suse.de> writes:
>>
>>> Add a new migration structure to consolidate the capabilities and
>>> parameters. This structure will be used in place of the s->parameters
>>> and s->capabilities data structures in the next few patches.
>>>
>>> The QAPI migration types now look like this:
>>>
>>> /* options previously known as parameters */
>>
>> Configuration previously known as parameters less the TLS stuff.
>>
>>> { 'struct': 'MigrationConfigBase',
>>> 'data': {
>>> <parameters>
>>> } }
>>>
>>>
>>> /* for compat with query-migrate-parameters */
>>> { 'struct': 'MigrationParameters',
>>> 'base': 'MigrationConfigBase',
>>> 'data': {
>>> <TLS options in 'str' format>
>>> } }
>>>
>>> /* for compat with migrate-set-parameters */
>>> { 'struct': 'MigrateSetParameters',
>>> 'base': 'MigrationConfigBase',
>>> 'data': {
>>> <TLS options in 'StrOrNull' format>
>>> } }
>>
>> Yes, this is the state since PATCH 05.
>>
>>> /* to replace MigrationParameters in the MigrationState */
>>> { 'struct': 'MigrationConfig',
>>> 'base': 'MigrationConfigBase'
>>> 'data': {
>>> <TLS options in 'str' format>
>>> } }
>>
>> This is new in this patch.
>>
>> Your description doesn't cover optionalness. Here's my understanding:
>>
>> * MigrationSetParameters has optional members, because
>> migrate-set-parameters needs that.
>>
>
> Yes.
>
>> * MigrationParameters would ideally have these members non-optional,
>> because query-migrate-parameters wants that.
>>
>
> Yes.
>
>> * But to enable sharing via common base type MigrationConfigBase, we
>> accept unwanted optional in MigrationParameters and thus
>> query-migrate-parameters.
>>
>
> Yes.
>
>> * This doesn't apply to the non-shared members of MigrationParameters,
>> so you made them non-optional. These are @tls-creds, @tls-hostname,
>> @tls-authz.
>>
>
> Yes.
>
>> * But in MigrationConfig they're optional again, because "empty string
>> means absent" is silly; we want "NULL means absent".
>>
>
> Yes. But mostly because MigrationConfig will become the type for the new
> '*config' argument to migrate/migrate_incoming (patches 12 & 13) and we
> want to keep all members optional. Otherwise the user would have to pass
> all ~50 migration options in every migrate command, which is bad IMO.
Got it.
>> Correct?
>>
>> Up to here, this enables cleanup of some "empty string means absent"
>> silliness in later patches.
>>
>> The remainder is about unifying capabilities into parameters. I'd split
>> the patch (but I'm a compulsive patch splitter).
>>
>>> The above keeps the query/set-parameters commands stable. For the
>>> capabilities as well as the options added in the future, we have a
>>> choice of where to put them:
>>>
>>> 1) In MigrationConfigBase, this means that the existing
>>> query/set-parameters commands will be updated to deal with
>>> capabilities/new options.
>>>
>>> { 'struct': 'MigrationConfigBase',
>>> 'data': {
>>> <parameters>
>>> <capabilities>
>>> <new opts>
>>> } }
>>>
>>> { 'struct': 'MigrationConfig',
>>> 'base': 'MigrationConfigBase'
>>> 'data': {
>>> <TLS options in 'str' format>
>>> } }
>>>
>>> 2) In MigrationConfig, this means that the existing commands will be
>>> frozen in time.
>>>
>>> { 'struct': 'MigrationConfigBase',
>>> 'data': {
>>> <parameters>
>>> } }
>>>
>>> { 'struct': 'MigrationConfig',
>>> 'base': 'MigrationConfigBase'
>>> 'data': {
>>> <TLS options in 'str' format>
>>> <capabilities>
>>> <new opts>
>>> } }
>>>
>>> For now, I've chosen the option 1, all capabilities and new options go
>>> into MigrationConfigBase. This gives the option to keep the existing
>>> commands for as long as we'd like.
>>
>> Perhaps this would be slightly easier to digest for the reader if you
>> talked just about capabilities at first. Once that's understood,
>> mention we have the same choice for new configuration bits.
>>
>
> Ok, I'll reorganize, along with the other comments you've made.
>
>>> Note that the query/set capabilities commands will have to go, we can
>>> treat parameters as generic configuration options, but capabilities
>>> are just too different.
>>
>> I think the argument is that migration capabilities are a pointless
>> interface complication. One mechanism (parameters) is better than two
>> (parameters and capabilities).
>>
>
> Yes, that's the main point indeed.
Perhaps you can make this point more prominently.
>>> Signed-off-by: Fabiano Rosas <faro...@suse.de>
[...]
