From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:57149) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UV7DB-00067a-AE for qemu-devel@nongnu.org; Wed, 24 Apr 2013 17:26:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UV7D9-0001LN-QJ for qemu-devel@nongnu.org; Wed, 24 Apr 2013 17:26:45 -0400 Received: from mx1.redhat.com ([209.132.183.28]:16994) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UV7D9-0001Kr-Ar for qemu-devel@nongnu.org; Wed, 24 Apr 2013 17:26:43 -0400 Message-ID: <51784E0E.9090607@redhat.com> Date: Wed, 24 Apr 2013 15:26:38 -0600 From: Eric Blake MIME-Version: 1.0 References: In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2OPTRDTTGCVPHQBMVMLKU" 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: Pavel Hrdina Cc: kwolf@redhat.com, xiawenc@linux.vnet.ibm.com, lcapitulino@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2OPTRDTTGCVPHQBMVMLKU Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 04/24/2013 09:32 AM, Pavel Hrdina wrote: > Finding snapshot by a name which could also be an id isn't best way > how to do it. There will be rewrite of savevm, loadvm and delvm to > improve the behavior of these commands. The savevm and loadvm will > have their own patch series. >=20 > Now bdrv_snapshot_find takes more parameters. The name parameter will > be matched only against the name of the snapshot and the same applies > to id parameter. >=20 > There is one exception. If you set the last parameter, the name paramet= er > will be matched against the name or the id of a snapshot. This exceptio= n > is only for backward compatibility for other commands and it will be > dropped after all commands will be rewritten. >=20 > We only need to know if that snapshot exists or not. We don't care > about any error message. If snapshot exists it returns TRUE otherwise > it returns FALSE. >=20 > There is also new Error parameter which will containt error messeage if= double-typo and grammar: s/also/also a/ s/containt error messeage/contain the error message/ > something goes wrong. >=20 > Signed-off-by: Pavel Hrdina > --- > savevm.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++----------= -------- > 1 file changed, 67 insertions(+), 26 deletions(-) >=20 > diff --git a/savevm.c b/savevm.c > index ba97c41..1622c55 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -2262,26 +2262,66 @@ out: > return ret; > } > =20 > -static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *= sn_info, > - const char *name) > +static bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo = *sn_info, > + const char *name, const char *id, Error= **errp, > + bool old_match) I'd add a FIXME comment here documenting that you intend to remove the old_match parameter after all callers have been updated to the new semantics. > { > QEMUSnapshotInfo *sn_tab, *sn; > - int nb_sns, i, ret; > + int nb_sns, i; > + bool found =3D false; Bikeshedding: I don't think you need this variable, if you would instead do... > + > + assert(name || id); > =20 > - ret =3D -ENOENT; > nb_sns =3D bdrv_snapshot_list(bs, &sn_tab); > - if (nb_sns < 0) > - return ret; > - for(i =3D 0; i < nb_sns; i++) { > + if (nb_sns < 0) { > + error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list= "); > + return found; return false; > + } > + > + if (nb_sns =3D=3D 0) { > + error_setg(errp, "Device has no snapshots"); > + return found; return false; > + } *sn_info =3D NULL; > + > + for (i =3D 0; i < nb_sns; i++) { > sn =3D &sn_tab[i]; > - if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) { > - *sn_info =3D *sn; > - ret =3D 0; > - break; > + if (name && id) { > + if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) { > + *sn_info =3D *sn; > + found =3D true; Drop this assignment and others like it... > + break; > + } > + } else if (name) { > + /* for compatibility for old bdrv_snapshot_find call > + * will be removed */ > + if (old_match) { > + if (!strcmp(sn->id_str, name) || !strcmp(sn->name, nam= e)) { > + *sn_info =3D *sn; > + found =3D true; > + break; > + } > + } else { > + if (!strcmp(sn->name, name)) { > + *sn_info =3D *sn; > + found =3D true; > + break; > + } > + } > + } else if (id) { > + if (!strcmp(sn->id_str, id)) { > + *sn_info =3D *sn; > + found =3D true; > + break; > + } > } > } > + > + if (!found) { use 'if (*sn_info)' > + error_setg(errp, "Failed to find snapshot '%s'", name ? name := id); > + } > + > g_free(sn_tab); > - return ret; > + return found; return *sn_info !=3D NULL; > } If you _do_ decide to keep the boolean variable instead of hard-coding a false return and avoiding redundancy by using other variables to determine the result, then at least s/found/ret/, because I find 'return found' as a way to intentionally fail rather odd-looking. At any rate, I can live with this logic, and all the conversions of existing call sites properly passed the given name, NULL id, and true for old_match semantics; along with optional deciding whether to pass NULL or a local error based on whether it would ignore lookup failure or propagate it as a failure of the higher-level operation that needed a lookup. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2OPTRDTTGCVPHQBMVMLKU 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/ iQEcBAEBCAAGBQJReE4OAAoJEKeha0olJ0NqnusIAIBT8Uz0VFP3hClfe2eJ4ZhS wVzMV1ffyN4JPoZL3IyWX4p1gaJBDPIavhpAZqUC4+tljKk07X4ZtiPw2EpcGh+i 8g39c2S5gmM9rkK8unLpSgtubsa3EDgT4G1jWDPnPlMumZPZwVzNkCoDOT7h2/zz e5+uwIq0Xu8J/gZylHynqAmyshbdfaeyen/3v/cN0P8s5XO1Lt35XN1F3hO7vObR j0jpID2drHHl4blQ16cl9PQOpBVDyJDqUX78vl12LCr5Q8Ekqw0Aqs8/5/qrwjc7 XDHTCMF7HG4CsZ9W5xprFnXhiYzp+fEOJ9uTpmFbAoabyVNi+cU6QtVkT0VCNh8= =WMxg -----END PGP SIGNATURE----- ------enig2OPTRDTTGCVPHQBMVMLKU--