δΊ 2013-4-10 23:21, Markus Armbruster ει: > Wenchao Xia <xiaw...@linux.vnet.ibm.com> writes: > >> 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. >> Searching will be done with higher priority of id. This function also >> returns negative value from bdrv_snapshot_list() instead of -ENOENT on >> error now. >> >> Signed-off-by: Wenchao Xia <xiaw...@linux.vnet.ibm.com> >> Reviewed-by: Eric Blake <ebl...@redhat.com> >> Reviewed-by: Kevin Wolf <kw...@redhat.com> >> --- >> block/snapshot.c | 46 >> ++++++++++++++++++++++++++++++++++++++-------- >> include/block/snapshot.h | 2 +- >> savevm.c | 10 +++++----- >> 3 files changed, 44 insertions(+), 14 deletions(-) >> >> diff --git a/block/snapshot.c b/block/snapshot.c >> index c47a899..7b2026c 100644 >> --- a/block/snapshot.c >> +++ b/block/snapshot.c >> @@ -24,8 +24,20 @@ >> >> #include "block/snapshot.h" >> >> +/* >> + * Try find an internal snapshot with @id or @name, @id have higher priority >> + * in searching. >> + * >> + * @bs: block device to search on, must not be NULL. >> + * @sn_info: snapshot information to be filled in, must not be NULL. >> + * @id: snapshot id to search with, can be NULL. >> + * @name: snapshot name to search with, can be NULL. >> + * >> + * returns 0 and @sn_info is filled with related information if found, >> + * otherwise it returns negative value. >> + */ > > Let me try to improve: > > /** > * Look up an internal snapshot by @id, or else by @name. > * @bs: block device to search > * @sn_info: location to store information on the snapshot found > * @id: unique snapshot ID, or NULL > * @name: snapshot name, or NULL > * > * If the snapshot with unique ID @id exists, find it. > * Else, if snapshots with name @name exists, find one of them. > * > * Returns: 0 when a snapshot is found, else -errno. > */ > I will use it, thanks.
> I'd do two separate functions, one to loop up a snapshot by ID, and one > to look it up by name, because I find them simpler. Matter of taste. > >> int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, >> - const char *name) >> + const char *id, const char *name) >> { >> QEMUSnapshotInfo *sn_tab, *sn; >> int nb_sns, i, ret; >> @@ -33,16 +45,34 @@ int bdrv_snapshot_find(BlockDriverState *bs, >> QEMUSnapshotInfo *sn_info, >> ret = -ENOENT; >> nb_sns = bdrv_snapshot_list(bs, &sn_tab); >> if (nb_sns < 0) { >> - return ret; >> + return nb_sns; >> } >> - 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; >> + >> + /* search by id */ >> + if (id) { >> + for (i = 0; i < nb_sns; i++) { >> + sn = &sn_tab[i]; >> + if (!strcmp(sn->id_str, id)) { >> + *sn_info = *sn; >> + ret = 0; >> + goto out; >> + } >> } >> } >> + >> + /* search by name */ >> + if (name) { >> + for (i = 0; i < nb_sns; i++) { >> + sn = &sn_tab[i]; >> + if (!strcmp(sn->name, name)) { >> + *sn_info = *sn; >> + ret = 0; >> + goto out; >> + } >> + } >> + } >> + >> + out: >> g_free(sn_tab); >> return ret; >> } >> diff --git a/include/block/snapshot.h b/include/block/snapshot.h >> index 4ad070c..a047a8e 100644 >> --- a/include/block/snapshot.h >> +++ b/include/block/snapshot.h >> @@ -33,5 +33,5 @@ >> #include "block.h" >> >> int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info, >> - const char *name); >> + const char *id, const char *name); >> #endif >> diff --git a/savevm.c b/savevm.c >> index 88acc38..1d2da99 100644 >> --- a/savevm.c >> +++ b/savevm.c >> @@ -2212,7 +2212,7 @@ 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, name) >= 0) >> { >> ret = bdrv_snapshot_delete(bs, name); >> if (ret < 0) { > > Before your patch: deletes first snapshot sn whose sn->id_str or > sn->name match name. > > After your patch: deletes first snapshot sn whose sn->id_str matches > name, or else first snapshot whose sn->name matches name. > > Running example: say we have the following two snapshots with rather > unwisely chosen names: > > id_str name > 1 2 > 2 1 > > Which one will del_existing_snapshots(mon, "2") delete? > > Before your patch: first one. > > After: second one. > > See below for impact. > >> @@ -2272,7 +2272,7 @@ void do_savevm(Monitor *mon, const QDict *qdict) >> sn->vm_clock_nsec = qemu_get_clock_ns(vm_clock); >> >> if (name) { >> - ret = bdrv_snapshot_find(bs, old_sn, name); >> + ret = bdrv_snapshot_find(bs, old_sn, name, name); >> if (ret >= 0) { >> pstrcpy(sn->name, sizeof(sn->name), old_sn->name); >> pstrcpy(sn->id_str, sizeof(sn->id_str), old_sn->id_str); > > Similar change. > > This function is the only user of del_existing_snapshots(). > > Same example. > > Before your patch: "savevm 2" overwrites the first snapshot. > > After: it overwrites the second snapshot. > > Thus, your patch changes del_existing_snapshots() > > I'm fine with the change, but I want it noted *prominently* in the > commit message that it can change savevm's interpretation of ambiguous > name arguments. > OK. >> @@ -2363,7 +2363,7 @@ int load_vmstate(const char *name) >> } >> >> /* Don't even try to load empty VM states */ >> - ret = bdrv_snapshot_find(bs_vm_state, &sn, name); >> + ret = bdrv_snapshot_find(bs_vm_state, &sn, name, name); >> if (ret < 0) { >> return ret; >> } else if (sn.vm_state_size == 0) { >> @@ -2387,7 +2387,7 @@ int load_vmstate(const char *name) >> return -ENOTSUP; >> } >> >> - ret = bdrv_snapshot_find(bs, &sn, name); >> + ret = bdrv_snapshot_find(bs, &sn, name, name); >> if (ret < 0) { >> error_report("Device '%s' does not have the requested snapshot >> '%s'", >> bdrv_get_device_name(bs), name); > > Same example. > > Before your patch: "loadvm 2" loads the first snapshot. > > After: it loads the second snapshot. > > Document in commit message, just like the savevm change. > >> @@ -2493,7 +2493,7 @@ 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); >> + ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str, NULL); >> if (ret < 0) { >> available = 0; >> break; > > Different from the other calls: name argument is NULL rather than same > as id argument. Let's see what difference it makes. > > The loop iterates over all snapshots on the "primary" snapshot device > (the one that gets the VM state on savevm). For each one, it checks > whether all other snapshot devices have that snapshot. > > Before your patch: a device has the snapshot when it has one whose > sn->id_str or sn->name match the primary snapshot's id_str. Nuts :) > > After your patch: a device has the snapshot when it has one whose > sn->id_str matches the primary snapshot's id_str. > > I'm fine with the change, but it needs to be documented in the commit > message. > -- Best Regards Wenchao Xia