Re: [PATCH 01/18] iotests: Filter refcount_order in 036

[Maxim Levitsky]

On Mon, 2019-09-30 at 14:43 +0200, Max Reitz wrote:
> On 29.09.19 18:31, Maxim Levitsky wrote:
> > On Fri, 2019-09-27 at 11:42 +0200, Max Reitz wrote:
> > > This test can run just fine with other values for refcount_bits, so we
> > > should filter the value from qcow2.py's dump-header.
> > > 
> > > (036 currently ignores user-specified image options, but that will be
> > > fixed in the next patch.)
> > > 
> > > Signed-off-by: Max Reitz <address@hidden>
> > > ---
> > >  tests/qemu-iotests/036     | 9 ++++++---
> > >  tests/qemu-iotests/036.out | 6 +++---
> > >  2 files changed, 9 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/tests/qemu-iotests/036 b/tests/qemu-iotests/036
> > > index f06ff67408..69d0f9f903 100755
> > > --- a/tests/qemu-iotests/036
> > > +++ b/tests/qemu-iotests/036
> > > @@ -55,7 +55,8 @@ $PYTHON qcow2.py "$TEST_IMG" set-feature-bit 
> > > incompatible 63
> > >  
> > >  # Without feature table
> > >  $PYTHON qcow2.py "$TEST_IMG" del-header-ext 0x6803f857
> > > -$PYTHON qcow2.py "$TEST_IMG" dump-header
> > > +$PYTHON qcow2.py "$TEST_IMG" dump-header \
> > > +    | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/'
> > >  _img_info
> > >  
> > >  # With feature table containing bit 63
> > > @@ -103,14 +104,16 @@ echo === Create image with unknown autoclear 
> > > feature bit ===
> > >  echo
> > >  _make_test_img 64M
> > >  $PYTHON qcow2.py "$TEST_IMG" set-feature-bit autoclear 63
> > > -$PYTHON qcow2.py "$TEST_IMG" dump-header
> > > +$PYTHON qcow2.py "$TEST_IMG" dump-header \
> > > +    | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/'
> > >  
> > >  echo
> > >  echo === Repair image ===
> > >  echo
> > >  _check_test_img -r all
> > >  
> > > -$PYTHON qcow2.py "$TEST_IMG" dump-header
> > > +$PYTHON qcow2.py "$TEST_IMG" dump-header \
> > > +    | sed -e 's/^\(refcount_order\s*\).*/\1(filtered)/'
> > >  
> > >  # success, all done
> > >  echo "*** done"
> > > diff --git a/tests/qemu-iotests/036.out b/tests/qemu-iotests/036.out
> > > index e489b44386..998c2a8a35 100644
> > > --- a/tests/qemu-iotests/036.out
> > > +++ b/tests/qemu-iotests/036.out
> > > @@ -19,7 +19,7 @@ snapshot_offset           0x0
> > >  incompatible_features     0x8000000000000000
> > >  compatible_features       0x0
> > >  autoclear_features        0x0
> > > -refcount_order            4
> > > +refcount_order            (filtered)
> > >  header_length             104
> > >  
> > >  qemu-img: Could not open 'TEST_DIR/t.IMGFMT': Unsupported IMGFMT 
> > > feature(s): Unknown incompatible feature: 8000000000000000
> > > @@ -53,7 +53,7 @@ snapshot_offset           0x0
> > >  incompatible_features     0x0
> > >  compatible_features       0x0
> > >  autoclear_features        0x8000000000000000
> > > -refcount_order            4
> > > +refcount_order            (filtered)
> > >  header_length             104
> > >  
> > >  Header extension:
> > > @@ -81,7 +81,7 @@ snapshot_offset           0x0
> > >  incompatible_features     0x0
> > >  compatible_features       0x0
> > >  autoclear_features        0x0
> > > -refcount_order            4
> > > +refcount_order            (filtered)
> > >  header_length             104
> > >  
> > >  Header extension:
> > 
> > Minor notes:
> > 
> > 1. Maybe put the sed command into a function to avoid duplication?
> 
> Hm, maybe, but that would increase the LoC, so I’m not sure whether it
> really would be a simplification.
IMHO, in my opinion, regardless of LOC, duplicated code is almost always
bad. Common functions are mostly for the next guy that will be able to use
them instead of searching through code to see how this is done there and there.

> 
> > 2. As I understand the test, it only checks the feature bits.
> >    So maybe instead of blacklisting some of the values, white list
> >    only the feature bits in the output?
> 
> Hm.  Seems reasonable, but then again I’d prefer a minimal change, and
> changing it to a whitelist would be more change...
I don't think this is bad, again in my eyes, the code should be as friendly
as possible for the next guy that will have to change it, so adding
some more extra changes don't seem a problem to me.
Of course this is only my personal option and each approach has its own cons,
and pros. This doesn't matter that much to me.

Best regards,
        Maxim Levitsky