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

[Pavel Hrdina <phrdina@redhat.com>]

On 2.5.2013 04:02, Wenchao Xia wrote:
> 于 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?

Yes, we can create a new function with the new logic and the old one 
will be dropped in my patch series. That's why I removed the old find 
logic in my patch series in the last patch.

>
>>> @@ -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?
>>
>
>