Re: [Qemu-devel] [PATCH V3 07/11] block: export function bdrv_find_snapshot()

[Pavel Hrdina <phrdina@redhat.com>]

On 01/15/2013 01:01 PM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
>
>> On 01/14/2013 12:09 AM, Wenchao Xia wrote:
>>>    This patch move it from savevm.c to block.c and export it.
>>>
>>> Signed-off-by: Wenchao Xia <xiawenc@linux.vnet.ibm.com>
>>> ---
>>>   block.c               |   23 +++++++++++++++++++++++
>>>   include/block/block.h |    2 ++
>>>   savevm.c              |   22 ----------------------
>>>   3 files changed, 25 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/block.c b/block.c
>>> index 8192d8e..b7d2f03 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -3351,6 +3351,29 @@ int bdrv_snapshot_load_tmp(BlockDriverState *bs,
>>>       return -ENOTSUP;
>>>   }
>>>
>>> +int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>> +                       const char *name)
>>> +{
>>> +    QEMUSnapshotInfo *sn_tab, *sn;
>>> +    int nb_sns, i, ret;
>>> +
>>> +    ret = -ENOENT;
>>> +    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
>>> +    if (nb_sns < 0) {
>>> +        return ret;
>>> +    }
>>> +    for (i = 0; i < nb_sns; i++) {
>>> +        sn = &sn_tab[i];
>>> +        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
>>
>> It is possible (albeit probably stupid) to create a qcow2 file where
>> snapshot names are merely numeric strings.  In fact, just to see what
>> would happen, I once[1] created a file where:
>>
>> snapshot id '1' was named '2'
>> snapshot id '2' was named 'foo'
>>
>> This code comparison favors ids over names; so if I request to delvm 2,
>> I end up removing the second snapshot, not the first.  This is okay, but
>> probably worth documenting, and probably worth making sure that all code
>> that looks up a snapshot by name or id goes through this function so
>> that we get the same behavior everywhere.  My experiment was done
>> several months ago, but my recollection was that at the time, there was
>> an inconsistency where 'qemu-img snapshot' picked a different snapshot
>> for the request of '2' than the online 'delvm' monitor command of qemu;
>> making it unsafe to rely on either behavior in that version of qemu
>> source code.
>>
>> [1]https://bugzilla.redhat.com/show_bug.cgi?id=733143
>
> In QemuOpts, we restricted IDs to [[:alpha:]][[:alnum:]-._]*, see
> id_wellformed().  I'd recommend the same for new interfaces.  But this
> is an old one, and we shouldn't make existing snapshots inaccessible
> just because their names were chosen unwisely.
>

There is my proposal for handling snapshots id and name.

http://lists.gnu.org/archive/html/qemu-devel/2013-01/msg01551.html

In general, we will have two options for operation with snapshots, an 
'id' option and a 'name' option. You could choose one of them or both to 
specify which snapshot you want to load/delete/create/modify.

This behaviour should be the same for online and offline snapshots even 
for vm-snapshots or block-snapshots.

With these modifications old snapshots should also works.

Pavel