Re: [Qemu-devel] [PATCH 3/3 resend v2] arch_init: right return for ram_save_iterate

[Paolo Bonzini]

Il 11/09/2013 11:17, Juan Quintela ha scritto:
> Lei Li <address@hidden> wrote:
>> qemu_file_rate_limit() never return negative value since the refactor
>> by Commit 1964a39, this patch gets rid of the negative check for it,
>> adjust bytes_transferred and return value correspondingly in 
>> ram_save_iterate().
>>
>> Signed-off-by: Lei Li <address@hidden>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> ---
>>
>> Change since v1:
>>   Return fixes and improvement from Paolo Bonzini.
>>   
>>  arch_init.c |   15 ++++++++++-----
>>  1 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index 94d45e1..a26bc89 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -709,15 +709,20 @@ static int ram_save_iterate(QEMUFile *f, void *opaque)
>>       */
>>      ram_control_after_iterate(f, RAM_CONTROL_ROUND);
>>  
>> +    bytes_transferred += total_sent;
> 
> Agreed.
> 
>> +
>> +    /*
>> +     * Do not count these 8 bytes into total_sent, so that we can
>> +     * return 0 if no page had been dirtied.
>> +     */
>> +    qemu_put_be64(f, RAM_SAVE_FLAG_EOS);
>> +    bytes_transferred += 8;
>> +
>> +    ret = qemu_file_get_error(f);
>>      if (ret < 0) {
> 
> Not sure this is the right solution.
> 
> We are sending anyways RAM_SAVE_FLAG_EOS.

If there is an error, the qemu_put_be64 will do nothing.  It is part of
the design of QEMUFile that you can keep sending stuff to it after an
error happened.

> And I think that the right solution is make qemu_get_rate_limit() to
> return -1 in case of error (or the error, I don't care).

You might do both things, it would avoid the useless g_usleep you
pointed out below.  But Lei's patch is good, because an error could
happen exactly during the qemu_put_be64 that writes RAM_SAVE_FLAG_EOS.

> savevm.c: qemu_savevm_state_iterate()
> 
>         if (qemu_file_rate_limit(f)) {
>             return 0;
>         }
> 
> check is incorrect again, we should return an error if there is one
> error.

Nothing cares if qemu_savevm_state_iterate returns 0 or negative, so
changing qemu_savevm_state_iterate to only return 0/1 would make sense too.

Paolo


> 
> I think that returning qemu_rate_limit() to return 0/1/negative makes sense.
> 
> Thoughts?
> 
> Thanks, Juan.
>