Re: [Qemu-devel] [PATCH 1/3] migration: Add multifd-compress parameter

[Juan Quintela]

"Dr. David Alan Gilbert" <address@hidden> wrote:
> * Juan Quintela (address@hidden) wrote:
>> Signed-off-by: Juan Quintela <address@hidden>

>> +    case MIGRATION_PARAMETER_MULTIFD_COMPRESS:
>> +        p->has_multifd_compress = true;
>> +        visit_type_enum(v, param, &compress_type,
>> +                        &MultifdCompress_lookup, &err);
>> +        if (err) {
>> +            break;
>> +        }
>> +        if (compress_type < 0 || compress_type > MULTIFD_COMPRESS__MAX) {
>
> I think that needs to be >= rather than >
> (Actually it surprises me visit_type_enum doesn't turn those cases into
> errors)

You are right.

>> @@ -81,6 +81,7 @@
>>  /* The delay time (in ms) between two COLO checkpoints */
>>  #define DEFAULT_MIGRATE_X_CHECKPOINT_DELAY (200 * 100)
>>  #define DEFAULT_MIGRATE_MULTIFD_CHANNELS 2
>> +#define DEFAULT_MIGRATE_MULTIFD_COMPRESS false
>
> Shouldn't that be an enum value?

Fixed

>> +
>> +##
>> +# @MultifdCompress:
>> +#
>> +# An enumeration of multifd compression.
>> +#
>> +# @none: no compression.
>> +#
>> +# @zlib: Compress using zlib.
>> +#
>> +# Since: 3.1
>
> 4.

fixed.

>>  # @max-cpu-throttle: maximum cpu throttle percentage.
>>  #                    Defaults to 99. (Since 3.1)
>> +#
>> +# @multifd-compress: What compression method to use.
>> +#                    Defaults to none. (Since 4.0)
>> +#
>
> I think that should be 'Which' rather than 'What'.

You are the native speaker.

>>  # Since: 2.4
>>  ##
>>  { 'enum': 'MigrationParameter',
>> @@ -568,7 +572,7 @@
>>             'downtime-limit', 'x-checkpoint-delay', 'block-incremental',
>>             'multifd-channels',
>>             'xbzrle-cache-size', 'max-postcopy-bandwidth',
>> -           'max-cpu-throttle' ] }
>> +           'max-cpu-throttle', 'multifd-compress' ] }
>>  
>>  ##
>>  # @MigrateSetParameters:
>> @@ -644,7 +648,10 @@
>>  #                     (Since 3.0)
>>  #
>>  # @max-cpu-throttle: maximum cpu throttle percentage.
>> -#                    The default value is 99. (Since 3.1)
>> +#                    The default value is 99. (Since 4.0)
>
> That change is wrong?

Fixed.

Thanks, Juan.