Patchwork [V3,07/11] block: export function bdrv_find_snapshot()

login
register
mail settings
Submitter Wayne Xia
Date Jan. 14, 2013, 7:09 a.m.
Message ID <1358147387-8221-8-git-send-email-xiawenc@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/211724/
State New
Headers show

Comments

Wayne Xia - Jan. 14, 2013, 7:09 a.m.
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(-)
Eric Blake - Jan. 14, 2013, 11:39 p.m.
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
Wayne Xia - Jan. 15, 2013, 10:24 a.m.
于 2013-1-15 7:39, Eric Blake 写道:
> 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
>

how about:
/*  if id is not NULL, try find it with id, if not exist, return NULL
  *  if id is NULL and name is not NULL, try find it with name.
  */ if id and name is NULL, direct return fail.
int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
                        const char *id, const char *name)
Markus Armbruster - Jan. 15, 2013, 12:01 p.m.
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.
Pavel Hrdina - Jan. 15, 2013, 1:18 p.m.
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
Eric Blake - Jan. 15, 2013, 5:57 p.m.
On 01/15/2013 03:24 AM, Wenchao Xia wrote:

>>>
>>> +int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>> +                       const char *name)

>>> +        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
>>

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

> how about:
> /*  if id is not NULL, try find it with id, if not exist, return NULL
>  *  if id is NULL and name is not NULL, try find it with name.
>  */ if id and name is NULL, direct return fail.
> int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>                        const char *id, const char *name)

That would be pushing the burden onto the callers to decide whether they
are doing an id lookup, a name lookup, or both.  In QMP terms, that
means that your QMP command to delete a snapshot would now need an
additional optional argument to decide whether the associated name is
only an id, only a name, or can match either.  But I'm not sure you want
that.

What I was trying to get at is that given a single string "2", it does
seem nicer to do both an id and a name lookup, and return the first hit;
you just need to document that ids take preference over names (and thus,
naming a snapshot "2" may make the snapshot become invisible by name,
but not by id, if a later snapshot creation causes id 2 to be used).
Then your QMP command for deleting a snapshot no longer needs to care
whether "2" is an id or a name, just whether it matches.

Hmm, while typing this, I thought of another snag.  Suppose you have a
VM with two disks, but where only the first disk previously had a
snapshot with id 1.  If I create a new snapshot across both disks, does
that mean disk 1 gets id 2 while disk 2 gets id 1, or do both disks get
id 2, even though that means disk 2 skips over id 1?  As long as the
snapshot is named, you can refer to the name to get the same snapshot
across both disks, regardless of what id it has.  But if the name is
numeric, and id takes preference over name when doing a lookup, we could
get ourselves into the situation where a snapshot created with name "2"
can eventually never be restored in one piece, because the individual
disks have different ids for the same snapshot name.  So maybe we DO
need a way after all for QMP to specify whether a name lookup is for id,
name, or both.
Wayne Xia - Jan. 16, 2013, 4:28 a.m.
于 2013-1-16 1:57, Eric Blake 写道:
> On 01/15/2013 03:24 AM, Wenchao Xia wrote:
>
>>>>
>>>> +int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>>> +                       const char *name)
>
>>>> +        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
>>>
>
>>>
>>> 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,
>
>> how about:
>> /*  if id is not NULL, try find it with id, if not exist, return NULL
>>   *  if id is NULL and name is not NULL, try find it with name.
>>   */ if id and name is NULL, direct return fail.
>> int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>                         const char *id, const char *name)
>
> That would be pushing the burden onto the callers to decide whether they
> are doing an id lookup, a name lookup, or both.  In QMP terms, that
> means that your QMP command to delete a snapshot would now need an
> additional optional argument to decide whether the associated name is
> only an id, only a name, or can match either.  But I'm not sure you want
> that.
>
> What I was trying to get at is that given a single string "2", it does
> seem nicer to do both an id and a name lookup, and return the first hit;
> you just need to document that ids take preference over names (and thus,
> naming a snapshot "2" may make the snapshot become invisible by name,
> but not by id, if a later snapshot creation causes id 2 to be used).
> Then your QMP command for deleting a snapshot no longer needs to care
> whether "2" is an id or a name, just whether it matches.
>
   OK, I guess following is better and clear:

  /* Try to find the snapshot with @id and @name, @id have higher
   * priority in searching. Both @id and @name can be NULL.
   */
int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
                         const char *id, const char *name)

Then caller at hmp/qmp layer can decide how to use it. For info of
snapshot retrieving in this serial, things are simple. for create/delete
of snapshot in future, I think a check with id_wellformed() and an
interface like the proposal from Pavel is needed.
http://lists.gnu.org/archive/html/qemu-devel/2013-01/msg01551.html

