Re: [PATCH 15/18] iotests: Make 137 work with data_file

[Maxim Levitsky <mlevitsk@redhat.com>]

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
>