Re: [PATCH 02/21] migration: Remove MigrateSetParameters

[Markus Armbruster <armbru@redhat.com>]

Fabiano Rosas <farosas@suse.de> writes:

> Markus Armbruster <armbru@redhat.com> writes:
>
>> Fabiano Rosas <farosas@suse.de> writes:
>>
>>> Now that the TLS options have been made the same between
>>> migrate-set-parameters and query-migrate-parameters, a single type can
>>> be used. Remove MigrateSetParameters.
>>>
>>> The TLS options documentation from MigrationParameters were replaced
>>> with the ones from MigrateSetParameters which was more complete.
>>>
>>> I'm choosing to somewhat ignore any ambiguity between "query" and
>>> "set" because other options' docs are already ambiguous in that
>>> regard.
>>>
>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>> ---
>>>  migration/migration-hmp-cmds.c |   4 +-
>>>  migration/options.c            |   6 +-
>>>  qapi/migration.json            | 221 +++------------------------------
>>>  3 files changed, 20 insertions(+), 211 deletions(-)
>>>
>>> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
>>> index bc8179c582..aacffdc532 100644
>>> --- a/migration/migration-hmp-cmds.c
>>> +++ b/migration/migration-hmp-cmds.c
>>> @@ -490,7 +490,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>>>      const char *param = qdict_get_str(qdict, "parameter");
>>>      const char *valuestr = qdict_get_str(qdict, "value");
>>>      Visitor *v = string_input_visitor_new(valuestr);
>>> -    MigrateSetParameters *p = g_new0(MigrateSetParameters, 1);
>>> +    MigrationParameters *p = g_new0(MigrationParameters, 1);
>>>      uint64_t valuebw = 0;
>>>      uint64_t cache_size;
>>>      Error *err = NULL;
>>> @@ -656,7 +656,7 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
>>>      qmp_migrate_set_parameters(p, &err);
>>>  
>>>   cleanup:
>>> -    qapi_free_MigrateSetParameters(p);
>>> +    qapi_free_MigrationParameters(p);
>>>      visit_free(v);
>>>      hmp_handle_error(mon, err);
>>>  }
>>> diff --git a/migration/options.c b/migration/options.c
>>> index 45a95dc6da..e49d584a99 100644
>>> --- a/migration/options.c
>>> +++ b/migration/options.c
>>> @@ -1227,7 +1227,7 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
>>>      return true;
>>>  }
>>>  
>>> -static void migrate_params_test_apply(MigrateSetParameters *params,
>>> +static void migrate_params_test_apply(MigrationParameters *params,
>>>                                        MigrationParameters *dest)
>>>  {
>>>      *dest = migrate_get_current()->parameters;
>>> @@ -1350,7 +1350,7 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>>>      }
>>>  }
>>>  
>>> -static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>>> +static void migrate_params_apply(MigrationParameters *params, Error **errp)
>>>  {
>>>      MigrationState *s = migrate_get_current();
>>>  
>>> @@ -1479,7 +1479,7 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>>>      }
>>>  }
>>>  
>>> -void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
>>> +void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
>>>  {
>>>      MigrationParameters tmp;
>>>  
>>> diff --git a/qapi/migration.json b/qapi/migration.json
>>> index fa42d94810..080968993a 100644
>>> --- a/qapi/migration.json
>>> +++ b/qapi/migration.json
>>> @@ -914,202 +914,6 @@
>>>             'zero-page-detection',
>>>             'direct-io'] }
>>>  
>>> -##
>>> -# @MigrateSetParameters:
>>
>> Only use is argument type of migrate-set-parameters.  You're replacing
>> it by MigrationParameters there.  Let's compare the deleted docs to
>> their replacement.  I'll quote replacement docs exactly where they
>> differ.
>>
>>    # @MigrationParameters:
>>    #
>>    # Migration parameters. Optional members are optional when used with
>>    # an input command, otherwise mandatory.
>>
>> Figuring out which commands are input commands is left to the reader.
>> Why not simply "optional with migrate-set-parameters"?
>>
>
> Future patches include migrate and migrate-incoming. I can enumerate
> them if that's better.

Not necessary if you move the note to commands as discussed below.

>> However, it doesn't end there.  The paragraph creates a problem with
>> John Snow's "inliner", which I hope to merge later this year.  Let me
>> explain.
>>
>> Generated command documentation normally looks like this:
>>
>>     Command migrate-set-capabilities (Since: 1.2)
>>
>>        Enable/Disable the following migration capabilities (like xbzrle)
>>
>>        Arguments:
>>           * **capabilities** ("[""MigrationCapabilityStatus""]") -- json
>>             array of capability modifications to make
>>
>> Except when we happen to use a named type for the arguments.  This
>> should be an implementation detail, and it is, except for generated
>> documentation, which looks like
>>
>>     Command migrate-set-parameters (Since: 2.4)
>>
>>        Set various migration parameters.
>>
>>        Arguments:
>>           * The members of "MigrationParameters".
>>
>> The arguments are hidden behind a link.  The "inliner" will show the
>> them normally *always*, for better usability.  It will not, however,
>> inline the introductory paragraph above.  I can explain why if
>> necessary.
>>
>> To compensate for the loss of that paragraph, we'll have to add suitable
>> text to migrate-set-parameters's doc comment.
>>
>> I think we could just as well do that *now*: scratch the paragraph here,
>> add a suitable paragraph there.
>>
>
> Ok, no worries.

[...]