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

[Juan Quintela <quintela@redhat.com>]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 11/09/2013 11:17, Juan Quintela ha scritto:
>> Lei Li <lilei@linux.vnet.ibm.com> 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 <lilei@linux.vnet.ibm.com>
>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>> ---
>>>
>>> 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.


Caller checks also.  This is the reason I wanted qemu_file_* callos to
return an error.  It has some advantages and some disadvantages.  We
don't agree on which ones are bigger O:-)

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

In this case, 0 means:
  please, call us again
when what we mean is:
  don't care about calling us again, there is an error.  Handle the error.

Notice that qemu_save_iterate() already returns errors in other code
paths, not there because it don't know, code should be:

ret = qemu_file_rate_limit(f))

if (ret == 1) {
   return 0;
} else if (ret < 0) {
   return ret;
}

If we change th ereturn value for qemu_file_rate_limit() the change that
cames with this patch is not needed, that was my point.

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