[Top][All Lists]

[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: Lei Li
Subject: Re: [Qemu-devel] [PATCH 3/3 resend v2] arch_init: right return for ram_save_iterate
Date: Thu, 12 Sep 2013 15:11:53 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Thunderbird/17.0

On 09/11/2013 07: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).

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

It's her. :)

leave qemu_file_rate_limit aside since the idea behind QEMUFile is to
only handle the error at the top.

Yes, I changed the return value for qemu_file_rate_limit() to negative for
that if there has been an error at the beginning. After the discussion with
Paolo, I think he's suggestion based on the idea that only handle error at
the top behind QEMUFile makes sense to me, so that's where this patch come



reply via email to

[Prev in Thread] Current Thread [Next in Thread]