Re: [PATCH v1 08/17] migration/colo: Use ram_block_discard_set_broken()

[David Hildenbrand]

On 15.05.20 15:58, Dr. David Alan Gilbert wrote:
> * David Hildenbrand (address@hidden) wrote:
>> COLO will copy all memory in a RAM block, mark discarding of RAM broken.
>>
>> Cc: "Michael S. Tsirkin" <address@hidden>
>> Cc: Hailiang Zhang <address@hidden>
>> Cc: Juan Quintela <address@hidden>
>> Cc: "Dr. David Alan Gilbert" <address@hidden>
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
>>  include/migration/colo.h |  2 +-
>>  migration/migration.c    |  8 +++++++-
>>  migration/savevm.c       | 11 +++++++++--
>>  3 files changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/migration/colo.h b/include/migration/colo.h
>> index 1636e6f907..768e1f04c3 100644
>> --- a/include/migration/colo.h
>> +++ b/include/migration/colo.h
>> @@ -25,7 +25,7 @@ void migrate_start_colo_process(MigrationState *s);
>>  bool migration_in_colo_state(void);
>>  
>>  /* loadvm */
>> -void migration_incoming_enable_colo(void);
>> +int migration_incoming_enable_colo(void);
>>  void migration_incoming_disable_colo(void);
>>  bool migration_incoming_colo_enabled(void);
>>  void *colo_process_incoming_thread(void *opaque);
>> diff --git a/migration/migration.c b/migration/migration.c
>> index 177cce9e95..f6830e4620 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -338,12 +338,18 @@ bool migration_incoming_colo_enabled(void)
>>  
>>  void migration_incoming_disable_colo(void)
>>  {
>> +    ram_block_discard_set_broken(false);
>>      migration_colo_enabled = false;
>>  }
>>  
>> -void migration_incoming_enable_colo(void)
>> +int migration_incoming_enable_colo(void)
>>  {
>> +    if (ram_block_discard_set_broken(true)) {
>> +        error_report("COLO: cannot set discarding of RAM broken");
> 
> I'd prefer 'COLO: cannot disable RAM discard'
> 
> 'broken' suggests the user has to go and fix something or report a bug
> or something.

Sounds better, I'll adjust similar messages in the other patches. Thanks!

> 
> Other than that:
> 
> 
> Reviewed-by: Dr. David Alan Gilbert <address@hidden>
> 
> Dave


-- 
Thanks,

David / dhildenb