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 <address@hidden>
Signed-off-by: Pavel Hrdina <address@hidden>
---
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).