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

[Juan Quintela]

Steven Sistare <steven.sistare@oracle.com> wrote:
> 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.

I reviewed it, but I agree that looks legit to me O:-)

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

Thanks.

>>>  # @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'] }

Exactwly.  Same for the comments at the end of the list.

> - Steve