Re: [Qemu-devel] [PATCH 1/2] migration: Test for disabled features on reception

[Juan Quintela <quintela@redhat.com>]

Peter Xu <peterx@redhat.com> wrote:
> On Tue, Jun 13, 2017 at 11:54:31AM +0200, Juan Quintela wrote:
>> Right now, if we receive a compressed page while this features are
>> disabled, Bad Things (TM) can happen.  Just add a test for them.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> 
>> --
>> 
>> I had XBZRLE here also, but it don't need extra resources on
>> destination, only on source.  Additionally libvirt don't enable it on
>> destination, so don't put it here.
>> ---
>>  migration/ram.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>> 
>> diff --git a/migration/ram.c b/migration/ram.c
>> index f35d65a..f2d1bce 100644
>> --- a/migration/ram.c
>> +++ b/migration/ram.c
>> @@ -2477,7 +2477,7 @@ static int ram_load_postcopy(QEMUFile *f)
>>  
>>  static int ram_load(QEMUFile *f, void *opaque, int version_id)
>>  {
>> -    int flags = 0, ret = 0;
>> +    int flags = 0, ret = 0, invalid_flags;
>>      static uint64_t seq_iter;
>>      int len = 0;
>>      /*
>> @@ -2494,6 +2494,11 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>>          ret = -EINVAL;
>>      }
>>  
>> +    invalid_flags = 0;
>
> Nit: set zero when declare (just like flags, ret)?

Fixed.

As answered to dave, I use to do it that way.
But here it is more consistant to do it the other way.

>> +
>> +    if (!migrate_use_compression()) {
>> +        invalid_flags |= RAM_SAVE_FLAG_COMPRESS_PAGE;
>> +    }
>>      /* This RCU critical section can be very long running.
>>       * When RCU reclaims in the code start to become numerous,
>>       * it will be necessary to reduce the granularity of this
>> @@ -2514,6 +2519,15 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id)
>>          flags = addr & ~TARGET_PAGE_MASK;
>>          addr &= TARGET_PAGE_MASK;
>>  
>> +        if (flags & invalid_flags) {
>> +            if (flags & invalid_flags  & RAM_SAVE_FLAG_COMPRESS_PAGE) {
>                                         ^
> Nit: one extra space -------------------+

Fixed.

>> +                error_report("Received an unexpected compressed page");
>> +            }
>> +
>> +            ret = -EINVAL;
>> +            break;
>> +        }
>> +
>>          if (flags & (RAM_SAVE_FLAG_ZERO | RAM_SAVE_FLAG_PAGE |
>>                       RAM_SAVE_FLAG_COMPRESS_PAGE | RAM_SAVE_FLAG_XBZRLE)) {
>>              RAMBlock *block = ram_block_from_stream(f, flags);
>> -- 
>> 2.9.4
>> 
>
> Besides the nits:
>
> Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks.