From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:48370) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UY0nh-0006OF-8D for qemu-devel@nongnu.org; Thu, 02 May 2013 17:12:26 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UY0nf-000353-KO for qemu-devel@nongnu.org; Thu, 02 May 2013 17:12:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:61124) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UY0nf-00034s-Cg for qemu-devel@nongnu.org; Thu, 02 May 2013 17:12:23 -0400 Message-ID: <5182D6AF.6000001@redhat.com> Date: Thu, 02 May 2013 23:12:15 +0200 From: Pavel Hrdina 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> <51800A98.4040005@redhat.com> <5181C933.2050708@linux.vnet.ibm.com> In-Reply-To: <5181C933.2050708@linux.vnet.ibm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable 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, stefanha@gmail.com, qemu-devel@nongnu.org, armbru@redhat.com, pbonzini@redhat.com, lcapitulino@redhat.com On 2.5.2013 04:02, Wenchao Xia wrote: > =E4=BA=8E 2013-5-1 2:16, Eric Blake =E5=86=99=E9=81=93: >> 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 >>> name. >>> If not found, *errp will be set to tip why. >>> >>> Note that the caller logic is changed a bit: >>> 1) In del_existing_snapshots() called by do_savevm(), it travers twic= e >>> 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 t= o >>> 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 >>> loaded, >>> 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 matchi= ng. >>> 4) In do_info_snapshot(), in validation, the logic is changed to be m= ore >>> strict to require both id and name matching. >>> >>> Savevm, loadvm logic may need to be improved later, to avoid mixing >>> of them. >>> >>> Some code is borrowed from Pavel's patch. >>> >>> 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_info, >>> + const char *id, const char *name, Error **er= rp) >> >> 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 paramete= rs >> last (sn_info, errp). >> >>> { >>> QEMUSnapshotInfo *sn_tab, *sn; >>> - int nb_sns, i, ret; >>> + int nb_sns, i; >>> + bool ret =3D false; >>> >>> - 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"); >>> + } >> >> ...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, >>> const 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 tha= t >> 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-patc= h, >> find(1,NULL) gives id 1, find(2,NULL) gives id 2, find(3,NULL) gives i= d >> 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, wher= e >> your change now favors the id lookup over the entire set instead of th= e >> 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-on= ly >> 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 i= t >> deserves explicit documentation in the commit message on what semantic= s >> are changing (favoring id more strongly) and why (so that we can selec= t >> all possible snapshots, instead of being unable to select snapshots >> whose id was claimed as a name of an earlier snapshot). >> > To avoid trouble, I think a new function named > bdrv_snapshot_find_by_id_and_name() is better. Later Pavel > can directly call this new function, and after that we can > delete original bdrv_snapshot_find(). Pavel, what do you > think? Yes, we can create a new function with the new logic and the old one=20 will be dropped in my patch series. That's why I removed the old find=20 logic in my patch series in the last patch. > >>> @@ -2437,12 +2437,14 @@ int load_vmstate(const char *name) >>> @@ -2461,11 +2463,11 @@ int load_vmstate(const char *name) >>> return -ENOTSUP; >>> } >>> >>> - ret =3D bdrv_snapshot_find(bs, &sn, name); >>> - if (ret < 0) { >>> + /* vm snapshot will always have same id and name, check >>> do_savevm(). */ >>> + if (!bdrv_snapshot_find(bs, &sn, sn.id_str, sn.name, NULL)) = { >>> error_report("Device '%s' does not have the requested >>> snapshot '%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? I= f >> it is possible to have non-matched ids across same-name snapshots, the= n >> looking up by requiring a match of both id and name will fail, whereas >> the pre-patch code would succeed. >> > not possible, I checked the existing code, a loadable snapshot, that > is the one with vmstate, will always have same id and name, see savevm > logic and qcow2's snapshot creation code. > What changes is: previous, in "info snapshot", it will try show some > snapshot that may have the situation you described above, which may be > brought by qemu-img or hotplug operation, and which can't be loaded in > "loadvm". Now it will be filtered out. > Maybe there are more complicated case, but I think let management > stack handling it, is a better option. > > >>> } >>> >>> @@ -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) >>> >>> 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 tha= t >> have a consistent name but non-matching ids? >> > >