| Message ID | 1343218884-14980-8-git-send-email-stefanha@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 07/25/2012 06:21 AM, Stefan Hajnoczi wrote: > This tests establishes the basic post-conditions of the qcow2 lazy > refcounts features: > > 1. If the image was closed normally, it is marked clean. > > 2. If an allocating write was performed and the image was not close > normally, then it is marked dirty. > > a. Written data can be read back successfully. > b. The image file can be repaired and will be marked clean again. > > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > +++ b/tests/qemu-iotests/039 > @@ -0,0 +1,117 @@ > +#!/bin/bash Since you are assuming bash (and even if you were to assume POSIX /bin/sh)... > + > +seq=`basename $0` I prefer $() over ``. > +echo "QA output created by $seq" > + > +here=`pwd` POSIX (and therefore bash) guarantees that $PWD is sane, and faster to access than $(pwd). > +tmp=/tmp/$$ That's not very secure. It may be worth using bash's $RANDOM, or using mkstemp(1). Beyond that, the series seemed reasonable to me.
On Wed, Jul 25, 2012 at 11:54:50AM -0600, Eric Blake wrote: > On 07/25/2012 06:21 AM, Stefan Hajnoczi wrote: > > This tests establishes the basic post-conditions of the qcow2 lazy > > refcounts features: > > > > 1. If the image was closed normally, it is marked clean. > > > > 2. If an allocating write was performed and the image was not close > > normally, then it is marked dirty. > > > > a. Written data can be read back successfully. > > b. The image file can be repaired and will be marked clean again. > > > > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > > > +++ b/tests/qemu-iotests/039 > > @@ -0,0 +1,117 @@ > > +#!/bin/bash > > Since you are assuming bash (and even if you were to assume POSIX > /bin/sh)... > > > + > > +seq=`basename $0` > > I prefer $() over ``. > > > +echo "QA output created by $seq" > > + > > +here=`pwd` > > POSIX (and therefore bash) guarantees that $PWD is sane, and faster to > access than $(pwd). > > > +tmp=/tmp/$$ > > That's not very secure. It may be worth using bash's $RANDOM, or using > mkstemp(1). > > Beyond that, the series seemed reasonable to me. All qemu-iotests scripts do these things in the same way and I'd like for them to be consistent. If we make these changes they should be applied to all qemu-iotests scripts. I agree with your points but also think the value in making the change now is small. Do you want to send a patch that fixes these issues in qemu-iotests? The general shell scripting style used there is quite old school and makes use of backquotes often. Stefan
On 07/25/2012 04:45 PM, Stefan Hajnoczi wrote: >> Since you are assuming bash (and even if you were to assume POSIX >> /bin/sh)... >> >>> + >>> +seq=`basename $0` >> >> I prefer $() over ``. >> >>> +echo "QA output created by $seq" >>> + >>> +here=`pwd` >> >> POSIX (and therefore bash) guarantees that $PWD is sane, and faster to >> access than $(pwd). >> >>> +tmp=/tmp/$$ >> >> That's not very secure. It may be worth using bash's $RANDOM, or using >> mkstemp(1). >> >> Beyond that, the series seemed reasonable to me. > > All qemu-iotests scripts do these things in the same way and I'd like > for them to be consistent. Good argument. > > If we make these changes they should be applied to all qemu-iotests > scripts. I agree with your points but also think the value in making > the change now is small. Indeed - what you have is technically correct, even if not the most efficient. Any such cleanups should, as you say, be a separate patch globally applied to the qemu-iotests, and not this test in isolation. > > Do you want to send a patch that fixes these issues in qemu-iotests? Up to you; or read another way, it bothered me enough to comment, but not enough to write the patch myself, so I'm fine living with status quo if it doesn't bother anyone else either. Old-school techniques aren't wrong, per se, just inefficient; and while the insecure temp file name could be exploited, people running the testsuite tend to be on personal platforms rather than enterprise systems, and the cost of exploiting a testsuite is not as severe as the cost of exploiting an installed script.
Am 25.07.2012 14:21, schrieb Stefan Hajnoczi: > This tests establishes the basic post-conditions of the qcow2 lazy > refcounts features: > > 1. If the image was closed normally, it is marked clean. > > 2. If an allocating write was performed and the image was not close > normally, then it is marked dirty. > > a. Written data can be read back successfully. > b. The image file can be repaired and will be marked clean again. > > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> I think an important case that is missing here is opening a dirty image rw without having run qemu-img check -r first. > +== Read-only access must still work == > +read 512/512 bytes at offset 0 > +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > +incompatible_features 0x1 > + > +== Repairing the image file must succeed == > +ERROR OFLAG_COPIED: offset=8000000000050000 refcount=0 > +Repairing cluster 5 refcount=0 reference=1 > +No errors were found on the image. > +incompatible_features 0x0 I wonder what happened to the "The following inconsistencies were found and repaired" message. Most likely not a problem with qemu-iotests, though, but something unexpected in qemu-img. Kevin
On Thu, Jul 26, 2012 at 2:28 PM, Kevin Wolf <kwolf@redhat.com> wrote: > Am 25.07.2012 14:21, schrieb Stefan Hajnoczi: >> This tests establishes the basic post-conditions of the qcow2 lazy >> refcounts features: >> >> 1. If the image was closed normally, it is marked clean. >> >> 2. If an allocating write was performed and the image was not close >> normally, then it is marked dirty. >> >> a. Written data can be read back successfully. >> b. The image file can be repaired and will be marked clean again. >> >> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > > I think an important case that is missing here is opening a dirty image > rw without having run qemu-img check -r first. I have added that test case. >> +== Read-only access must still work == >> +read 512/512 bytes at offset 0 >> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) >> +incompatible_features 0x1 >> + >> +== Repairing the image file must succeed == >> +ERROR OFLAG_COPIED: offset=8000000000050000 refcount=0 >> +Repairing cluster 5 refcount=0 reference=1 >> +No errors were found on the image. >> +incompatible_features 0x0 > > I wonder what happened to the "The following inconsistencies were found > and repaired" message. Most likely not a problem with qemu-iotests, > though, but something unexpected in qemu-img. It's because opening a qcow2 image read/write when the dirty flag is set causes a repair. This accounts for the "Repairing cluster 5 ..." message. Then qemu-img check -r all calls bdrv_check() on an already repaired image file and we get the "No errors were found on the image". Stefan
Am 27.07.2012 09:56, schrieb Stefan Hajnoczi: > On Thu, Jul 26, 2012 at 2:28 PM, Kevin Wolf <kwolf@redhat.com> wrote: >> Am 25.07.2012 14:21, schrieb Stefan Hajnoczi: >>> +== Read-only access must still work == >>> +read 512/512 bytes at offset 0 >>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) >>> +incompatible_features 0x1 >>> + >>> +== Repairing the image file must succeed == >>> +ERROR OFLAG_COPIED: offset=8000000000050000 refcount=0 >>> +Repairing cluster 5 refcount=0 reference=1 >>> +No errors were found on the image. >>> +incompatible_features 0x0 >> >> I wonder what happened to the "The following inconsistencies were found >> and repaired" message. Most likely not a problem with qemu-iotests, >> though, but something unexpected in qemu-img. > > It's because opening a qcow2 image read/write when the dirty flag is > set causes a repair. This accounts for the "Repairing cluster 5 ..." > message. > > Then qemu-img check -r all calls bdrv_check() on an already repaired > image file and we get the "No errors were found on the image". I see. Not exactly how it was intended... Do we need a BDRV_O_CHECK flag that prevents the automatic repair or should we just live with the suboptimal output when lazy refcounting is enabled? Kevin
On Fri, Jul 27, 2012 at 9:07 AM, Kevin Wolf <kwolf@redhat.com> wrote: > Am 27.07.2012 09:56, schrieb Stefan Hajnoczi: >> On Thu, Jul 26, 2012 at 2:28 PM, Kevin Wolf <kwolf@redhat.com> wrote: >>> Am 25.07.2012 14:21, schrieb Stefan Hajnoczi: >>>> +== Read-only access must still work == >>>> +read 512/512 bytes at offset 0 >>>> +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) >>>> +incompatible_features 0x1 >>>> + >>>> +== Repairing the image file must succeed == >>>> +ERROR OFLAG_COPIED: offset=8000000000050000 refcount=0 >>>> +Repairing cluster 5 refcount=0 reference=1 >>>> +No errors were found on the image. >>>> +incompatible_features 0x0 >>> >>> I wonder what happened to the "The following inconsistencies were found >>> and repaired" message. Most likely not a problem with qemu-iotests, >>> though, but something unexpected in qemu-img. >> >> It's because opening a qcow2 image read/write when the dirty flag is >> set causes a repair. This accounts for the "Repairing cluster 5 ..." >> message. >> >> Then qemu-img check -r all calls bdrv_check() on an already repaired >> image file and we get the "No errors were found on the image". > > I see. Not exactly how it was intended... Do we need a BDRV_O_CHECK flag > that prevents the automatic repair or should we just live with the > suboptimal output when lazy refcounting is enabled? You noticed the issue so others might notice it too. I'll send a patch including qcow2 and qed changes to fix this using BDRV_O_CHECK. Stefan
diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039 new file mode 100755 index 0000000..4bf792f --- /dev/null +++ b/tests/qemu-iotests/039 @@ -0,0 +1,117 @@ +#!/bin/bash +# +# Test qcow2 lazy refcounts +# +# Copyright (C) 2012 Red Hat, Inc. +# Copyright IBM, Corp. 2010 +# +# Based on test 038. +# +# 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=stefanha@linux.vnet.ibm.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 qcow2 +_supported_proto generic +_supported_os Linux + +size=128M + +echo +echo "== Checking that image is clean on shutdown ==" + +IMGOPTS="compat=1.1,lazy_refcounts=on" +_make_test_img $size + +$QEMU_IO -c "write -P 0x5a 0 512" $TEST_IMG | _filter_qemu_io + +# The dirty bit must not be set +./qcow2.py $TEST_IMG dump-header | grep incompatible_features +_check_test_img + +echo +echo "== Creating a dirty image file ==" + +IMGOPTS="compat=1.1,lazy_refcounts=on" +_make_test_img $size + +old_ulimit=$(ulimit -c) +ulimit -c 0 # do not produce a core dump on abort(3) +$QEMU_IO -c "write -P 0x5a 0 512" -c "abort" $TEST_IMG | _filter_qemu_io +ulimit -c "$old_ulimit" + +# The dirty bit must be set +./qcow2.py $TEST_IMG dump-header | grep incompatible_features +_check_test_img + +echo +echo "== Read-only access must still work ==" + +$QEMU_IO -r -c "read -P 0x5a 0 512" $TEST_IMG | _filter_qemu_io + +# The dirty bit must be set +./qcow2.py $TEST_IMG dump-header | grep incompatible_features + +echo +echo "== Repairing the image file must succeed ==" + +$QEMU_IMG check -r all $TEST_IMG + +# The dirty bit must not be set +./qcow2.py $TEST_IMG dump-header | grep incompatible_features + +echo +echo "== Data should still be accessible after repair ==" + +$QEMU_IO -c "read -P 0x5a 0 512" $TEST_IMG | _filter_qemu_io + +echo +echo "== Creating an image file with lazy_refcounts=off ==" + +IMGOPTS="compat=1.1,lazy_refcounts=off" +_make_test_img $size + +old_ulimit=$(ulimit -c) +ulimit -c 0 # do not produce a core dump on abort(3) +$QEMU_IO -c "write -P 0x5a 0 512" -c "abort" $TEST_IMG | _filter_qemu_io +ulimit -c "$old_ulimit" + +# The dirty bit must not be set since lazy_refcounts=off +./qcow2.py $TEST_IMG dump-header | grep incompatible_features +_check_test_img + +# success, all done +echo "*** done" +rm -f $seq.full +status=0 + diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out new file mode 100644 index 0000000..6be47c0 --- /dev/null +++ b/tests/qemu-iotests/039.out @@ -0,0 +1,42 @@ +QA output created by 039 + +== Checking that image is clean on shutdown == +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 lazy_refcounts=on +wrote 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +incompatible_features 0x0 +No errors were found on the image. + +== Creating a dirty image file == +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 lazy_refcounts=on +wrote 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +incompatible_features 0x1 +ERROR OFLAG_COPIED: offset=8000000000050000 refcount=0 +ERROR cluster 5 refcount=0 reference=1 + +2 errors were found on the image. +Data may be corrupted, or further writes to the image may corrupt it. + +== Read-only access must still work == +read 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +incompatible_features 0x1 + +== Repairing the image file must succeed == +ERROR OFLAG_COPIED: offset=8000000000050000 refcount=0 +Repairing cluster 5 refcount=0 reference=1 +No errors were found on the image. +incompatible_features 0x0 + +== Data should still be accessible after repair == +read 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) + +== Creating an image file with lazy_refcounts=off == +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 +wrote 512/512 bytes at offset 0 +512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) +incompatible_features 0x0 +No errors were found on the image. +*** done diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group index 7a2c92b..ebb5ca4 100644 --- a/tests/qemu-iotests/group +++ b/tests/qemu-iotests/group @@ -45,3 +45,4 @@ 036 rw auto quick 037 rw auto backing 038 rw auto backing +039 rw auto
This tests establishes the basic post-conditions of the qcow2 lazy refcounts features: 1. If the image was closed normally, it is marked clean. 2. If an allocating write was performed and the image was not close normally, then it is marked dirty. a. Written data can be read back successfully. b. The image file can be repaired and will be marked clean again. Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> --- tests/qemu-iotests/039 | 117 ++++++++++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/039.out | 42 ++++++++++++++++ tests/qemu-iotests/group | 1 + 3 files changed, 160 insertions(+) create mode 100755 tests/qemu-iotests/039 create mode 100644 tests/qemu-iotests/039.out