δΊ 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
