[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 3/3 resend v2] arch_init: right return for ram_s
From: |
Orit Wasserman |
Subject: |
Re: [Qemu-devel] [PATCH 3/3 resend v2] arch_init: right return for ram_save_iterate |
Date: |
Thu, 12 Sep 2013 10:47:39 +0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130805 Thunderbird/17.0.8 |
On 09/11/2013 02:27 PM, Paolo Bonzini wrote:
> 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).
>
I also prefer the error checking at the top level.
Orit
>>>> 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
>
Re: [Qemu-devel] [PATCH 0/3 resend v2] Migration fix, Orit Wasserman, 2013/09/04