From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41230) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJgn4-0006S7-6s for qemu-devel@nongnu.org; Wed, 11 Sep 2013 05:32:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VJgmy-00011f-7E for qemu-devel@nongnu.org; Wed, 11 Sep 2013 05:32:50 -0400 Received: from mx1.redhat.com ([209.132.183.28]:25603) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJgmx-00011Q-U4 for qemu-devel@nongnu.org; Wed, 11 Sep 2013 05:32:44 -0400 Message-ID: <523038BD.4070706@redhat.com> Date: Wed, 11 Sep 2013 11:32:45 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1378285356-5308-1-git-send-email-lilei@linux.vnet.ibm.com> <1378285356-5308-4-git-send-email-lilei@linux.vnet.ibm.com> <877gen3efx.fsf@elfo.elfo> In-Reply-To: <877gen3efx.fsf@elfo.elfo> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 3/3 resend v2] arch_init: right return for ram_save_iterate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: quintela@redhat.com Cc: anthony@codemonkey.ws, Lei Li , mrhines@linux.vnet.ibm.com, qemu-devel@nongnu.org Il 11/09/2013 11:17, Juan Quintela ha scritto: > Lei Li 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 >> Signed-off-by: Paolo Bonzini >> --- >> >> 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. >