Disallow snapshots if the overlay doesn't support backing files
v3 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
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(-)
Signature · 1 line
2.6.2
PATCH v3 1/3 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.2
Re: [Qemu-devel] [PATCH v3 1/3] 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]>Reviewed-by Eric Blake <[email protected]>Reviewed-by Max Reitz <[email protected]>--- blockdev.c | 5 +++++ 1 file changed, 5 insertions(+)+++
b/blockdev.c
blockdev.c static void external_snapshot_prepare(BlkTransactionState *common, · line 1674
Quoted 8 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" );
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.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
PATCH v3 2/3 block: Remove inner quotation marks in iotest 085
In reply to Alberto GarciaThis patch removes the inner quotation marks in all cases like this:
cmd=" ... "${variable}" ... "--- tests/qemu-iotests/085 | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
tests/qemu-iotests/085 +8 -8
@@ -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
Signature · 1 line
2.6.2
Re: [Qemu-devel] [PATCH v3 2/3] block: Remove inner quotation marks in iotest 085
Quoted 8 lines · Alberto Garcia
This patch removes the inner quotation marks in all cases like this:
cmd=" ... "${variable}" ... "Signed-off-by Alberto Garcia <[email protected]>--- tests/qemu-iotests/085 | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
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.
tests/qemu-iotests/085 echo === Invalid command - missing device and nodename === · line 157
Quoted 8 lines
tests/qemu-iotests/085 +1
-1@@ -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.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Re: [Qemu-devel] [PATCH v3 2/3] block: Remove inner quotation marks in iotest 085
Quoted 17 lines · Eric Blake
Quoted 8 lines · Alberto Garcia
This patch removes the inner quotation marks in all cases like this:
cmd=" ... "${variable}" ... "Signed-off-by Alberto Garcia <[email protected]>--- tests/qemu-iotests/085 | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)Reviewed-by Eric Blake <[email protected]>I might have worded the commit message differently, though:
block: Remove incorrect "" in iotest 085
We had the patterns:
Quoted 8 lines
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
PATCH v3 3/3 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 if the requested overlay node is a BDS which 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.2
Re: [Qemu-devel] [PATCH v3 3/3] block: test 'blockdev-snapshot' using a file BDS as the overlay
Quoted 12 lines · 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 <[email protected]>--- tests/qemu-iotests/085 | 12 +++++++++++- tests/qemu-iotests/085.out | 4 ++++ 2 files changed, 15 insertions(+), 1 deletion(-)+++ b/tests/qemu-iotests/085.out
tests/qemu-iotests/085.out Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/ · line 67
Quoted 6 lines
tests/qemu-iotests/085.out +3
-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"}}
This message could use improvement; more on that in 1/3.
Signature · 2 lines
Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Kevin Wolf
AppliedAm 03.11.2015 um 11:32 hat Alberto Garcia geschrieben:
Quoted 13 lines
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' } }
Quoted 2 lines
This patch forbids snapshots if the overlay node does not support backing files.
Thanks, applied to the block branch.
Kevin