> Hmm, while typing this, I thought of another snag.  Suppose you have a
> VM with two disks, but where only the first disk previously had a
> snapshot with id 1.  If I create a new snapshot across both disks, does
> that mean disk 1 gets id 2 while disk 2 gets id 1, or do both disks get
> id 2, even though that means disk 2 skips over id 1?  As long as the
> snapshot is named, you can refer to the name to get the same snapshot
> across both disks, regardless of what id it has.  But if the name is
> numeric, and id takes preference over name when doing a lookup, we could
> get ourselves into the situation where a snapshot created with name "2"
> can eventually never be restored in one piece, because the individual
> disks have different ids for the same snapshot name.  So maybe we DO
> need a way after all for QMP to specify whether a name lookup is for id,
> name, or both.
>
Markus Armbruster - Jan. 25, 2013, 8:20 a.m.
Eric Blake <eblake@redhat.com> writes:

> On 01/15/2013 03:24 AM, Wenchao Xia wrote:
>
>>>>
>>>> +int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>>> +                       const char *name)
>
>>>> +        if (!strcmp(sn->id_str, name) || !strcmp(sn->name, name)) {
>>>
>
>>>
>>> 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,
>
>> how about:
>> /*  if id is not NULL, try find it with id, if not exist, return NULL
>>  *  if id is NULL and name is not NULL, try find it with name.
>>  */ if id and name is NULL, direct return fail.
>> int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
>>                        const char *id, const char *name)
>
> That would be pushing the burden onto the callers to decide whether they
> are doing an id lookup, a name lookup, or both.  In QMP terms, that
> means that your QMP command to delete a snapshot would now need an
> additional optional argument to decide whether the associated name is
> only an id, only a name, or can match either.  But I'm not sure you want
> that.
>
> What I was trying to get at is that given a single string "2", it does
> seem nicer to do both an id and a name lookup, and return the first hit;
> you just need to document that ids take preference over names (and thus,
> naming a snapshot "2" may make the snapshot become invisible by name,
> but not by id, if a later snapshot creation causes id 2 to be used).
> Then your QMP command for deleting a snapshot no longer needs to care
> whether "2" is an id or a name, just whether it matches.
>
> Hmm, while typing this, I thought of another snag.  Suppose you have a
> VM with two disks, but where only the first disk previously had a
> snapshot with id 1.  If I create a new snapshot across both disks, does
> that mean disk 1 gets id 2 while disk 2 gets id 1, or do both disks get
> id 2, even though that means disk 2 skips over id 1?  As long as the
> snapshot is named, you can refer to the name to get the same snapshot
> across both disks, regardless of what id it has.  But if the name is
> numeric, and id takes preference over name when doing a lookup, we could
> get ourselves into the situation where a snapshot created with name "2"
> can eventually never be restored in one piece, because the individual
> disks have different ids for the same snapshot name.  So maybe we DO
> need a way after all for QMP to specify whether a name lookup is for id,
> name, or both.

Once you move beyond a single image, internal snapshots provide
effectively nothing at all to ensure machine-level consistency.

Besides the name vs. ID confusion you mentioned, there's hotplug.  Save
snapshot, unplug a disk, restore snapshot, disk is still gone.  Plug it
back, disk is still in the future.  Save snapshot, plug another disk,
restore snapshot fails, because the new disk doesn't have it.

In my opinion, the sane mental model for dealing with internal snapshots
is to treat them strictly as a per disk thing, plus a funky way to
save/restore the machine state along with the disk state.

Perhaps something that's actually useful at the (non-toy, multi-image)
machine-level can be built from them.  But why bother; external
snapshots exist.

Patch

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)) {
+            *sn_info = *sn;
+            ret = 0;
+            break;
+        }
+    }
+    g_free(sn_tab);
+    return ret;
+}
+
 /* backing_file can either be relative, or absolute, or a protocol.  If it is
  * relative, it must be relative to the chain.  So, passing in bs->filename
  * from a BDS as backing_file should not be done, as that may be relative to
diff --git a/include/block/block.h b/include/block/block.h
index 5ce817a..c473be5 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -340,6 +340,8 @@  int bdrv_snapshot_list(BlockDriverState *bs,
 int bdrv_snapshot_load_tmp(BlockDriverState *bs,
                            const char *snapshot_name);
 char *bdrv_snapshot_dump(char *buf, int buf_size, QEMUSnapshotInfo *sn);
+int bdrv_snapshot_find(BlockDriverState *bs, QEMUSnapshotInfo *sn_info,
+                       const char *name);
 
 char *get_human_readable_size(char *buf, int buf_size, int64_t size);
 int path_is_absolute(const char *path);
diff --git a/savevm.c b/savevm.c
index 021420f..9af2605 100644
--- a/savevm.c
+++ b/savevm.c
@@ -2036,28 +2036,6 @@  out:
     return ret;
 }
 
-static 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)) {
-            *sn_info = *sn;
-            ret = 0;
-            break;
-        }
-    }
-    g_free(sn_tab);
-    return ret;
-}
-
 /*
  * Deletes snapshots of a given name in all opened images.
  */