| Message ID | 20190927094242.11152-16-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iotests: Allow ./check -o data_file | expand |
On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote: > When using an external data file, there are no refcounts for data > clusters. We thus have to adjust the corruption test in this patch to > not be based around a data cluster allocation, but the L2 table > allocation (L2 tables are still refcounted with external data files). > > Doing so means this test works both with and without external data > files. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > tests/qemu-iotests/137 | 10 ++++++---- > tests/qemu-iotests/137.out | 4 +--- > 2 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137 > index 6cf2997577..dd3484205e 100755 > --- a/tests/qemu-iotests/137 > +++ b/tests/qemu-iotests/137 > @@ -138,14 +138,16 @@ $QEMU_IO \ > "$TEST_IMG" 2>&1 | _filter_qemu_io > > # The dirty bit must not be set > -$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features > +# (Filter the external data file bit) > +$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features \ > + | sed -e 's/0x4/0x0/' Maybe it is better to filter all the feature bits, but the dirty bit, since only it is needed here, so that when we start running tests with more features, we won't need to do this again? > > # Similarly we can test whether corruption detection has been enabled: > -# Create L1/L2, overwrite first entry in refcount block, allocate something. > +# Create L1, overwrite refcounts, force allocation of L2 by writing > +# data. > # Disabling the checks should fail, so the corruption must be detected. > _make_test_img 64M > -$QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io > -poke_file "$TEST_IMG" "$((0x20000))" "\x00\x00" > +poke_file "$TEST_IMG" "$((0x20000))" "\x00\x00\x00\x00\x00\x00\x00\x00" I am wondering if there is any better way to do this (regardless of this patch), Basically the above code pokes into the 3rd cluster on the disk I *think*, and I don't understand why it has to contain refcounts. Hmm... First cluster I can guess will have the header, 2nd cluster probably L1 table, and 3rd, refcounts? If so, the test should specify that it needs 64K clusters, because the day will come when we will need to test this as well, but I guess in a separate patch, > $QEMU_IO \ > -c "reopen -o overlap-check=none,lazy-refcounts=42" \ > -c "write 64k 64k" \ > diff --git a/tests/qemu-iotests/137.out b/tests/qemu-iotests/137.out > index 1c6569eb2c..bd5f76d604 100644 > --- a/tests/qemu-iotests/137.out > +++ b/tests/qemu-iotests/137.out > @@ -38,9 +38,7 @@ wrote 512/512 bytes at offset 0 > ./common.rc: Killed ( VALGRIND_QEMU="${VALGRIND_QEMU_IO}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" ) > incompatible_features 0x0 > Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 > -wrote 65536/65536 bytes at offset 0 > -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) > qemu-io: Parameter 'lazy-refcounts' expects 'on' or 'off' > -qcow2: Marking image as corrupt: Preventing invalid write on metadata (overlaps with qcow2_header); further corruption events will be suppressed > +qcow2: Marking image as corrupt: Preventing invalid allocation of L2 table at offset 0; further corruption events will be suppressed > write failed: Input/output error > *** done Best regards, Maxim Levitsky
On 29.09.19 18:38, Maxim Levitsky wrote: > On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote: >> When using an external data file, there are no refcounts for data >> clusters. We thus have to adjust the corruption test in this patch to >> not be based around a data cluster allocation, but the L2 table >> allocation (L2 tables are still refcounted with external data files). >> >> Doing so means this test works both with and without external data >> files. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> tests/qemu-iotests/137 | 10 ++++++---- >> tests/qemu-iotests/137.out | 4 +--- >> 2 files changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137 >> index 6cf2997577..dd3484205e 100755 >> --- a/tests/qemu-iotests/137 >> +++ b/tests/qemu-iotests/137 >> @@ -138,14 +138,16 @@ $QEMU_IO \ >> "$TEST_IMG" 2>&1 | _filter_qemu_io >> >> # The dirty bit must not be set >> -$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features >> +# (Filter the external data file bit) >> +$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features \ >> + | sed -e 's/0x4/0x0/' > Maybe it is better to filter all the feature bits, but the dirty bit, > since only it is needed here, so that when we start running tests with > more features, we won't need to do this again? I’d hate a filter s/[02468ace]$/no dirty bit/ though. >> >> # Similarly we can test whether corruption detection has been enabled: >> -# Create L1/L2, overwrite first entry in refcount block, allocate something. >> +# Create L1, overwrite refcounts, force allocation of L2 by writing >> +# data. >> # Disabling the checks should fail, so the corruption must be detected. >> _make_test_img 64M >> -$QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io >> -poke_file "$TEST_IMG" "$((0x20000))" "\x00\x00" >> +poke_file "$TEST_IMG" "$((0x20000))" "\x00\x00\x00\x00\x00\x00\x00\x00" > > I am wondering if there is any better way to do this (regardless of this patch), > Basically the above code pokes into the 3rd cluster on the disk I *think*, and I don't > understand why it has to contain refcounts. Hmm... > First cluster I can guess will have the header, 2nd cluster probably L1 table, and 3rd, refcounts? > If so, the test should specify that it needs 64K clusters, because the day will come when > we will need to test this as well, but I guess in a separate patch, When that day comes, a whole lot of other stuff will break, too. Max
On Mon, 2019-09-30 at 15:38 +0200, Max Reitz wrote: > On 29.09.19 18:38, Maxim Levitsky wrote: > > On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote: > > > When using an external data file, there are no refcounts for data > > > clusters. We thus have to adjust the corruption test in this patch to > > > not be based around a data cluster allocation, but the L2 table > > > allocation (L2 tables are still refcounted with external data files). > > > > > > Doing so means this test works both with and without external data > > > files. > > > > > > Signed-off-by: Max Reitz <mreitz@redhat.com> > > > --- > > > tests/qemu-iotests/137 | 10 ++++++---- > > > tests/qemu-iotests/137.out | 4 +--- > > > 2 files changed, 7 insertions(+), 7 deletions(-) > > > > > > diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137 > > > index 6cf2997577..dd3484205e 100755 > > > --- a/tests/qemu-iotests/137 > > > +++ b/tests/qemu-iotests/137 > > > @@ -138,14 +138,16 @@ $QEMU_IO \ > > > "$TEST_IMG" 2>&1 | _filter_qemu_io > > > > > > # The dirty bit must not be set > > > -$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features > > > +# (Filter the external data file bit) > > > +$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features \ > > > + | sed -e 's/0x4/0x0/' > > > > Maybe it is better to filter all the feature bits, but the dirty bit, > > since only it is needed here, so that when we start running tests with > > more features, we won't need to do this again? > > I’d hate a filter s/[02468ace]$/no dirty bit/ though. Nothing a helper function can't solve IMHO, I would convert this to a number, and then check bitwise the bit 2, assuming that is the dirty bit) Again, note that my approach to code is to make it as easy as possible for the next guy to change, so I am noticing such places. Eventually someone of us, will be that next guy. Then again, I don't mind leaving this as is, just noting this. > > > > > > > # Similarly we can test whether corruption detection has been enabled: > > > -# Create L1/L2, overwrite first entry in refcount block, allocate something. > > > +# Create L1, overwrite refcounts, force allocation of L2 by writing > > > +# data. > > > # Disabling the checks should fail, so the corruption must be detected. > > > _make_test_img 64M > > > -$QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io > > > -poke_file "$TEST_IMG" "$((0x20000))" "\x00\x00" > > > +poke_file "$TEST_IMG" "$((0x20000))" "\x00\x00\x00\x00\x00\x00\x00\x00" > > > > I am wondering if there is any better way to do this (regardless of this patch), > > Basically the above code pokes into the 3rd cluster on the disk I *think*, and I don't > > understand why it has to contain refcounts. Hmm... > > First cluster I can guess will have the header, 2nd cluster probably L1 table, and 3rd, refcounts? > > If so, the test should specify that it needs 64K clusters, because the day will come when > > we will need to test this as well, but I guess in a separate patch, > > When that day comes, a whole lot of other stuff will break, too. I guess so, won't argue about this one. Best regards, Maxim Levitsky > > Max >
On 30.09.19 15:46, Maxim Levitsky wrote: > On Mon, 2019-09-30 at 15:38 +0200, Max Reitz wrote: >> On 29.09.19 18:38, Maxim Levitsky wrote: >>> On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote: >>>> When using an external data file, there are no refcounts for data >>>> clusters. We thus have to adjust the corruption test in this patch to >>>> not be based around a data cluster allocation, but the L2 table >>>> allocation (L2 tables are still refcounted with external data files). >>>> >>>> Doing so means this test works both with and without external data >>>> files. >>>> >>>> Signed-off-by: Max Reitz <mreitz@redhat.com> >>>> --- >>>> tests/qemu-iotests/137 | 10 ++++++---- >>>> tests/qemu-iotests/137.out | 4 +--- >>>> 2 files changed, 7 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137 >>>> index 6cf2997577..dd3484205e 100755 >>>> --- a/tests/qemu-iotests/137 >>>> +++ b/tests/qemu-iotests/137 >>>> @@ -138,14 +138,16 @@ $QEMU_IO \ >>>> "$TEST_IMG" 2>&1 | _filter_qemu_io >>>> >>>> # The dirty bit must not be set >>>> -$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features >>>> +# (Filter the external data file bit) >>>> +$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features \ >>>> + | sed -e 's/0x4/0x0/' >>> >>> Maybe it is better to filter all the feature bits, but the dirty bit, >>> since only it is needed here, so that when we start running tests with >>> more features, we won't need to do this again? >> >> I’d hate a filter s/[02468ace]$/no dirty bit/ though. > Nothing a helper function can't solve IMHO, I would convert this to a number, > and then check bitwise the bit 2, assuming that is the dirty bit) > Again, note that my approach to code is to make it as easy as possible for the > next guy to change, so I am noticing such places. Eventually someone of us, > will be that next guy. Then again, I don't mind leaving this as is, just noting this. Again, my approach to tests is that they aren’t classical code. This is a very personal opinion, but I have found that tests that have the most ad-hoc code with the least function calls are the easiest to work with. Tests that have a whole lot of infrastructure and try to have nice code are a horror to work with because you first have to understand how they work. Tests should be simple, not complex. Some ad-hoc filters do not make them complex as long as it’s obvious what they do. Also, the correct approach here is not to do number crunching in bash. It is to change qcow2.py to emit more easily filterable information. Max
On Mon, 2019-09-30 at 15:57 +0200, Max Reitz wrote: > On 30.09.19 15:46, Maxim Levitsky wrote: > > On Mon, 2019-09-30 at 15:38 +0200, Max Reitz wrote: > > > On 29.09.19 18:38, Maxim Levitsky wrote: > > > > On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote: > > > > > When using an external data file, there are no refcounts for data > > > > > clusters. We thus have to adjust the corruption test in this patch to > > > > > not be based around a data cluster allocation, but the L2 table > > > > > allocation (L2 tables are still refcounted with external data files). > > > > > > > > > > Doing so means this test works both with and without external data > > > > > files. > > > > > > > > > > Signed-off-by: Max Reitz <mreitz@redhat.com> > > > > > --- > > > > > tests/qemu-iotests/137 | 10 ++++++---- > > > > > tests/qemu-iotests/137.out | 4 +--- > > > > > 2 files changed, 7 insertions(+), 7 deletions(-) > > > > > > > > > > diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137 > > > > > index 6cf2997577..dd3484205e 100755 > > > > > --- a/tests/qemu-iotests/137 > > > > > +++ b/tests/qemu-iotests/137 > > > > > @@ -138,14 +138,16 @@ $QEMU_IO \ > > > > > "$TEST_IMG" 2>&1 | _filter_qemu_io > > > > > > > > > > # The dirty bit must not be set > > > > > -$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features > > > > > +# (Filter the external data file bit) > > > > > +$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features \ > > > > > + | sed -e 's/0x4/0x0/' > > > > > > > > Maybe it is better to filter all the feature bits, but the dirty bit, > > > > since only it is needed here, so that when we start running tests with > > > > more features, we won't need to do this again? > > > > > > I’d hate a filter s/[02468ace]$/no dirty bit/ though. > > > > Nothing a helper function can't solve IMHO, I would convert this to a number, > > and then check bitwise the bit 2, assuming that is the dirty bit) > > Again, note that my approach to code is to make it as easy as possible for the > > next guy to change, so I am noticing such places. Eventually someone of us, > > will be that next guy. Then again, I don't mind leaving this as is, just noting this. > > Again, my approach to tests is that they aren’t classical code. > > This is a very personal opinion, but I have found that tests that have > the most ad-hoc code with the least function calls are the easiest to > work with. > > Tests that have a whole lot of infrastructure and try to have nice code > are a horror to work with because you first have to understand how they > work. I guess everything should be in balance, but I understand very well what you mean. > > Tests should be simple, not complex. Some ad-hoc filters do not make > them complex as long as it’s obvious what they do. Yea, but the point is that it makes it harder to test new features, that slightly change the output of the tests, and break various tests that hardcode unneeded things. Pretty much exactly what this patch series does, is to remove some of the ad-hoc stuff to make the tests work with a new feature. A bit more generic tests, might be able to reduce this work. Anyway I won't argue with this as well, its all matter of balance, both extremes are no doubt bad. > > > Also, the correct approach here is not to do number crunching in bash. > It is to change qcow2.py to emit more easily filterable information. 100% agree. > > Max > Best regards, Maxim Levitsky
diff --git a/tests/qemu-iotests/137 b/tests/qemu-iotests/137 index 6cf2997577..dd3484205e 100755 --- a/tests/qemu-iotests/137 +++ b/tests/qemu-iotests/137 @@ -138,14 +138,16 @@ $QEMU_IO \ "$TEST_IMG" 2>&1 | _filter_qemu_io # The dirty bit must not be set -$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features +# (Filter the external data file bit) +$PYTHON qcow2.py "$TEST_IMG" dump-header | grep incompatible_features \ + | sed -e 's/0x4/0x0/' # Similarly we can test whether corruption detection has been enabled: -# Create L1/L2, overwrite first entry in refcount block, allocate something. +# Create L1, overwrite refcounts, force allocation of L2 by writing +# data. # Disabling the checks should fail, so the corruption must be detected. _make_test_img 64M -$QEMU_IO -c "write 0 64k" "$TEST_IMG" | _filter_qemu_io -poke_file "$TEST_IMG" "$((0x20000))" "\x00\x00" +poke_file "$TEST_IMG" "$((0x20000))" "\x00\x00\x00\x00\x00\x00\x00\x00" $QEMU_IO \ -c "reopen -o overlap-check=none,lazy-refcounts=42" \ -c "write 64k 64k" \ diff --git a/tests/qemu-iotests/137.out b/tests/qemu-iotests/137.out index 1c6569eb2c..bd5f76d604 100644 --- a/tests/qemu-iotests/137.out +++ b/tests/qemu-iotests/137.out @@ -38,9 +38,7 @@ wrote 512/512 bytes at offset 0 ./common.rc: Killed ( VALGRIND_QEMU="${VALGRIND_QEMU_IO}" _qemu_proc_exec "${VALGRIND_LOGFILE}" "$QEMU_IO_PROG" $QEMU_IO_ARGS "$@" ) incompatible_features 0x0 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864 -wrote 65536/65536 bytes at offset 0 -64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) qemu-io: Parameter 'lazy-refcounts' expects 'on' or 'off' -qcow2: Marking image as corrupt: Preventing invalid write on metadata (overlaps with qcow2_header); further corruption events will be suppressed +qcow2: Marking image as corrupt: Preventing invalid allocation of L2 table at offset 0; further corruption events will be suppressed write failed: Input/output error *** done
When using an external data file, there are no refcounts for data clusters. We thus have to adjust the corruption test in this patch to not be based around a data cluster allocation, but the L2 table allocation (L2 tables are still refcounted with external data files). Doing so means this test works both with and without external data files. Signed-off-by: Max Reitz <mreitz@redhat.com> --- tests/qemu-iotests/137 | 10 ++++++---- tests/qemu-iotests/137.out | 4 +--- 2 files changed, 7 insertions(+), 7 deletions(-)