[Qemu-devel] [PATCH v3 0/3] Disallow snapshots if the overlay doesn't support backing files

[Qemu-devel] [PATCH v3 0/3] Disallow snapshots if the overlay doesn't support backing files

[Alberto Garcia]

We are currently allowing snapshots in cases like this one:

       { 'execute': 'blockdev-add', 'arguments':
         { 'options': { 'driver': 'qcow2',
                        'node-name': 'new0',
                        'file': { 'driver': 'file',
                                  'filename': 'new.qcow2',
                                  'node-name': 'file0' } } } }

       { 'execute': 'blockdev-snapshot', 'arguments':
         { 'node': 'virtio0',
           'overlay': 'file0' } }

This patch forbids snapshots if the overlay node does not support
backing files.

Regards,

Berto

v3:
- patch 2: new patch to remove all the existing inner quotation marks in
  test 085 [Max, Eric]
- patch 3: fix description and remove inner quotation marks [Max, Eric]

v2: https://lists.gnu.org/archive/html/qemu-block/2015-11/msg00005.html
- Disallow snapshots if new_bs->drv->supports_backing is false [Max]

v1: https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg06875.html
- Initial version.

Alberto Garcia (3):
  block: Disallow snapshots if the overlay doesn't support backing files
  block: Remove inner quotation marks in iotest 085
  block: test 'blockdev-snapshot' using a file BDS as the overlay

 blockdev.c                 |  5 +++++
 tests/qemu-iotests/085     | 26 ++++++++++++++++++--------
 tests/qemu-iotests/085.out |  4 ++++
 3 files changed, 27 insertions(+), 8 deletions(-)

-- 
2.6.2

[Qemu-devel] [PATCH v3 1/3] block: Disallow snapshots if the overlay doesn't support backing files

[Alberto Garcia]

This addresses scenarios like this one:

  { 'execute': 'blockdev-add', 'arguments':
    { 'options': { 'driver': 'qcow2',
                   'node-name': 'new0',
                   'file': { 'driver': 'file',
                             'filename': 'new.qcow2',
                             'node-name': 'file0' } } } }

  { 'execute': 'blockdev-snapshot', 'arguments':
    { 'node': 'virtio0',
      'overlay': 'file0' } }

Signed-off-by: Alberto Garcia <berto@igalia.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 blockdev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index a567a05..2774bf5 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1667,6 +1667,11 @@ static void external_snapshot_prepare(BlkTransactionState *common,
 
     if (state->new_bs->backing != NULL) {
         error_setg(errp, "The snapshot already has a backing image");
+        return;
+    }
+
+    if (!state->new_bs->drv->supports_backing) {
+        error_setg(errp, "The snapshot does not support backing images");
     }
 }
 
-- 
2.6.2

Re: [Qemu-devel] [PATCH v3 1/3] block: Disallow snapshots if the overlay doesn't support backing files

[Eric Blake]

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

On 11/03/2015 03:32 AM, Alberto Garcia wrote:
> This addresses scenarios like this one:
> 
>   { 'execute': 'blockdev-add', 'arguments':
>     { 'options': { 'driver': 'qcow2',
>                    'node-name': 'new0',
>                    'file': { 'driver': 'file',
>                              'filename': 'new.qcow2',
>                              'node-name': 'file0' } } } }
> 
>   { 'execute': 'blockdev-snapshot', 'arguments':
>     { 'node': 'virtio0',
>       'overlay': 'file0' } }
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  blockdev.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 

> +++ b/blockdev.c
> @@ -1667,6 +1667,11 @@ static void external_snapshot_prepare(BlkTransactionState *common,
>  
>      if (state->new_bs->backing != NULL) {
>          error_setg(errp, "The snapshot already has a backing image");
> +        return;
> +    }
> +
> +    if (!state->new_bs->drv->supports_backing) {
> +        error_setg(errp, "The snapshot does not support backing images");

If we do s/snapshot/overlay/ here, the error message will make more
sense (I noticed it in 3/3).

My R-b stands either way, though.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

[Qemu-devel] [PATCH v3 2/3] block: Remove inner quotation marks in iotest 085

[Alberto Garcia]

This patch removes the inner quotation marks in all cases like this:

   cmd=" ... "${variable}" ... "

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/085 | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index 9484117..80e547d 100755
--- a/tests/qemu-iotests/085
+++ b/tests/qemu-iotests/085
@@ -65,7 +65,7 @@ function create_single_snapshot()
 {
     cmd="{ 'execute': 'blockdev-snapshot-sync',
                       'arguments': { 'device': 'virtio0',
-                                     'snapshot-file':'"${TEST_DIR}/${1}-${snapshot_virt0}"',
+                                     'snapshot-file':'${TEST_DIR}/${1}-${snapshot_virt0}',
                                      'format': 'qcow2' } }"
     _send_qemu_cmd $h "${cmd}" "return"
 }
@@ -77,10 +77,10 @@ function create_group_snapshot()
            {'actions': [
                { 'type': 'blockdev-snapshot-sync', 'data' :
                    { 'device': 'virtio0',
-                      'snapshot-file': '"${TEST_DIR}/${1}-${snapshot_virt0}"' } },
+                      'snapshot-file': '${TEST_DIR}/${1}-${snapshot_virt0}' } },
                { 'type': 'blockdev-snapshot-sync', 'data' :
                    { 'device': 'virtio1',
-                       'snapshot-file': '"${TEST_DIR}/${1}-${snapshot_virt1}"' } } ]
+                       'snapshot-file': '${TEST_DIR}/${1}-${snapshot_virt1}' } } ]
              } }"
 
     _send_qemu_cmd $h "${cmd}" "return"
