Re: [PATCH V1 1/4] migration: mode parameter

[Steven Sistare <steven.sistare@oracle.com>]

On 10/20/2023 5:29 AM, Juan Quintela wrote:
> Steve Sistare <steven.sistare@oracle.com> wrote:
>> Create a mode migration parameter that can be used to select alternate
>> migration algorithms.  The default mode is normal, representing the
>> current migration algorithm, and does not need to be explicitly set.
>>
>> No functional change until a new mode is added, except that the mode is
>> shown by the 'info migrate' command.
>>
>> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> 
> [... qdev definition ...]
> 
> Looks legit, but I am not an expert here.

Nor I, but I copied a similar definition line for line, see
  qdev_prop_blockdev_on_error
  DEFINE_PROP_BLOCKDEV_ON_ERROR

However, I now see I am missing:
  QEMU_BUILD_BUG_ON(sizeof(MigMode) != sizeof(int));

I can ask Daniel Berrange to review this part if you prefer.

>> @@ -867,6 +870,13 @@ uint64_t migrate_xbzrle_cache_size(void)
>>      return s->parameters.xbzrle_cache_size;
>>  }
>>  
>> +MigMode migrate_mode(void)
>> +{
>> +    MigrationState *s = migrate_get_current();
>> +
>> +    return s->parameters.mode;
>> +}
>> +
> 
> Inside parameters, I try to get the functions sorted by name.  the same
> for options.h

Sure, will do.

>> diff --git a/qapi/migration.json b/qapi/migration.json
>> index db3df12..184fb78 100644
>> --- a/qapi/migration.json
>> +++ b/qapi/migration.json
>> @@ -616,6 +616,15 @@
>>              { 'name': 'zstd', 'if': 'CONFIG_ZSTD' } ] }
>>  
>>  ##
>> +# @MigMode:
>> +#
>> +# @normal: the original form of migration. (since 8.2)
>> +#
>> +##
>> +{ 'enum': 'MigMode',
>> +  'data': [ 'normal' ] }
>> +
>> +##
> 
> Here you only have normal, but in qdev you also have exec.

Good eye.  I will remove exec from .description in this patch, and add
cpr-reboot to it in patch 4.

>>  # @BitmapMigrationBitmapAliasTransform:
>>  #
>>  # @persistent: If present, the bitmap will be made persistent or
>> @@ -675,6 +684,9 @@
>>  #
>>  # Migration parameters enumeration
>>  #
>> +# @mode: Migration mode. See description in @MigMode. Default is 'normal'.
>> +#        (Since 8.2)
>> +#
> 
> You normally put comments and values at the end of the comments and
> sections. Your sshould be last.

Do you mean, add the mode parameter at the end of the existing parameters, 
after vcpu-dirty-limit?

> Feel free to use a single line in the json.  More than one value for
> line make it a bit more compress, but makes changes more complicated.

Like this?

{ 'enum': 'MigrationParameter',
  'data': ['announce-initial', 'announce-max',
          ...
          'vcpu-dirty-limit',
          'mode'] }

- Steve