From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50286) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UXirQ-0001Dy-Qz for qemu-devel@nongnu.org; Wed, 01 May 2013 22:03:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UXirP-0004zG-Ap for qemu-devel@nongnu.org; Wed, 01 May 2013 22:03:04 -0400 Received: from e23smtp04.au.ibm.com ([202.81.31.146]:48004) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UXirO-0004yY-Gl for qemu-devel@nongnu.org; Wed, 01 May 2013 22:03:03 -0400 Received: from /spool/local by e23smtp04.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 2 May 2013 11:50:44 +1000 Received: from d23relay03.au.ibm.com (d23relay03.au.ibm.com [9.190.235.21]) by d23dlp02.au.ibm.com (Postfix) with ESMTP id 183E22BB0050 for ; Thu, 2 May 2013 12:02:51 +1000 (EST) Received: from d23av03.au.ibm.com (d23av03.au.ibm.com [9.190.234.97]) by d23relay03.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r4222iUW21364922 for ; Thu, 2 May 2013 12:02:44 +1000 Received: from d23av03.au.ibm.com (loopback [127.0.0.1]) by d23av03.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id r4222onq000491 for ; Thu, 2 May 2013 12:02:50 +1000 Message-ID: <5181C933.2050708@linux.vnet.ibm.com> Date: Thu, 02 May 2013 10:02:27 +0800 From: Wenchao Xia 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> In-Reply-To: <51800A98.4040005@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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: Eric Blake Cc: kwolf@redhat.com, phrdina@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org, lcapitulino@redhat.com, stefanha@gmail.com, pbonzini@redhat.com 于 2013-5-1 2:16, Eric Blake 写道: > 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 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 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 matching. >> 4) In do_info_snapshot(), in validation, the logic is changed to be more >> 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 != NULL. >> + */ >> +bool bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, >> + 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 = false; >> >> - ret = -ENOENT; >> nb_sns = 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 == 0) { >> + error_setg(errp, "Device has no snapshots"); >> + return false; >> } >> - for (i = 0; i < nb_sns; i++) { >> - sn = &sn_tab[i]; >> - if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) { >> - *sn_info = *sn; >> - ret = 0; >> - break; >> + > > No assertion that at least one of id or name is provided,... > >> + >> + if (id && name) { >> + for (i = 0; i < nb_sns; i++) { >> + sn = &sn_tab[i]; >> + if (!strcmp(sn->id_str, id) && !strcmp(sn->name, name)) { >> + *sn_info = *sn; >> + ret = true; >> + break; >> + } >> + } >> + } else if (id) { >> + for (i = 0; i < nb_sns; i++) { >> + sn = &sn_tab[i]; >> + if (!strcmp(sn->id_str, id)) { >> + *sn_info = *sn; >> + ret = true; >> + break; >> + } >> + } >> + } else if (name) { >> + for (i = 0; i < nb_sns; i++) { >> + sn = &sn_tab[i]; >> + if (!strcmp(sn->name, name)) { >> + *sn_info = *sn; >> + ret = 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 = NULL; >> while ((bs = bdrv_next(bs))) { >> if (bdrv_can_snapshot(bs) && >> - bdrv_snapshot_find(bs, snapshot, name) >= 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). > 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? >> @@ -2437,12 +2437,14 @@ int load_vmstate(const char *name) >> @@ -2461,11 +2463,11 @@ int load_vmstate(const char *name) >> return -ENOTSUP; >> } >> >> - ret = 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? 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. > 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 = &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 = bdrv_next(bs1))) { >> if (bdrv_can_snapshot(bs1) && bs1 != bs) { >> - ret = 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? > -- Best Regards Wenchao Xia