* [Qemu-devel] [PATCH v2 0/2] block/drive-mirror: Check for NULL backing_hd
@ 2013-11-05 0:35 Max Reitz
2013-11-05 0:35 ` [Qemu-devel] [PATCH v2 1/2] " Max Reitz
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Max Reitz @ 2013-11-05 0:35 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Max Reitz
It should be possible to execute the QMP "drive-mirror" command in
"none" sync mode and "absolute-paths" mode even for block devices
lacking a backing file.
"absolute-paths" does in fact not require a backing file to be present,
as can be seen from the "top" sync mode code path. "top" basically
states that the device should indeed have a backing file - however, the
current code catches the case if it doesn't and then simply treats it as
"full" sync mode, creating a target image without a backing file (in
"absolute-paths" mode). Thus, "absolute-paths" does not imply the target
file must indeed have a backing file.
Therefore, the target file may be left unbacked in case of "none" sync
mode as well, if the specified device is not backed either. Currently,
qemu will crash trying to dereference the backing file pointer since it
assumes that it will always be non-NULL in that case ("none" with
"absolute-paths").
The first patch in this series adds a check whether the specified block
device is backed or not (creating an unbacked target image, if required);
the second patch adds a test case for mirroring unbacked block devices.
Max Reitz (2):
block/drive-mirror: Check for NULL backing_hd
qemu-iotests: Add test for unbacked mirroring
blockdev.c | 4 +-
tests/qemu-iotests/070 | 91 ++++++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/070.out | 33 +++++++++++++++++
tests/qemu-iotests/group | 1 +
4 files changed, 127 insertions(+), 2 deletions(-)
create mode 100755 tests/qemu-iotests/070
create mode 100644 tests/qemu-iotests/070.out
--
1.8.4.2
^ permalink raw reply [flat|nested] 8+ messages in thread* [Qemu-devel] [PATCH v2 1/2] block/drive-mirror: Check for NULL backing_hd 2013-11-05 0:35 [Qemu-devel] [PATCH v2 0/2] block/drive-mirror: Check for NULL backing_hd Max Reitz @ 2013-11-05 0:35 ` Max Reitz 2013-11-05 3:32 ` Fam Zheng 2013-11-05 0:35 ` [Qemu-devel] [PATCH v2 2/2] qemu-iotests: Add test for unbacked mirroring Max Reitz 2013-11-05 10:53 ` [Qemu-devel] [PATCH v2 0/2] block/drive-mirror: Check for NULL backing_hd Paolo Bonzini 2 siblings, 1 reply; 8+ messages in thread From: Max Reitz @ 2013-11-05 0:35 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Max Reitz It should be possible to execute the QMP "drive-mirror" command in "none" sync mode and "absolute-paths" mode even for block devices lacking a backing file. "absolute-paths" does in fact not require a backing file to be present, as can be seen from the "top" sync mode code path. "top" basically states that the device should indeed have a backing file - however, the current code catches the case if it doesn't and then simply treats it as "full" sync mode, creating a target image without a backing file (in "absolute-paths" mode). Thus, "absolute-paths" does not imply the target file must indeed have a backing file. Therefore, the target file may be left unbacked in case of "none" sync mode as well, if the specified device is not backed either. Currently, qemu will crash trying to dereference the backing file pointer since it assumes that it will always be non-NULL in that case ("none" with "absolute-paths"). Signed-off-by: Max Reitz <mreitz@redhat.com> --- blockdev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/blockdev.c b/blockdev.c index b260477..986e59d 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2038,8 +2038,8 @@ void qmp_drive_mirror(const char *device, const char *target, case NEW_IMAGE_MODE_ABSOLUTE_PATHS: /* create new image with backing file */ bdrv_img_create(target, format, - source->filename, - source->drv->format_name, + source ? source->filename : NULL, + source ? source->drv->format_name : NULL, NULL, size, flags, &local_err, false); break; default: -- 1.8.4.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] block/drive-mirror: Check for NULL backing_hd 2013-11-05 0:35 ` [Qemu-devel] [PATCH v2 1/2] " Max Reitz @ 2013-11-05 3:32 ` Fam Zheng 2013-11-05 19:06 ` Max Reitz 0 siblings, 1 reply; 8+ messages in thread From: Fam Zheng @ 2013-11-05 3:32 UTC (permalink / raw) To: Max Reitz, qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi On 11/05/2013 08:35 AM, Max Reitz wrote: > It should be possible to execute the QMP "drive-mirror" command in > "none" sync mode and "absolute-paths" mode even for block devices > lacking a backing file. > > "absolute-paths" does in fact not require a backing file to be present, > as can be seen from the "top" sync mode code path. "top" basically > states that the device should indeed have a backing file - however, the > current code catches the case if it doesn't and then simply treats it as > "full" sync mode, creating a target image without a backing file (in > "absolute-paths" mode). Thus, "absolute-paths" does not imply the target > file must indeed have a backing file. > > Therefore, the target file may be left unbacked in case of "none" sync > mode as well, if the specified device is not backed either. Currently, > qemu will crash trying to dereference the backing file pointer since it > assumes that it will always be non-NULL in that case ("none" with > "absolute-paths"). > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > blockdev.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/blockdev.c b/blockdev.c > index b260477..986e59d 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2038,8 +2038,8 @@ void qmp_drive_mirror(const char *device, const char *target, > case NEW_IMAGE_MODE_ABSOLUTE_PATHS: > /* create new image with backing file */ > bdrv_img_create(target, format, > - source->filename, > - source->drv->format_name, > + source ? source->filename : NULL, > + source ? source->drv->format_name : NULL, > NULL, size, flags, &local_err, false); > break; > default: The code around here is: 2029 if (sync == MIRROR_SYNC_MODE_FULL && mode != NEW_IMAGE_MODE_EXISTING) { 2030 /* create new image w/o backing file */ 2031 assert(format && drv); 2032 bdrv_img_create(target, format, 2033 NULL, NULL, NULL, size, flags, &local_err, false); 2034 } else { 2035 switch (mode) { 2036 case NEW_IMAGE_MODE_EXISTING: 2037 break; 2038 case NEW_IMAGE_MODE_ABSOLUTE_PATHS: 2039 /* create new image with backing file */ 2040 bdrv_img_create(target, format, 2041 source->filename, 2042 source->drv->format_name, 2043 NULL, size, flags, &local_err, false); 2044 break; 2045 default: 2046 abort(); 2047 } 2048 } Why not update the if condition and reuse the branch, I think this is a better branching? Either should be fine, but in your change you should also update the comment in line 2039. Thanks, Fam ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/2] block/drive-mirror: Check for NULL backing_hd 2013-11-05 3:32 ` Fam Zheng @ 2013-11-05 19:06 ` Max Reitz 0 siblings, 0 replies; 8+ messages in thread From: Max Reitz @ 2013-11-05 19:06 UTC (permalink / raw) To: Fam Zheng, qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi On 05.11.2013 04:32, Fam Zheng wrote: > > On 11/05/2013 08:35 AM, Max Reitz wrote: >> It should be possible to execute the QMP "drive-mirror" command in >> "none" sync mode and "absolute-paths" mode even for block devices >> lacking a backing file. >> >> "absolute-paths" does in fact not require a backing file to be present, >> as can be seen from the "top" sync mode code path. "top" basically >> states that the device should indeed have a backing file - however, the >> current code catches the case if it doesn't and then simply treats it as >> "full" sync mode, creating a target image without a backing file (in >> "absolute-paths" mode). Thus, "absolute-paths" does not imply the target >> file must indeed have a backing file. >> >> Therefore, the target file may be left unbacked in case of "none" sync >> mode as well, if the specified device is not backed either. Currently, >> qemu will crash trying to dereference the backing file pointer since it >> assumes that it will always be non-NULL in that case ("none" with >> "absolute-paths"). >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> blockdev.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/blockdev.c b/blockdev.c >> index b260477..986e59d 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -2038,8 +2038,8 @@ void qmp_drive_mirror(const char *device, const >> char *target, >> case NEW_IMAGE_MODE_ABSOLUTE_PATHS: >> /* create new image with backing file */ >> bdrv_img_create(target, format, >> - source->filename, >> - source->drv->format_name, >> + source ? source->filename : NULL, >> + source ? source->drv->format_name : NULL, >> NULL, size, flags, &local_err, false); >> break; >> default: > The code around here is: > > 2029 if (sync == MIRROR_SYNC_MODE_FULL && mode != > NEW_IMAGE_MODE_EXISTING) { > 2030 /* create new image w/o backing file */ > 2031 assert(format && drv); > 2032 bdrv_img_create(target, format, > 2033 NULL, NULL, NULL, size, flags, > &local_err, false); > 2034 } else { > 2035 switch (mode) { > 2036 case NEW_IMAGE_MODE_EXISTING: > 2037 break; > 2038 case NEW_IMAGE_MODE_ABSOLUTE_PATHS: > 2039 /* create new image with backing file */ > 2040 bdrv_img_create(target, format, > 2041 source->filename, > 2042 source->drv->format_name, > 2043 NULL, size, flags, &local_err, false); > 2044 break; > 2045 default: > 2046 abort(); > 2047 } > 2048 } > > Why not update the if condition and reuse the branch, I think this is > a better branching? Either should be fine, but in your change you > should also update the comment in line 2039. Okay, I'll go for updating the condition. Max > Thanks, > Fam ^ permalink raw reply [flat|nested] 8+ messages in thread
* [Qemu-devel] [PATCH v2 2/2] qemu-iotests: Add test for unbacked mirroring 2013-11-05 0:35 [Qemu-devel] [PATCH v2 0/2] block/drive-mirror: Check for NULL backing_hd Max Reitz 2013-11-05 0:35 ` [Qemu-devel] [PATCH v2 1/2] " Max Reitz @ 2013-11-05 0:35 ` Max Reitz 2013-11-05 9:00 ` Wenchao Xia 2013-11-05 10:53 ` [Qemu-devel] [PATCH v2 0/2] block/drive-mirror: Check for NULL backing_hd Paolo Bonzini 2 siblings, 1 reply; 8+ messages in thread From: Max Reitz @ 2013-11-05 0:35 UTC (permalink / raw) To: qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi, Max Reitz Add a new test for mirroring unbacked images in "absolute-paths" mode. This should work, if possible, but most importantly, qemu should never crash. Signed-off-by: Max Reitz <mreitz@redhat.com> --- tests/qemu-iotests/070 | 91 ++++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/070.out | 33 +++++++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 125 insertions(+) create mode 100755 tests/qemu-iotests/070 create mode 100644 tests/qemu-iotests/070.out diff --git a/tests/qemu-iotests/070 b/tests/qemu-iotests/070 new file mode 100755 index 0000000..25ecf99 --- /dev/null +++ b/tests/qemu-iotests/070 @@ -0,0 +1,91 @@ +#!/bin/bash +# +# Test mirroring block device without backing file in absolute-paths mode +# +# Copyright (C) 2013 Red Hat, Inc. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. +# + +# creator +owner=mreitz@redhat.com + +seq="$(basename $0)" +echo "QA output created by $seq" + +here="$PWD" +tmp=/tmp/$$ +status=1 # failure is the default! + +_cleanup() +{ + _cleanup_test_img +} +trap "_cleanup; exit \$status" 0 1 2 3 15 + +# get standard environment, filters and checks +. ./common.rc +. ./common.filter + +_supported_fmt qed qcow2 qcow2 vmdk +_supported_proto file +_supported_os Linux + +function do_run_qemu() +{ + echo Testing: "$@" | _filter_imgfmt + $QEMU -nographic -qmp stdio -serial none "$@" + echo +} + +function run_qemu() +{ + # Filter out empty returns and the SHUTDOWN event, because these may occur + # interleaved with the block job events (in a non-deterministic manner). + # Also, filter out the "Formatting" message which is emitted when the target + # image is created - it contains format-specific information. + do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp |\ + grep -v '{"return": {}}' | grep -v '"event": "SHUTDOWN"' |\ + grep -v '^Formatting' +} + +size=128K + +_make_test_img $size + +for sync in full top none +do + +echo +echo "=== Mirroring non-backed image in absolute-paths mode with sync=$sync ===" +echo + +run_qemu -drive file="$TEST_IMG",format="$IMGFMT",if=none,id=disk \ + -device virtio-blk-pci,drive=disk,id=virtio0 <<EOF +{ "execute": "qmp_capabilities" } +{ "execute": "drive-mirror", + "arguments": { "device": "disk", "target": "$TEST_IMG.target", + "format": "$IMGFMT", "mode": "absolute-paths", + "sync": "$sync" } } +{ "execute": "quit" } +EOF + +rm -f $TEST_IMG.target + +done + +# success, all done +echo "*** done" +rm -f $seq.full +status=0 diff --git a/tests/qemu-iotests/070.out b/tests/qemu-iotests/070.out new file mode 100644 index 0000000..246b0f1 --- /dev/null +++ b/tests/qemu-iotests/070.out @@ -0,0 +1,33 @@ +QA output created by 070 +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072 + +=== Mirroring non-backed image in absolute-paths mode with sync=full === + +Testing: -drive file=TEST_DIR/t.IMGFMT,format=IMGFMT,if=none,id=disk -device virtio-blk-pci,drive=disk,id=virtio0 +QMP_VERSION +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "disk", "len": 131072, "offset": 131072, "speed": 0, "type": "mirror"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 131072, "offset": 131072, "speed": 0, "type": "mirror"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} + + +=== Mirroring non-backed image in absolute-paths mode with sync=top === + +Testing: -drive file=TEST_DIR/t.IMGFMT,format=IMGFMT,if=none,id=disk -device virtio-blk-pci,drive=disk,id=virtio0 +QMP_VERSION +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "disk", "len": 131072, "offset": 131072, "speed": 0, "type": "mirror"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 131072, "offset": 131072, "speed": 0, "type": "mirror"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} + + +=== Mirroring non-backed image in absolute-paths mode with sync=none === + +Testing: -drive file=TEST_DIR/t.IMGFMT,format=IMGFMT,if=none,id=disk -device virtio-blk-pci,drive=disk,id=virtio0 +QMP_VERSION +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "disk", "len": 131072, "offset": 131072, "speed": 0, "type": "mirror"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 131072, "offset": 131072, "speed": 0, "type": "mirror"}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} + +*** done diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index c57ff35..b18b241 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -75,3 +75,4 @@ 067 rw auto 068 rw auto 069 rw auto +070 rw auto -- 1.8.4.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] qemu-iotests: Add test for unbacked mirroring 2013-11-05 0:35 ` [Qemu-devel] [PATCH v2 2/2] qemu-iotests: Add test for unbacked mirroring Max Reitz @ 2013-11-05 9:00 ` Wenchao Xia 2013-11-05 19:08 ` Max Reitz 0 siblings, 1 reply; 8+ messages in thread From: Wenchao Xia @ 2013-11-05 9:00 UTC (permalink / raw) To: Max Reitz, qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi 于 2013/11/5 8:35, Max Reitz 写道: > Add a new test for mirroring unbacked images in "absolute-paths" mode. > This should work, if possible, but most importantly, qemu should never > crash. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > tests/qemu-iotests/070 | 91 ++++++++++++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/070.out | 33 +++++++++++++++++ > tests/qemu-iotests/group | 1 + > 3 files changed, 125 insertions(+) > create mode 100755 tests/qemu-iotests/070 > create mode 100644 tests/qemu-iotests/070.out > > diff --git a/tests/qemu-iotests/070 b/tests/qemu-iotests/070 > new file mode 100755 > index 0000000..25ecf99 > --- /dev/null > +++ b/tests/qemu-iotests/070 > @@ -0,0 +1,91 @@ > +#!/bin/bash > +# > +# Test mirroring block device without backing file in absolute-paths mode > +# > +# Copyright (C) 2013 Red Hat, Inc. > +# > +# This program is free software; you can redistribute it and/or modify > +# it under the terms of the GNU General Public License as published by > +# the Free Software Foundation; either version 2 of the License, or > +# (at your option) any later version. > +# > +# This program is distributed in the hope that it will be useful, > +# but WITHOUT ANY WARRANTY; without even the implied warranty of > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > +# GNU General Public License for more details. > +# > +# You should have received a copy of the GNU General Public License > +# along with this program. If not, see <http://www.gnu.org/licenses/>. > +# > + > +# creator > +owner=mreitz@redhat.com > + > +seq="$(basename $0)" > +echo "QA output created by $seq" > + > +here="$PWD" > +tmp=/tmp/$$ > +status=1 # failure is the default! > + > +_cleanup() > +{ > + _cleanup_test_img > +} > +trap "_cleanup; exit \$status" 0 1 2 3 15 > + > +# get standard environment, filters and checks > +. ./common.rc > +. ./common.filter > + > +_supported_fmt qed qcow2 qcow2 vmdk > +_supported_proto file > +_supported_os Linux > + > +function do_run_qemu() > +{ > + echo Testing: "$@" | _filter_imgfmt > + $QEMU -nographic -qmp stdio -serial none "$@" > + echo > +} > + > +function run_qemu() > +{ > + # Filter out empty returns and the SHUTDOWN event, because these may occur > + # interleaved with the block job events (in a non-deterministic manner). > + # Also, filter out the "Formatting" message which is emitted when the target > + # image is created - it contains format-specific information. > + do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp |\ > + grep -v '{"return": {}}' | grep -v '"event": "SHUTDOWN"' |\ > + grep -v '^Formatting' > +} > + > +size=128K > + > +_make_test_img $size > + > +for sync in full top none > +do > + > +echo > +echo "=== Mirroring non-backed image in absolute-paths mode with sync=$sync ===" > +echo > + > +run_qemu -drive file="$TEST_IMG",format="$IMGFMT",if=none,id=disk \ > + -device virtio-blk-pci,drive=disk,id=virtio0 <<EOF > +{ "execute": "qmp_capabilities" } > +{ "execute": "drive-mirror", > + "arguments": { "device": "disk", "target": "$TEST_IMG.target", > + "format": "$IMGFMT", "mode": "absolute-paths", > + "sync": "$sync" } } > +{ "execute": "quit" } > +EOF > + > +rm -f $TEST_IMG.target > + > +done > + > +# success, all done > +echo "*** done" > +rm -f $seq.full > +status=0 > diff --git a/tests/qemu-iotests/070.out b/tests/qemu-iotests/070.out > new file mode 100644 > index 0000000..246b0f1 > --- /dev/null > +++ b/tests/qemu-iotests/070.out > @@ -0,0 +1,33 @@ > +QA output created by 070 > +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072 > + > +=== Mirroring non-backed image in absolute-paths mode with sync=full === > + > +Testing: -drive file=TEST_DIR/t.IMGFMT,format=IMGFMT,if=none,id=disk -device virtio-blk-pci,drive=disk,id=virtio0 > +QMP_VERSION > +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "disk", "len": 131072, "offset": 131072, "speed": 0, "type": "mirror"}} > +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 131072, "offset": 131072, "speed": 0, "type": "mirror"}} > +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} > +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} > + > + > +=== Mirroring non-backed image in absolute-paths mode with sync=top === > + > +Testing: -drive file=TEST_DIR/t.IMGFMT,format=IMGFMT,if=none,id=disk -device virtio-blk-pci,drive=disk,id=virtio0 > +QMP_VERSION > +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "disk", "len": 131072, "offset": 131072, "speed": 0, "type": "mirror"}} > +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 131072, "offset": 131072, "speed": 0, "type": "mirror"}} > +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} > +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} > + > + > +=== Mirroring non-backed image in absolute-paths mode with sync=none === > + > +Testing: -drive file=TEST_DIR/t.IMGFMT,format=IMGFMT,if=none,id=disk -device virtio-blk-pci,drive=disk,id=virtio0 > +QMP_VERSION > +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "disk", "len": 131072, "offset": 131072, "speed": 0, "type": "mirror"}} > +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 131072, "offset": 131072, "speed": 0, "type": "mirror"}} > +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} > +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} > + > +*** done > diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group > index c57ff35..b18b241 100644 > --- a/tests/qemu-iotests/group > +++ b/tests/qemu-iotests/group > @@ -75,3 +75,4 @@ > 067 rw auto > 068 rw auto > 069 rw auto > +070 rw auto > The code seems fine, but why not use python code as 056? I think for cases that need start qemu, python is better since the infras is ready and easier to talk with qmp protocol. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] qemu-iotests: Add test for unbacked mirroring 2013-11-05 9:00 ` Wenchao Xia @ 2013-11-05 19:08 ` Max Reitz 0 siblings, 0 replies; 8+ messages in thread From: Max Reitz @ 2013-11-05 19:08 UTC (permalink / raw) To: Wenchao Xia, qemu-devel; +Cc: Kevin Wolf, Stefan Hajnoczi On 05.11.2013 10:00, Wenchao Xia wrote: > 于 2013/11/5 8:35, Max Reitz 写道: >> Add a new test for mirroring unbacked images in "absolute-paths" mode. >> This should work, if possible, but most importantly, qemu should never >> crash. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> tests/qemu-iotests/070 | 91 ++++++++++++++++++++++++++++++++++++++++++++++ >> tests/qemu-iotests/070.out | 33 +++++++++++++++++ >> tests/qemu-iotests/group | 1 + >> 3 files changed, 125 insertions(+) >> create mode 100755 tests/qemu-iotests/070 >> create mode 100644 tests/qemu-iotests/070.out >> >> diff --git a/tests/qemu-iotests/070 b/tests/qemu-iotests/070 >> new file mode 100755 >> index 0000000..25ecf99 >> --- /dev/null >> +++ b/tests/qemu-iotests/070 >> @@ -0,0 +1,91 @@ >> +#!/bin/bash >> +# >> +# Test mirroring block device without backing file in absolute-paths mode >> +# >> +# Copyright (C) 2013 Red Hat, Inc. >> +# >> +# This program is free software; you can redistribute it and/or modify >> +# it under the terms of the GNU General Public License as published by >> +# the Free Software Foundation; either version 2 of the License, or >> +# (at your option) any later version. >> +# >> +# This program is distributed in the hope that it will be useful, >> +# but WITHOUT ANY WARRANTY; without even the implied warranty of >> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> +# GNU General Public License for more details. >> +# >> +# You should have received a copy of the GNU General Public License >> +# along with this program. If not, see <http://www.gnu.org/licenses/>. >> +# >> + >> +# creator >> +owner=mreitz@redhat.com >> + >> +seq="$(basename $0)" >> +echo "QA output created by $seq" >> + >> +here="$PWD" >> +tmp=/tmp/$$ >> +status=1 # failure is the default! >> + >> +_cleanup() >> +{ >> + _cleanup_test_img >> +} >> +trap "_cleanup; exit \$status" 0 1 2 3 15 >> + >> +# get standard environment, filters and checks >> +. ./common.rc >> +. ./common.filter >> + >> +_supported_fmt qed qcow2 qcow2 vmdk >> +_supported_proto file >> +_supported_os Linux >> + >> +function do_run_qemu() >> +{ >> + echo Testing: "$@" | _filter_imgfmt >> + $QEMU -nographic -qmp stdio -serial none "$@" >> + echo >> +} >> + >> +function run_qemu() >> +{ >> + # Filter out empty returns and the SHUTDOWN event, because these may occur >> + # interleaved with the block job events (in a non-deterministic manner). >> + # Also, filter out the "Formatting" message which is emitted when the target >> + # image is created - it contains format-specific information. >> + do_run_qemu "$@" 2>&1 | _filter_testdir | _filter_qmp |\ >> + grep -v '{"return": {}}' | grep -v '"event": "SHUTDOWN"' |\ >> + grep -v '^Formatting' >> +} >> + >> +size=128K >> + >> +_make_test_img $size >> + >> +for sync in full top none >> +do >> + >> +echo >> +echo "=== Mirroring non-backed image in absolute-paths mode with sync=$sync ===" >> +echo >> + >> +run_qemu -drive file="$TEST_IMG",format="$IMGFMT",if=none,id=disk \ >> + -device virtio-blk-pci,drive=disk,id=virtio0 <<EOF >> +{ "execute": "qmp_capabilities" } >> +{ "execute": "drive-mirror", >> + "arguments": { "device": "disk", "target": "$TEST_IMG.target", >> + "format": "$IMGFMT", "mode": "absolute-paths", >> + "sync": "$sync" } } >> +{ "execute": "quit" } >> +EOF >> + >> +rm -f $TEST_IMG.target >> + >> +done >> + >> +# success, all done >> +echo "*** done" >> +rm -f $seq.full >> +status=0 >> diff --git a/tests/qemu-iotests/070.out b/tests/qemu-iotests/070.out >> new file mode 100644 >> index 0000000..246b0f1 >> --- /dev/null >> +++ b/tests/qemu-iotests/070.out >> @@ -0,0 +1,33 @@ >> +QA output created by 070 >> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072 >> + >> +=== Mirroring non-backed image in absolute-paths mode with sync=full === >> + >> +Testing: -drive file=TEST_DIR/t.IMGFMT,format=IMGFMT,if=none,id=disk -device virtio-blk-pci,drive=disk,id=virtio0 >> +QMP_VERSION >> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "disk", "len": 131072, "offset": 131072, "speed": 0, "type": "mirror"}} >> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 131072, "offset": 131072, "speed": 0, "type": "mirror"}} >> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} >> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} >> + >> + >> +=== Mirroring non-backed image in absolute-paths mode with sync=top === >> + >> +Testing: -drive file=TEST_DIR/t.IMGFMT,format=IMGFMT,if=none,id=disk -device virtio-blk-pci,drive=disk,id=virtio0 >> +QMP_VERSION >> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "disk", "len": 131072, "offset": 131072, "speed": 0, "type": "mirror"}} >> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 131072, "offset": 131072, "speed": 0, "type": "mirror"}} >> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} >> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} >> + >> + >> +=== Mirroring non-backed image in absolute-paths mode with sync=none === >> + >> +Testing: -drive file=TEST_DIR/t.IMGFMT,format=IMGFMT,if=none,id=disk -device virtio-blk-pci,drive=disk,id=virtio0 >> +QMP_VERSION >> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_READY", "data": {"device": "disk", "len": 131072, "offset": 131072, "speed": 0, "type": "mirror"}} >> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_JOB_COMPLETED", "data": {"device": "disk", "len": 131072, "offset": 131072, "speed": 0, "type": "mirror"}} >> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "ide1-cd0", "tray-open": true}} >> +{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "DEVICE_TRAY_MOVED", "data": {"device": "floppy0", "tray-open": true}} >> + >> +*** done >> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group >> index c57ff35..b18b241 100644 >> --- a/tests/qemu-iotests/group >> +++ b/tests/qemu-iotests/group >> @@ -75,3 +75,4 @@ >> 067 rw auto >> 068 rw auto >> 069 rw auto >> +070 rw auto >> > The code seems fine, but why not use python code as 056? I think for > cases that need start qemu, python is better since the infras is ready > and easier to talk with qmp protocol. Creating a bash script is so much easier. ;-) But, yes, you're right, a python script is probably the better way to go. I'll also take a look whether I can extend a current test case instead (as Paolo suggested). Max ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH v2 0/2] block/drive-mirror: Check for NULL backing_hd 2013-11-05 0:35 [Qemu-devel] [PATCH v2 0/2] block/drive-mirror: Check for NULL backing_hd Max Reitz 2013-11-05 0:35 ` [Qemu-devel] [PATCH v2 1/2] " Max Reitz 2013-11-05 0:35 ` [Qemu-devel] [PATCH v2 2/2] qemu-iotests: Add test for unbacked mirroring Max Reitz @ 2013-11-05 10:53 ` Paolo Bonzini 2 siblings, 0 replies; 8+ messages in thread From: Paolo Bonzini @ 2013-11-05 10:53 UTC (permalink / raw) To: Max Reitz; +Cc: Kevin Wolf, qemu-devel, Stefan Hajnoczi Il 05/11/2013 01:35, Max Reitz ha scritto: > It should be possible to execute the QMP "drive-mirror" command in > "none" sync mode and "absolute-paths" mode even for block devices > lacking a backing file. > > "absolute-paths" does in fact not require a backing file to be present, > as can be seen from the "top" sync mode code path. "top" basically > states that the device should indeed have a backing file - however, the > current code catches the case if it doesn't and then simply treats it as > "full" sync mode, creating a target image without a backing file (in > "absolute-paths" mode). Thus, "absolute-paths" does not imply the target > file must indeed have a backing file. > > Therefore, the target file may be left unbacked in case of "none" sync > mode as well, if the specified device is not backed either. Currently, > qemu will crash trying to dereference the backing file pointer since it > assumes that it will always be non-NULL in that case ("none" with > "absolute-paths"). > > The first patch in this series adds a check whether the specified block > device is backed or not (creating an unbacked target image, if required); > the second patch adds a test case for mirroring unbacked block devices. > > > Max Reitz (2): > block/drive-mirror: Check for NULL backing_hd > qemu-iotests: Add test for unbacked mirroring > > blockdev.c | 4 +- > tests/qemu-iotests/070 | 91 ++++++++++++++++++++++++++++++++++++++++++++++ > tests/qemu-iotests/070.out | 33 +++++++++++++++++ > tests/qemu-iotests/group | 1 + > 4 files changed, 127 insertions(+), 2 deletions(-) > create mode 100755 tests/qemu-iotests/070 > create mode 100644 tests/qemu-iotests/070.out > Patch 1 is fine. For patch 2, there are existing drive-mirror tests written in Python. I'll let the maintainers whether they are fine with a new test, or prefer to extend those with a new testcase. Paolo ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-11-05 19:08 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-11-05 0:35 [Qemu-devel] [PATCH v2 0/2] block/drive-mirror: Check for NULL backing_hd Max Reitz 2013-11-05 0:35 ` [Qemu-devel] [PATCH v2 1/2] " Max Reitz 2013-11-05 3:32 ` Fam Zheng 2013-11-05 19:06 ` Max Reitz 2013-11-05 0:35 ` [Qemu-devel] [PATCH v2 2/2] qemu-iotests: Add test for unbacked mirroring Max Reitz 2013-11-05 9:00 ` Wenchao Xia 2013-11-05 19:08 ` Max Reitz 2013-11-05 10:53 ` [Qemu-devel] [PATCH v2 0/2] block/drive-mirror: Check for NULL backing_hd Paolo Bonzini
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.