From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49135) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UXFBx-0001nw-PK for qemu-devel@nongnu.org; Tue, 30 Apr 2013 14:22:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UXFBu-0007cg-Sp for qemu-devel@nongnu.org; Tue, 30 Apr 2013 14:22:17 -0400 Received: from mx1.redhat.com ([209.132.183.28]:31168) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UXFBu-0007cP-Lv for qemu-devel@nongnu.org; Tue, 30 Apr 2013 14:22:14 -0400 Message-ID: <51800A98.4040005@redhat.com> Date: Tue, 30 Apr 2013 12:16:56 -0600 From: Eric Blake MIME-Version: 1.0 References: <1366968675-1451-1-git-send-email-xiawenc@linux.vnet.ibm.com> <1366968675-1451-5-git-send-email-xiawenc@linux.vnet.ibm.com> In-Reply-To: <1366968675-1451-5-git-send-email-xiawenc@linux.vnet.ibm.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="----enig2JFLNRHRLSLWXUFNJIDTT" Subject: Re: [Qemu-devel] [PATCH 4/7] block: distinguish id and name in bdrv_find_snapshot() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Wenchao Xia Cc: kwolf@redhat.com, phrdina@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, stefanha@gmail.com, pbonzini@redhat.com This is an OpenPGP/MIME signed message (RFC 4880 and 3156) ------enig2JFLNRHRLSLWXUFNJIDTT Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 04/26/2013 03:31 AM, Wenchao Xia wrote: > To make it clear about id and name in searching, the API is changed > a bit to distinguish them, and caller can choose to search by id or nam= e. > If not found, *errp will be set to tip why. >=20 > Note that the caller logic is changed a bit: > 1) In del_existing_snapshots() called by do_savevm(), it travers twice > to find the snapshot, instead once, so matching sequence may change > if there are unwisely chosen, mixed id and names. > 2) In do_savevm(), same with del_existing_snapshot(), when it tries to > find the snapshot to overwrite, matching sequence may change for same > reason. > 3) In load_vmstate(), first when it tries to find the snapshot to be lo= aded, > sequence may change for the same reason of above. Later in validation, = the > logic is changed to be more strict to require both id and name matching= =2E > 4) In do_info_snapshot(), in validation, the logic is changed to be mor= e > strict to require both id and name matching. >=20 > Savevm, loadvm logic may need to be improved later, to avoid mixing of = them. >=20 > Some code is borrowed from Pavel's patch. >=20 > Signed-off-by: Wenchao Xia > Signed-off-by: Pavel Hrdina > --- > block/snapshot.c | 72 ++++++++++++++++++++++++++++++++++++++= +------- > include/block/snapshot.h | 5 ++- > savevm.c | 35 ++++++++++++---------- > 3 files changed, 83 insertions(+), 29 deletions(-) > + * > + * Returns: true when a snapshot is found and @sn_info will be filled,= false > + * when error or not found with @errp filled if errp !=3D NULL. > + */ > +bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_inf= o, > + const char *id, const char *name, Error **errp= ) Unusual convention to have (input, output, input, input, output) parameters; as long as you are changing the signature, I'd consider putting all input parameters (bs, id, name) firs, then output parameters last (sn_info, errp). > { > QEMUSnapshotInfo *sn_tab, *sn; > - int nb_sns, i, ret; > + int nb_sns, i; > + bool ret =3D false; > =20 > - ret =3D -ENOENT; > nb_sns =3D bdrv_snapshot_list(bs, &sn_tab); > if (nb_sns < 0) { > - return ret; > + error_setg_errno(errp, -nb_sns, "Failed to get a snapshot list= "); > + return false; > + } else if (nb_sns =3D=3D 0) { > + error_setg(errp, "Device has no snapshots"); > + return false; > } > - 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; > + No assertion that at least one of id or name is provided,... > + > + if (id && name) { > + for (i =3D 0; i < nb_sns; i++) { > + sn =3D &sn_tab[i]; > + if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) { > + *sn_info =3D *sn; > + ret =3D true; > + break; > + } > + } > + } else if (id) { > + for (i =3D 0; i < nb_sns; i++) { > + sn =3D &sn_tab[i]; > + if (!strcmp(sn->id_str, id)) { > + *sn_info =3D *sn; > + ret =3D true; > + break; > + } > + } > + } else if (name) { > + for (i =3D 0; i < nb_sns; i++) { > + sn =3D &sn_tab[i]; > + if (!strcmp(sn->name, name)) { > + *sn_info =3D *sn; > + ret =3D true; > + break; > + } > } > } > + > + if (!ret) { > + error_setg(errp, "Device have no matching snapshot"); > + } =2E..therefore, if I call bdrv_snapshot_find(bs, &info, NULL, NULL, errp)= , I'll get this error. Seems okay. > +++ b/savevm.c > @@ -2286,8 +2286,8 @@ static int del_existing_snapshots(Monitor *mon, c= onst char *name) > bs =3D NULL; > while ((bs =3D bdrv_next(bs))) { > if (bdrv_can_snapshot(bs) && > - bdrv_snapshot_find(bs, snapshot, name) >=3D 0) > - { > + (bdrv_snapshot_find(bs, snapshot, name, NULL, NULL) || > + bdrv_snapshot_find(bs, snapshot, NULL, name, NULL))) { This does an id lookup first, and falls back to a name lookup. Is that what we want? Consider an image with the following snapshots: id 1 name 2 id 2 name 3 id 3 name 1 id 4 name 5 Pre-patch, find(1) gives id 1, find(2) gives id 1, find(3) gives id 2, find(4) gives id 4, find(5) gives id 4; no way to get id 3. Post-patch, find(1,NULL) gives id 1, find(2,NULL) gives id 2, find(3,NULL) gives id 3, find(4,NULL) gives id 4, find(5,NULL) fails and you fall back to find(NULL,5) to give id 4. Thus, it only makes a difference for snapshots whose name is a numeric string that also matches an id, where your change now favors the id lookup over the entire set instead of the first name or id match while doing a single pass over the set. Pavel's series on top of this would change the code to favor a name-only lookup, or an explicit HMP option to do an id-only lookup, instead of this code's double lookup. At this point, I'm okay with the semantics of this patch (especially since we may be cleaning it up further in Pavel's patch series), but it deserves explicit documentation in the commit message on what semantics are changing (favoring id more strongly) and why (so that we can select all possible snapshots, instead of being unable to select snapshots whose id was claimed as a name of an earlier snapshot). > @@ -2437,12 +2437,14 @@ int load_vmstate(const char *name) > @@ -2461,11 +2463,11 @@ int load_vmstate(const char *name) > return -ENOTSUP; > } > =20 > - ret =3D bdrv_snapshot_find(bs, &sn, name); > - if (ret < 0) { > + /* vm snapshot will always have same id and name, check do_sav= evm(). */ > + if (!bdrv_snapshot_find(bs, &sn, sn.id_str, sn.name, NULL)) { > error_report("Device '%s' does not have the requested snap= shot '%s'", > bdrv_get_device_name(bs), name); > - return ret; > + return -ENOENT; > } Are we 100% sure that a given snapshot name has the same id across all block devices? Or is it possible to have: disk a: [id 1 name A, id 2 name B] disk b: [id 1 name B] where it is possible to load snapshot [B] and get consistent state? If it is possible to have non-matched ids across same-name snapshots, then looking up by requiring a match of both id and name will fail, whereas the pre-patch code would succeed. > } > =20 > @@ -2536,7 +2538,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]; > @@ -2567,8 +2569,9 @@ 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) { > + /* vm snapshot will always have same id and name */ > + if (!bdrv_snapshot_find(bs1, sn_info, > + sn->id_str, sn->name, NULL)) {= Again, is this true, or are you needlessly filtering out snapshots that have a consistent name but non-matching ids? --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org ------enig2JFLNRHRLSLWXUFNJIDTT 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/ iQEcBAEBCAAGBQJRgAqYAAoJEKeha0olJ0Nq5csH/2PLZ4HN2ZnaXc0xrQAY5P0i NxTHTdfIAG+xNpCTH/lvQZlTBnMiitZsCkmTRiilEgyXi6Iib8yeBbdXJxEy5M40 8BWuo0dTXo+RlbDHV/97M9wDb6YiSStGzPipuEup7itk+2QOAnoJdFPrIfdpX/FR Kq076BUR234HV7wwNIj55g/iY+AgLNoduyJUNb5CPbCtJ6xEFWzO53YimGLesQZB zn0q7RF7t6k2GD/UmC5TSrFw3HI6kSPTPWACoKIHowKvDvWRSTj2iT+h2msESNAB wss07zrYao/hJ75dUhA0snSVxlnhs8rhWrzTLkByHWAorpVpYTllBPA1AXcmcfM= =5T1S -----END PGP SIGNATURE----- ------enig2JFLNRHRLSLWXUFNJIDTT--