From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:38926) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1US9lz-0005xU-Gi for qemu-devel@nongnu.org; Tue, 16 Apr 2013 13:34:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1US9lx-0007eh-Mp for qemu-devel@nongnu.org; Tue, 16 Apr 2013 13:34:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37131) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1US9lx-0007eW-Ej for qemu-devel@nongnu.org; Tue, 16 Apr 2013 13:34:25 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r3GHYNhw021207 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 16 Apr 2013 13:34:24 -0400 Message-ID: <516D8B9E.6010609@redhat.com> Date: Tue, 16 Apr 2013 11:34:22 -0600 From: Eric Blake MIME-Version: 1.0 References: <17626585faf740878394cb2cbdc733cfb0c535e8.1366127809.git.phrdina@redhat.com> In-Reply-To: <17626585faf740878394cb2cbdc733cfb0c535e8.1366127809.git.phrdina@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2AJTUOWHUKKBFDVJKNSNK" Subject: Re: [Qemu-devel] [PATCH 03/11] savevm: update bdrv_snapshot_find() to find snapshot by id or name List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Pavel Hrdina Cc: lcapitulino@redhat.com, qemu-devel@nongnu.org, armbru@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2AJTUOWHUKKBFDVJKNSNK Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 04/16/2013 10:05 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. Fair enough, even if it is a bit of churn on the signature and callers. >=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 1 otherwise > it returns 0. If you are only returning a yes/no result, bool is better than int. >=20 > Signed-off-by: Pavel Hrdina > --- > savevm.c | 73 ++++++++++++++++++++++++++++++++++++++++++++------------= -------- > 1 file changed, 50 insertions(+), 23 deletions(-) >=20 > diff --git a/savevm.c b/savevm.c > index 6af84fd..96a2340 100644 > --- a/savevm.c > +++ b/savevm.c > @@ -2197,25 +2197,55 @@ out: > } > =20 > static int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *= sn_info, Return bool... > - const char *name) > + const char *name, const char *id, bool o= ld_match) > { > QEMUSnapshotInfo *sn_tab, *sn; > - int nb_sns, i, ret; > + int nb_sns, i, found =3D 0; =2E.. and initialize 'bool found =3D false'. > + > + if (!name && !id) { > + return found; > + } It may be appropriate to do assert(name || id), if you want to guarantee that all callers supply at least one of the two arguments (and that the QMP code where both arguments are optional does its own sanity checking that the user supplied an argument. > =20 > - ret =3D -ENOENT; > nb_sns =3D bdrv_snapshot_list(bs, &sn_tab); > - if (nb_sns < 0) > - return ret; > + if (nb_sns < 0) { > + return found; Calling the return value 'found' makes the code read awkwardly when reporting failure; maybe keeping it named 'ret' is better after all. > + } > + > for(i =3D 0; i < nb_sns; i++) { As long as you are touching this code, you could insert the space after 'for'. > 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 1; > + break; > + } > + } else if (name) { > + /* for compatibility for old bdrv_snapshot_find call > + * will be removed */ > + if (old_match) { > + if (!strcmp(sn->id_str, id) || !strcmp(sn->name, name)= ) { > + *sn_info =3D *sn; > + found =3D 1; > + break; > + } > + } else { > + if (!strcmp(sn->name, name)) { > + *sn_info =3D *sn; > + found =3D 1; > + break; > + } > + } > + } else if (id) { > + if (!strcmp(sn->id_str, id)) { > + *sn_info =3D *sn; > + found =3D 1; > + break; > + } Note that this says that if both name and id are specified, then that takes priority, no matter what was specified for old_match. > } > } > + > g_free(sn_tab); > - return ret; > + return found; > } > =20 > /* > @@ -2229,8 +2259,8 @@ static int del_existing_snapshots(Monitor *mon, c= onst char *name) > =20 > bs =3D NULL; > while ((bs =3D bdrv_next(bs))) { > - if (bdrv_can_snapshot(bs) && > - bdrv_snapshot_find(bs, snapshot, name) >=3D 0) > + if (bdrv_can_snapshot(bs) > + && bdrv_snapshot_find(bs, snapshot, name, name, true))= Needless style change. The old style was sufficient, with: if (bdrv_can_snapshot(bs) && bdrv_snapshot_find(bs, snapshot, name, name, true)) Except that bdrv_snapshot_find(bs, snapshot, name, name, anything) results in a lookup that requires sn->id_str and sn->name be the same string, which is not what you intended. You really want to be using bdrv_snapshot_find(bs, snapshot, NULL, name, true), if you want the old_match parameter to make a difference. =2E.. > @@ -2474,7 +2501,7 @@ void do_info_snapshots(Monitor *mon, const QDict = *qdict) > { > BlockDriverState *bs, *bs1; > QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info =3D &s; > - int nb_sns, i, ret, available; > + int nb_sns, i, available; > int total; > int *available_snapshots; > char buf[256]; > @@ -2505,8 +2532,8 @@ void do_info_snapshots(Monitor *mon, const QDict = *qdict) > =20 > while ((bs1 =3D bdrv_next(bs1))) { > if (bdrv_can_snapshot(bs1) && bs1 !=3D bs) { > - ret =3D bdrv_snapshot_find(bs1, sn_info, sn->id_str); > - if (ret < 0) { > + if (!bdrv_snapshot_find(bs1, sn_info, sn->name, sn->id= _str, > + true)) { This was the only conversion that passed both id and name that looked correct. All the other conversions to 'name, name, true' need fixing. --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2AJTUOWHUKKBFDVJKNSNK 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/ iQEcBAEBCAAGBQJRbYueAAoJEKeha0olJ0NqriYH/2whhMNxF1I1O0Jk/ze73N5+ /To8dqS2rLmC7xs52IQQ4dcll8bsvKFlRw2LwgeKaoUYhRHjUbx/g5yHeTDQN2MK V06dVT5DDf/6tYZg6UG8scfBmQcn3C4y+NJ2zr1v4/OlB8BafpYXYoWSbs/obgNV q1Uq9Et1LcoXBzZl5ueUiW2Z4d6f1HyphFv7W3qIWxItP1uMDU6GGu2hqXeorPWH VCeQ+19Puz9C0lzxhEpjJrGnqgZmjKD5CJOugNKmO7lX2Fykx/b9un7iKDX04EEW nQ0iub6Xj+TgcK6PcKXVElcve8E/PkO/4LyVFQ5pXFmuWMMqr29jpm+kZk2hrmM= =uiyB -----END PGP SIGNATURE----- ------enig2AJTUOWHUKKBFDVJKNSNK--