[4/7] block: distinguish id and name in bdrv_find_snapshot()

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(-)

On Fri, Apr 26, 2013 at 05:31:12PM +0800, 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

s/travers/traverse/

Also in comments in the code.

> 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.

It's easy to avoid changing semantics: keep the old name or id behavior
around.  Use the new name-and-id behavior for #3 and #4.

Please include a justification for breaking the search order.

On 04/26/2013 08:34 AM, Stefan Hajnoczi wrote:
> On Fri, Apr 26, 2013 at 05:31:12PM +0800, 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
> 
> s/travers/traverse/
> 
> Also in comments in the code.
> 
>> 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.
> 
> It's easy to avoid changing semantics: keep the old name or id behavior
> around.  Use the new name-and-id behavior for #3 and #4.
> 
> Please include a justification for breaking the search order.

A good start for such a justification would be to edit parts of my email
[1] into your commit message, showing how the old semantics make it
IMPOSSIBLE to delete or load 'id 2 tag 1' without first getting 'id 1
tag 2' out of the way, assuming that a qcow2 file with poor naming
conventions exists (even if such a bad file can only be created
externally).  The goal of a commit message is to convince the readers
that a change in semantics is appropriate because the alternative of
leaving things broken is worse.

[1]https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg03501.html

于 2013-4-26 22:34, Stefan Hajnoczi 写道:
> On Fri, Apr 26, 2013 at 05:31:12PM +0800, 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
>
> s/travers/traverse/
>
> Also in comments in the code.
>
   OK.

>> 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.
>
> It's easy to avoid changing semantics: keep the old name or id behavior
> around.  Use the new name-and-id behavior for #3 and #4.
   You mean adding a new function, instead of change
bdrv_find_snapshot()?


>
> Please include a justification for breaking the search order.
>

On 04/26/2013 09:34 PM, Wenchao Xia wrote:
>>> 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.
>>
>> It's easy to avoid changing semantics: keep the old name or id behavior
>> around.  Use the new name-and-id behavior for #3 and #4.
>   You mean adding a new function, instead of change
> bdrv_find_snapshot()?

That's certainly an option.  Although after Pavel's proposed series for
adding QMP counterpart to savevm, there are no longer any uses of the
old semantics.

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).

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

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

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

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