@@ -101,9 +101,9 @@ function add_snapshot_image()
     mv "${TEST_IMG}" "${snapshot_file}"
     cmd="{ 'execute': 'blockdev-add', 'arguments':
            { 'options':
-             { 'driver': 'qcow2', 'node-name': 'snap_"${1}"', "${extra_params}"
+             { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${extra_params}
                'file':
-               { 'driver': 'file', 'filename': '"${snapshot_file}"' } } } }"
+               { 'driver': 'file', 'filename': '${snapshot_file}' } } } }"
     _send_qemu_cmd $h "${cmd}" "return"
 }
 
@@ -113,7 +113,7 @@ function blockdev_snapshot()
 {
     cmd="{ 'execute': 'blockdev-snapshot',
                       'arguments': { 'node': 'virtio0',
-                                     'overlay':'snap_"${1}"' } }"
+                                     'overlay':'snap_${1}' } }"
     _send_qemu_cmd $h "${cmd}" "${2:-return}"
 }
 
@@ -152,7 +152,7 @@ echo === Invalid command - missing device and nodename ===
 echo
 
 _send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot-sync',
-                         'arguments': { 'snapshot-file':'"${TEST_DIR}/1-${snapshot_virt0}"',
+                         'arguments': { 'snapshot-file':'${TEST_DIR}/1-${snapshot_virt0}',
                                      'format': 'qcow2' } }" "error"
 
 echo
@@ -224,7 +224,7 @@ blockdev_snapshot $((${SNAPSHOTS}+1)) error
 
 _send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot',
                      'arguments': { 'node':'nodevice',
-                                    'overlay':'snap_"${SNAPSHOTS}"' }
+                                    'overlay':'snap_${SNAPSHOTS}' }
                    }" "error"
 
 # success, all done
-- 
2.6.2

Re: [Qemu-devel] [PATCH v3 2/3] block: Remove inner quotation marks in iotest 085

[Eric Blake]

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

On 11/03/2015 03:32 AM, Alberto Garcia wrote:
> This patch removes the inner quotation marks in all cases like this:
> 
>    cmd=" ... "${variable}" ... "
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  tests/qemu-iotests/085 | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

I might have worded the commit message differently, though:

block: Remove incorrect "" in iotest 085

We had the patterns:
   cmd="..."${variable}"..."
   _send_qemu_cmd ... "..."${variable}"..."

which is equivalent to using ${variable} unquoted.  In the cmd= usage,
that happened to be okay even though it is unusual (because no word
splitting occurs on variable assignment); but where the usage appeared
as an argument to _send_qemu_cmd, it was actively wrong (any whitespace
in ${variable} would have caused word splitting).

Fix it by removing the inner "", leaving ${variable} to be expanded
inside the outer "" as desired.


> @@ -152,7 +152,7 @@ echo === Invalid command - missing device and nodename ===
>  echo
>  
>  _send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot-sync',
> -                         'arguments': { 'snapshot-file':'"${TEST_DIR}/1-${snapshot_virt0}"',
> +                         'arguments': { 'snapshot-file':'${TEST_DIR}/1-${snapshot_virt0}',
>                                       'format': 'qcow2' } }" "error"
>  

Here's an example of the actual bug being fixed.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 2/3] block: Remove inner quotation marks in iotest 085

[Alberto Garcia]

