From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34603) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJiFj-0004mb-Cv for qemu-devel@nongnu.org; Wed, 11 Sep 2013 07:06:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VJiFd-0004C5-9j for qemu-devel@nongnu.org; Wed, 11 Sep 2013 07:06:31 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41210) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VJiFd-0004C1-1x for qemu-devel@nongnu.org; Wed, 11 Sep 2013 07:06:25 -0400 From: Juan Quintela In-Reply-To: <523038BD.4070706@redhat.com> (Paolo Bonzini's message of "Wed, 11 Sep 2013 11:32:45 +0200") 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> <523038BD.4070706@redhat.com> Date: Wed, 11 Sep 2013 13:06:18 +0200 Message-ID: <8738pb39ed.fsf@elfo.elfo> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 3/3 resend v2] arch_init: right return for ram_save_iterate Reply-To: quintela@redhat.com List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: anthony@codemonkey.ws, Lei Li , mrhines@linux.vnet.ibm.com, qemu-devel@nongnu.org Paolo Bonzini wrote: > 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. Caller checks also. This is the reason I wanted qemu_file_* callos to return an error. It has some advantages and some disadvantages. We don't agree on which ones are bigger O:-) > >> 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. Notice that qemu_save_iterate() already returns errors in other code paths, not there because it don't know, code should be: ret = qemu_file_rate_limit(f)) if (ret == 1) { return 0; } else if (ret < 0) { return ret; } 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. > > Paolo > > >> >> I think that returning qemu_rate_limit() to return 0/1/negative makes sense. >> >> Thoughts? >> >> Thanks, Juan. >>