Disallow snapshots if the overlay doesn't support backing files
v2 cover Disallow snapshots if the overlay doesn't support backing files
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(-)
Signature · 1 line
2.6.1
PATCH v2 1/2 block: Disallow snapshots if the overlay doesn't support backing files
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' } }--- blockdev.c | 5 +++++ 1 file changed, 5 insertions(+)
blockdev.c +5 -0
@@ -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" ); } }
Signature · 1 line
2.6.1
Re: [Qemu-devel] [PATCH v2 1/2] block: Disallow snapshots if the overlay doesn't support backing files
Quoted 23 lines · 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 <[email protected]>--- blockdev.c | 5 +++++ 1 file changed, 5 insertions(+)blockdev.c +0
-0
blockdev.c static void external_snapshot_prepare(BlkTransactionState *common, · line 1676
Quoted 10 lines
blockdev.c +5
-0@@ -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.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH v2 1/2] block: Disallow snapshots if the overlay doesn't support backing files
Quoted 14 lines · Eric Blake
blockdev.c static void external_snapshot_prepare(BlkTransactionState *common, · line 1676
Quoted 11 lines
blockdev.c +5
-0@@ -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:
Quoted 5 lines
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 1/2] block: Disallow snapshots if the overlay doesn't support backing files
In reply to Alberto GarciaQuoted 23 lines · 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 <[email protected]>--- blockdev.c | 5 +++++ 1 file changed, 5 insertions(+)blockdev.c +0
-0
blockdev.c static void external_snapshot_prepare(BlkTransactionState *common, · line 1670
Quoted 4 lines
blockdev.c +1
-0@@ -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...
blockdev.c static void external_snapshot_prepare(BlkTransactionState *common, · line 1674
Quoted 4 lines
blockdev.c +4
-0+ } + +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:
Quoted 4 lines
blockdev.c +0
-0} }
PATCH v2 2/2 block: test 'blockdev-snapshot' using a file BDS as the overlay
In reply to Alberto GarciaThis test checks that it is not possible to create a snapshot using as the overlay node a BDS that does not support backing images.
--- tests/qemu-iotests/085 | 12 +++++++++++- tests/qemu-iotests/085.out | 4 ++++ 2 files changed, 15 insertions(+), 1 deletion(-)
tests/qemu-iotests/085 +11 -1
@@ -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
tests/qemu-iotests/085.out +4 -0
@@ -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"}}
Signature · 1 line
2.6.1
Re: [Qemu-devel] [PATCH v2 2/2] block: test 'blockdev-snapshot' using a file BDS as the overlay
Quoted 2 lines · 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.
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.
Quoted 11 lines
Signed-off-by Alberto Garcia <[email protected]>--- tests/qemu-iotests/085 | 12 +++++++++++- tests/qemu-iotests/085.out | 4 ++++ 2 files changed, 15 insertions(+), 1 deletion(-)tests/qemu-iotests/085 +0
-0
tests/qemu-iotests/085 function add_snapshot_image() · line 107
Quoted 6 lines
tests/qemu-iotests/085 +2
-1@@ -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*:
(Although I would indeed prefer the commit message to be parsable more easily.)
*Man, writing that was hard.
Quoted 35 lines
tests/qemu-iotests/085 +9
-0_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 === echotests/qemu-iotests/085.out +4
-0@@ -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"}}
Re: [Qemu-devel] [PATCH v2 2/2] block: test 'blockdev-snapshot' using a file BDS as the overlay
Quoted 11 lines · Max Reitz
Quoted 2 lines · 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.
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.
Quoted 11 lines
tests/qemu-iotests/085 function add_snapshot_image() · line 107
Quoted 8 lines
+++ b/tests/qemu-iotests/085
tests/qemu-iotests/085 +2
-1@@ -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).
Quoted 3 lines
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
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH v2 2/2] block: test 'blockdev-snapshot' using a file BDS as the overlay
Quoted 11 lines · Eric Blake
Quoted 10 lines
tests/qemu-iotests/085 function add_snapshot_image() · line 107
Quoted 7 lines
tests/qemu-iotests/085 +2
-1@@ -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?
Quoted 5 lines
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