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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 11/09/2013 13:06, Juan Quintela ha scritto:
>>> 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_* callers to
> return an error.  It has some advantages and some disadvantages.  We
> don't agree on which ones are bigger O:-)

I think the disadvantages are bigger.  It litters the code with error
handling, hides where things actually happen, and doesn't even simplify
QEMUFile itself.  Checking only at the toplevel is simpler, all we need
to do is ensure that we get there every now and then (and that's what
qemu_file_rate_limit does).

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

Or alternatively, 0 means:

   we haven't finished the work

when what we mean is:

   we haven't finished the work (BTW, please check if there is an error)

> Notice that qemu_save_iterate() already returns errors in other code
> paths

Yes that's also unnecessary.

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

This is what an earlier patch from Lei did.  I told him (or her?) to
leave qemu_file_rate_limit aside since the idea behind QEMUFile is to
only handle the error at the top.

Paolo