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

[Markus Armbruster]

Eric Blake <address@hidden> 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 <address@hidden>
>> ---
>>  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.