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

[Qemu-devel] [PATCH v2 0/2] 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

v2:
- 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 (2):
  block: Disallow snapshots if the overlay doesn't support backing files
  block: test 'blockdev-snapshot' using a file BDS as the overlay

 blockdev.c                 |  5 +++++
 tests/qemu-iotests/085     | 12 +++++++++++-
 tests/qemu-iotests/085.out |  4 ++++
 3 files changed, 20 insertions(+), 1 deletion(-)

-- 
2.6.1

[Qemu-devel] [PATCH v2 1/2] 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>
---
 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.1

[Qemu-devel] [PATCH v2 2/2] 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 using as
the overlay node a BDS that 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 9484117..ccde2ae 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.1

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

[Eric Blake]

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

On 11/02/2015 05:15 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>
> ---
>  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");
>      }
>  }

You could avoid the 'return' by doing:

if (state->new_bs->backing) {
    error_setg(...);
} else if (!state->new_bs->drv->supports_backing) {
    error_setg(...);
}

but I don't care enough to require a respin.

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 v2 1/2] block: Disallow snapshots if the overlay doesn't support backing files

[Max Reitz]

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

On 02.11.2015 13:15, 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>
> ---
>  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;

This is...

> +    }
> +
> +    if (!state->new_bs->drv->supports_backing) {
> +        error_setg(errp, "The snapshot does not support backing images");

...why a return statement might be good here, too.

With or without:

Reviewed-by: Max Reitz <mreitz@redhat.com>

>      }
>  }
>  
> 



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

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

[Max Reitz]

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

On 02.11.2015 13:15, Alberto Garcia wrote:
> This test checks that it is not possible to create a snapshot using as
> the overlay node a BDS that does not support backing images.

I don't think that works in English. I may be wrong, of course.

"a snapshot using a BDS that does not support backing images as the
overlay node", "a snapshot with the overlay node being a BDS that...",
"a snapshot using a BDS as the overlay node that...", or something like
that might work.

> 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 9484117..ccde2ae 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}"' } } } }"

Pre-existing, but do those "" actually do anything?

Since the latter is mainly out of curiosity, and because English too not
my mother language is, which is why I not the one be should, who himself
over that complains*:

Reviewed-by: Max Reitz <mreitz@redhat.com>

(Although I would indeed prefer the commit message to be parsable more
easily.)

*Man, writing that was hard.

>      _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"}}
> 



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

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

[Alberto Garcia]

On Mon 02 Nov 2015 05:11:46 PM CET, Eric Blake <eblake@redhat.com> wrote:
>> @@ -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");
>>      }
>>  }
>
> You could avoid the 'return' by doing:
>
> if (state->new_bs->backing) {
>     error_setg(...);
> } else if (!state->new_bs->drv->supports_backing) {
>     error_setg(...);
> }

There's three more checks immediately before these lines, so I would
have had to turn everything into a series of if / else if, which is a
bit more unreadable in this case in my opinion. That's why I decided to
leave it like it is in the patch.

Berto

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

[Eric Blake]

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

On 11/02/2015 10:07 AM, Max Reitz wrote:
> On 02.11.2015 13:15, Alberto Garcia wrote:
>> This test checks that it is not possible to create a snapshot using as
>> the overlay node a BDS that does not support backing images.
> 
> I don't think that works in English. I may be wrong, of course.
> 
> "a snapshot using a BDS that does not support backing images as the
> overlay node", "a snapshot with the overlay node being a BDS that...",
> "a snapshot using a BDS as the overlay node that...", or something like
> that might work.
> 

How about:

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.

>> +++ 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}"' } } } }"
> 
> Pre-existing, but do those "" actually do anything?
> 

Actually, the "" are wrong.  Look at the full context: we have:

cmd="..."${snapshot_file}"..."

which means the expansion of $snapshot_file is _unquoted_.  We really
want either:

cmd='...'"${snapshot_file}"'...'

(if we wanted to write the command with " instead of ' for internal
string quoting), or:

cmd="...${snapshot_file}..."


I suspect that it crept in because locally we have ' in the ..., and
'${...}' in isolation is usually wrong (which is why you have to look at
the full string, and not just the local change).

> Since the latter is mainly out of curiosity, and because English too not
> my mother language is, which is why I not the one be should, who himself
> over that complains*:

LOL

-- 
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 v2 2/2] block: test 'blockdev-snapshot' using a file BDS as the overlay

[Alberto Garcia]

On Mon 02 Nov 2015 06:29:14 PM CET, Eric Blake <eblake@redhat.com> wrote:
>>> @@ -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}"' } } } }"
>> 
>> Pre-existing, but do those "" actually do anything?
>> 
>
> Actually, the "" are wrong.  Look at the full context: we have:
>
> cmd="..."${snapshot_file}"..."
>
> which means the expansion of $snapshot_file is _unquoted_.

Not really, it's quoted in all cases:

   'node-name': 'snap_"${1}"'
   'filename':  '"${snapshot_file}"'
   'node-name': 'file_"${1}"'

But it's true that the double quotes don't do anything so I'll remove
them.

Berto