From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:43844) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VK14d-0006tH-E1 for qemu-devel@nongnu.org; Thu, 12 Sep 2013 03:12:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VK14T-0002IE-CE for qemu-devel@nongnu.org; Thu, 12 Sep 2013 03:12:19 -0400 Received: from e28smtp09.in.ibm.com ([122.248.162.9]:49608) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VK14S-0002Gp-Qc for qemu-devel@nongnu.org; Thu, 12 Sep 2013 03:12:09 -0400 Received: from /spool/local by e28smtp09.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 12 Sep 2013 12:35:37 +0530 Received: from d28relay04.in.ibm.com (d28relay04.in.ibm.com [9.184.220.61]) by d28dlp01.in.ibm.com (Postfix) with ESMTP id B37A9E0056 for ; Thu, 12 Sep 2013 12:42:50 +0530 (IST) Received: from d28av01.in.ibm.com (d28av01.in.ibm.com [9.184.220.63]) by d28relay04.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r8C7Br2c42926158 for ; Thu, 12 Sep 2013 12:41:56 +0530 Received: from d28av01.in.ibm.com (localhost [127.0.0.1]) by d28av01.in.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id r8C7Bt29014503 for ; Thu, 12 Sep 2013 12:41:55 +0530 Message-ID: <52316939.2090204@linux.vnet.ibm.com> Date: Thu, 12 Sep 2013 15:11:53 +0800 From: Lei Li 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> <523038BD.4070706@redhat.com> <8738pb39ed.fsf@elfo.elfo> <523053A8.4000009@redhat.com> In-Reply-To: <523053A8.4000009@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed 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: Paolo Bonzini , Juan Quintela Cc: mrhines@linux.vnet.ibm.com, anthony@codemonkey.ws, qemu-devel@nongnu.org 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 >>>> 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 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 from. > > Paolo > -- Lei