[Qemu-devel] [PATCH 1/2] qcow2-snapshot: Change filter function to avoid NULL parameter when a snapshot will be created.

[Qemu-devel] [PATCH 1/2] qcow2-snapshot: Change filter function to avoid NULL parameter when a snapshot will be created.

[Julio Faracco]

This is a trivial patch to change the filter 'find_snapshot_by_id_and_name'
to 'find_snapshot_by_id_or_name'. The field 'Name' is not required. So this
change avoid NULL parameters. And use the correct function to filter.

Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
---
 block/qcow2-snapshot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index 5b3903c..c7d4cfe 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -357,7 +357,7 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
     }
 
     /* Check that the ID is unique */
-    if (find_snapshot_by_id_and_name(bs, sn_info->id_str, NULL) >= 0) {
+    if (find_snapshot_by_id_or_name(bs, sn_info->id_str) >= 0) {
         return -EEXIST;
     }
 
-- 
1.9.1

[Qemu-devel] [PATCH 2/2] qcow2-snapshot: Fixing bug of creating snapshots with the same name.

[Julio Faracco]

This commit fixes the bug #1396497. You can create multiple snapshots with
the same name using 'qemu-img'. When you want to delete someone passing the
name, it will remove the first occurence of the snapshot with that name.
This commit fixes it.

Before:
$ qemu-img snapshot -c foo debian.qcow2
$ qemu-img snapshot -c foo debian.qcow2
$ qemu-img snapshot -c foo debian.qcow2
$ qemu-img snapshot -l debian.qcow2
ID        TAG                 VM SIZE                DATE       VM CLOCK
1         foo                    220M 2015-01-21 16:22:41   00:02:50.862
2         foo                    130M 2015-01-22 11:14:55   00:00:40.132
3         foo                     65M 2015-01-22 11:16:32   00:01:13.414

Now:
$ qemu-img snapshot -c foo debian.qcow2
$ qemu-img snapshot -c foo debian.qcow2
qemu-img: Could not create snapshot 'foo': -17 (File exists)
$ qemu-img snapshot -l debian.qcow2
ID        TAG                 VM SIZE                DATE       VM CLOCK
1         foo                    220M 2015-01-21 16:22:41   00:02:50.862

Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
---
 block/qcow2-snapshot.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
index c7d4cfe..873ac49 100644
--- a/block/qcow2-snapshot.c
+++ b/block/qcow2-snapshot.c
@@ -356,8 +356,9 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
         find_new_snapshot_id(bs, sn_info->id_str, sizeof(sn_info->id_str));
     }
 
-    /* Check that the ID is unique */
-    if (find_snapshot_by_id_or_name(bs, sn_info->id_str) >= 0) {
+    /* Check that the ID and Name is unique */
+    if (find_snapshot_by_id_or_name(bs, sn_info->id_str) >= 0 ||
+        find_snapshot_by_id_or_name(bs, sn_info->name) >= 0 ) {
         return -EEXIST;
     }
 
-- 
1.9.1

Re: [Qemu-devel] [PATCH 2/2] qcow2-snapshot: Fixing bug of creating snapshots with the same name.

[Max Reitz]

On 2015-01-22 at 20:53, Julio Faracco wrote:
> This commit fixes the bug #1396497. You can create multiple snapshots with
> the same name using 'qemu-img'. When you want to delete someone passing the
> name, it will remove the first occurence of the snapshot with that name.
> This commit fixes it.

I'm not so sure about these patches. Because there is an ID, I'd 
actually expect qemu to be able to create multiple snapshots with the 
same name (as long as the ID is unique for each snapshot).

I think the real problem is that find_snapshot_by_id_and_name() should 
not be successful if there are multiple snapshots which match the given 
ID/name combination (which should not happen if an ID has been given), 
which would prevent qcow2_snapshot_delete() from simply deleting the 
first matching snapshot.

Max

> Before:
> $ qemu-img snapshot -c foo debian.qcow2
> $ qemu-img snapshot -c foo debian.qcow2
> $ qemu-img snapshot -c foo debian.qcow2
> $ qemu-img snapshot -l debian.qcow2
> ID        TAG                 VM SIZE                DATE       VM CLOCK
> 1         foo                    220M 2015-01-21 16:22:41   00:02:50.862
> 2         foo                    130M 2015-01-22 11:14:55   00:00:40.132
> 3         foo                     65M 2015-01-22 11:16:32   00:01:13.414
>
> Now:
> $ qemu-img snapshot -c foo debian.qcow2
> $ qemu-img snapshot -c foo debian.qcow2
> qemu-img: Could not create snapshot 'foo': -17 (File exists)
> $ qemu-img snapshot -l debian.qcow2
> ID        TAG                 VM SIZE                DATE       VM CLOCK
> 1         foo                    220M 2015-01-21 16:22:41   00:02:50.862
>
> Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
> ---
>   block/qcow2-snapshot.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index c7d4cfe..873ac49 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -356,8 +356,9 @@ int qcow2_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
>           find_new_snapshot_id(bs, sn_info->id_str, sizeof(sn_info->id_str));
>       }
>   
> -    /* Check that the ID is unique */
> -    if (find_snapshot_by_id_or_name(bs, sn_info->id_str) >= 0) {
> +    /* Check that the ID and Name is unique */
> +    if (find_snapshot_by_id_or_name(bs, sn_info->id_str) >= 0 ||
> +        find_snapshot_by_id_or_name(bs, sn_info->name) >= 0 ) {
>           return -EEXIST;
>       }
>   

Re: [Qemu-devel] [PATCH 2/2] qcow2-snapshot: Fixing bug of creating snapshots with the same name.

[Julio Faracco]

[-- Attachment #1: Type: text/plain, Size: 385 bytes --]

Hi Max.

Yes, you are right! The main problem about this issue is deleting the first
occurence of the snapshot with that name.
Considering I have 3 snapshots with the name 'foo', you need to have the
option to choose what snapshot (what 'foo's) you want to remove or, as you
said, do not remove none of them.

Libvirt for example does not allow to create snapshots with the same name.

[-- Attachment #2: Type: text/html, Size: 475 bytes --]