From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:47654) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVYYI-0006l8-8C for qemu-devel@nongnu.org; Thu, 25 Apr 2013 22:38:23 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UVYYH-0004Fu-6X for qemu-devel@nongnu.org; Thu, 25 Apr 2013 22:38:22 -0400 Received: from e23smtp02.au.ibm.com ([202.81.31.144]:36625) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVYYE-0004CX-Qp for qemu-devel@nongnu.org; Thu, 25 Apr 2013 22:38:21 -0400 Received: from /spool/local by e23smtp02.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 26 Apr 2013 12:30:13 +1000 Received: from d23relay05.au.ibm.com (d23relay05.au.ibm.com [9.190.235.152]) by d23dlp01.au.ibm.com (Postfix) with ESMTP id D18012CE804D for ; Fri, 26 Apr 2013 12:38:05 +1000 (EST) Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay05.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r3Q2OI0q20906164 for ; Fri, 26 Apr 2013 12:24:19 +1000 Received: from d23av04.au.ibm.com (loopback [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r3Q2c4iP014371 for ; Fri, 26 Apr 2013 12:38:04 +1000 Message-ID: <5179E86B.4060903@linux.vnet.ibm.com> Date: Fri, 26 Apr 2013 10:37:31 +0800 From: Wenchao Xia MIME-Version: 1.0 References: <5178CDB7.8080706@linux.vnet.ibm.com> <51791E83.1050708@redhat.com> In-Reply-To: <51791E83.1050708@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Subject: Re: [Qemu-devel] [PATCH v2 03/12] savevm: update bdrv_snapshot_find() to find snapshot by id or name and add error parameter List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: kwolf@redhat.com, armbru@redhat.com, Pavel Hrdina , qemu-devel@nongnu.org, lcapitulino@redhat.com 于 2013-4-25 20:16, Eric Blake 写道: > On 04/25/2013 12:31 AM, Wenchao Xia wrote: >> >>> + >>> + if (!found) { >>> + error_setg(errp, "Failed to find snapshot '%s'", name ? name : id); >> suggest not to set error, since it is a normal case. > > The way I understand it, failure to find a snapshot might need to be > treated as an error - it's up to the caller's needs. Also, there pretty > much is only one failure mode - the requested snapshot was not found - > even if there are multiple ways that we can fail to find a requested > snapshot, so I'm fine with treating all 'false' returns as an error path. > > Thus, a caller that wants to probe for a snapshot existence but not set > an error calls: > bdrv_snapshot_find(bs, snapshot, name, id, NULL, false); > > while a caller that wants to report a missing snapshot as an error calls: > bdrv_snapshot_find(bs, snapshot, name, id, &local_err, false); > and then propagates local_err on upwards. > > > Or are you worried about a possible third case, where a caller cares > about failure during bdrv_snapshot_list(), differently than failure to > find a snapshot? What callers have that semantics? If that is a real > concern, then maybe returning a bool is the wrong approach, and we > should instead return an int. A return < 0 is a fatal error > (bdrv_snapshot_list failed to even look up snapshots); a return of 0 > means our lookup attempt hit no fatal errors but the snapshot was not > found, and a return of 1 means the snapshot was found. Then there would > be three calling styles: > > Probe for existence, with no error reporting: > if (bdrv_snapshot_find(bs, snapshot, name, id, NULL, false) > 0) { > // exists > } > Probe for existence but with error reporting on fatal errors: > exist = bdrv_snapshot_find(bs, snapshot, name, id, &local_err, false); > if (exist < 0) { > // propagate local_err > } else if (exist) { > // exists > } > Probe for snapshot, with error reporting even for failed lookup: > if (bdrv_snapshot_find(bs, snapshot, name, id, &local_err, false) <= 0) { > // propagate local_err > } > > But I don't know what the existing callers need, to make a decision on > whether a signature change is warranted. Again, more reason to defer > this series to 1.6. > Personally I prefer internal layer have clean meaning, setting error only for exception. But I am not strongly against it, if caller can make easier use of it, a document for this function is also OK. -- Best Regards Wenchao Xia