Re: [Qemu-devel] [PATCH V11 02/17] block: distinguish id and name in bdrv_find_snapshot()

[Wenchao Xia]

于 2013-4-10 23:21, Markus Armbruster 写道:
> Wenchao Xia <address@hidden> 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 <address@hidden>
>> Reviewed-by: Eric Blake <address@hidden>
>> Reviewed-by: Kevin Wolf <address@hidden>
>> ---
>>   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