On Tue 03 Nov 2015 04:12:44 PM CET, Eric Blake wrote:
> On 11/03/2015 03:32 AM, Alberto Garcia wrote:
>> This patch removes the inner quotation marks in all cases like this:
>> 
>>    cmd=" ... "${variable}" ... "
>> 
>> Signed-off-by: Alberto Garcia <berto@igalia.com>
>> ---
>>  tests/qemu-iotests/085 | 16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> I might have worded the commit message differently, though:
>
> block: Remove incorrect "" in iotest 085
>
> We had the patterns:
>    cmd="..."${variable}"..."
>    _send_qemu_cmd ... "..."${variable}"..."
>
> which is equivalent to using ${variable} unquoted.  In the cmd= usage,
> that happened to be okay even though it is unusual (because no word
> splitting occurs on variable assignment); but where the usage appeared
> as an argument to _send_qemu_cmd, it was actively wrong (any whitespace
> in ${variable} would have caused word splitting).

You're right, I overlooked the _send_qemu_cmd case !

I'll rewrite the message if there's a new version of the series.

Berto

[Qemu-devel] [PATCH v3 3/3] block: test 'blockdev-snapshot' using a file BDS as the overlay

[Alberto Garcia]

This test checks that it is not possible to create a snapshot if the
requested overlay node is a BDS which does not support backing images.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/085     | 12 +++++++++++-
 tests/qemu-iotests/085.out |  4 ++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index 80e547d..aa77eca 100755
--- a/tests/qemu-iotests/085
+++ b/tests/qemu-iotests/085
@@ -103,7 +103,8 @@ function add_snapshot_image()
            { 'options':
              { 'driver': 'qcow2', 'node-name': 'snap_${1}', ${extra_params}
                'file':
-               { 'driver': 'file', 'filename': '${snapshot_file}' } } } }"
+               { 'driver': 'file', 'filename': '${snapshot_file}',
+                 'node-name': 'file_${1}' } } } }"
     _send_qemu_cmd $h "${cmd}" "return"
 }
 
@@ -187,6 +188,15 @@ add_snapshot_image ${SNAPSHOTS}
 blockdev_snapshot ${SNAPSHOTS}
 
 echo
+echo === Invalid command - cannot create a snapshot using a file BDS ===
+echo
+
+_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot',
+                     'arguments': { 'node':'virtio0',
+                                    'overlay':'file_${SNAPSHOTS}' }
+                   }" "error"
+
+echo
 echo === Invalid command - snapshot node used as active layer ===
 echo
 
diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out
index 52292ea..01c78d6 100644
--- a/tests/qemu-iotests/085.out
+++ b/tests/qemu-iotests/085.out
@@ -62,6 +62,10 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/
 {"return": {}}
 {"return": {}}
 
+=== Invalid command - cannot create a snapshot using a file BDS ===
+
+{"error": {"class": "GenericError", "desc": "The snapshot does not support backing images"}}
+
 === Invalid command - snapshot node used as active layer ===
 
 {"error": {"class": "GenericError", "desc": "The snapshot is already in use by virtio0"}}
-- 
2.6.2

Re: [Qemu-devel] [PATCH v3 3/3] block: test 'blockdev-snapshot' using a file BDS as the overlay

[Eric Blake]

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

On 11/03/2015 03:32 AM, Alberto Garcia wrote:
> This test checks that it is not possible to create a snapshot if the
> requested overlay node is a BDS which does not support backing images.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  tests/qemu-iotests/085     | 12 +++++++++++-
>  tests/qemu-iotests/085.out |  4 ++++
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 

> +++ b/tests/qemu-iotests/085.out
> @@ -62,6 +62,10 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/
>  {"return": {}}
>  {"return": {}}
>  
> +=== Invalid command - cannot create a snapshot using a file BDS ===
> +
> +{"error": {"class": "GenericError", "desc": "The snapshot does not support backing images"}}

This message could use improvement; more on that in 1/3.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v3 0/3] Disallow snapshots if the overlay doesn't support backing files

[Kevin Wolf]

Am 03.11.2015 um 11:32 hat Alberto Garcia geschrieben:
> We are currently allowing snapshots in cases like this one:
> 
>        { 'execute': 'blockdev-add', 'arguments':
>          { 'options': { 'driver': 'qcow2',
>                         'node-name': 'new0',
>                         'file': { 'driver': 'file',
>                                   'filename': 'new.qcow2',
>                                   'node-name': 'file0' } } } }
> 
>        { 'execute': 'blockdev-snapshot', 'arguments':
>          { 'node': 'virtio0',
>            'overlay': 'file0' } }
> 
> This patch forbids snapshots if the overlay node does not support
> backing files.

Thanks, applied to the block branch.

Kevin