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