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