From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:44844) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVL6M-00022w-5T for qemu-devel@nongnu.org; Thu, 25 Apr 2013 08:16:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UVL6D-0000Wl-LK for qemu-devel@nongnu.org; Thu, 25 Apr 2013 08:16:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:41232) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UVL6D-0000WV-DQ for qemu-devel@nongnu.org; Thu, 25 Apr 2013 08:16:29 -0400 Message-ID: <51791E83.1050708@redhat.com> Date: Thu, 25 Apr 2013 06:16:03 -0600 From: Eric Blake MIME-Version: 1.0 References: <5178CDB7.8080706@linux.vnet.ibm.com> In-Reply-To: <5178CDB7.8080706@linux.vnet.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2JOFLFPDCPBIJCTXMLAHD" 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: Wenchao Xia Cc: lcapitulino@redhat.com, kwolf@redhat.com, Pavel Hrdina , qemu-devel@nongnu.org, armbru@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2JOFLFPDCPBIJCTXMLAHD Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 04/25/2013 12:31 AM, Wenchao Xia wrote: >=20 >> + >> + 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 =3D 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) <=3D = 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. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2JOFLFPDCPBIJCTXMLAHD Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJReR6DAAoJEKeha0olJ0NqNGUH/jgEtNJ/P3n9U1QxhQ+JSkoG CqKwuHn0I5s2eEcyjikq1dIeoQpR7frNyCdtE9Q7bN4ZbcMSPdPZicj47symxqsZ zcMdl3cIqArGQbQkt12SEe3lkXFaxQ43z7t7ekx6Jg8KlRd/9OcgALaHw78Ff6qp y7maX2LWYAkIl1xbhPaVaL6YYXKRvmTO904zTyyeC87vXw71QBRSlrUo+4th0vS9 o7otPQ1uealQnrKPZuFpo34tFsxJ4aIkERYcXuef2qyVZGc+iOLPKF2sr0BtKl7P eDBPcjCJTigpeepPCeC6mdqyA/PMp3d4kxsmOcnKJIYRkS5z3iVj1jbINtOGDQo= =rAfN -----END PGP SIGNATURE----- ------enig2JOFLFPDCPBIJCTXMLAHD--