Re: [Qemu-devel] [PATCH 4/7] block: distinguish id and name in bdrv_find_snapshot()

[Wenchao Xia <xiawenc@linux.vnet.ibm.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 <xiawenc@linux.vnet.ibm.com>
>> Signed-off-by: Pavel Hrdina <phrdina@redhat.com>
>> ---
>>   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