Re: [Qemu-devel] [PATCH 11/21] iotests: Prepare for refcount_width option

[Max Reitz]

On 2014-11-11 at 18:57, Eric Blake wrote:
> On 11/10/2014 06:45 AM, Max Reitz wrote:
> > Some tests do not work well with certain refcount widths (i.e. you
> > cannot create internal snapshots with refcount_width=1), so make those
> > widths unsupported.
> >
> > Furthermore, add another filter to _filter_img_create in common.filter
> > which filters out the refcount_width value.
> >
> > Signed-off-by: Max Reitz <address@hidden>
> > ---
> >   tests/qemu-iotests/007           |  4 ++++
> >   tests/qemu-iotests/015           |  1 +
> >   tests/qemu-iotests/026           | 11 +++++++++++
> >   tests/qemu-iotests/029           |  1 +
> >   tests/qemu-iotests/051           |  1 +
> >   tests/qemu-iotests/058           |  1 +
> >   tests/qemu-iotests/067           |  7 +++++++
> >   tests/qemu-iotests/079           |  1 +
> >   tests/qemu-iotests/080           |  1 +
> >   tests/qemu-iotests/089           |  7 +++++++
> >   tests/qemu-iotests/090           |  1 +
> >   tests/qemu-iotests/108           |  6 ++++++
> >   tests/qemu-iotests/common.filter |  3 ++-
> >   13 files changed, 44 insertions(+), 1 deletion(-)
> >
> > diff --git a/tests/qemu-iotests/007 b/tests/qemu-iotests/007
> > index fe1a743..de39d1b 100755
> > --- a/tests/qemu-iotests/007
> > +++ b/tests/qemu-iotests/007
> > @@ -43,6 +43,10 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> >   _supported_fmt qcow2
> >   _supported_proto generic
> >   _supported_os Linux
> > +# refcount_width must be at least 4 bits so we can create ten internal 
> > snapshots
> > +# (1 bit supports none, 2 bits support three, 4 bits support 15)
> Feels like an off-by-one comment.  A width of 1 bit support a max
> refcount of 1 (therefore no snapshots), a width of 2 bits supports a max
> refcount of 3 (therefore 2 snapshots in addition to the original), a
> width of 4 bits supports a max refcount of 15 (therefore only 14 snapshots).

Telling you how to correctly get to the right maximum refcount and then 
getting it wrong myself is a bit embarrassing...

> > +++ b/tests/qemu-iotests/067
> > @@ -35,6 +35,13 @@ status=1     # failure is the default!
> >   _supported_fmt qcow2
> >   _supported_proto file
> >   _supported_os Linux
> > +# Because this would change the output of query-block
> > +_unsupported_imgopts 'refcount_width=1[^0-9]' \
> > +                     'refcount_width=2[^0-9]' \
> > +                     'refcount_width=4[^0-9]' \
> > +                     'refcount_width=8[^0-9]' \
> > +                     'refcount_width=32[^0-9]' \
> > +                     'refcount_width=64[^0-9]'
> It might be more compact to exploit globbing and just say:
>
> _unsupported_imgopts 'refcount_width=?[^6]'
>
> which leaves refcount_width=16 as the only pattern that doesn't match
> the glob.  But that feels more fragile, so I can live with your longer list.

Well, maybe using ?[^6] is even better because this isn't about ruling 
out the options 1, 2, 4, 8, 32 and 64, but rather only allowing 16. 
Thus, using ?[^6] seems more explicit.

> > +++ b/tests/qemu-iotests/089
> > @@ -41,6 +41,13 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> >   _supported_fmt qcow2
> >   _supported_proto file
> >   _supported_os Linux
> > +# Because this would change the output of qemu_io -c info
> > +_unsupported_imgopts 'refcount_width=1[^0-9]' \
> I like how you give reasons for some tests...
>
> > +++ b/tests/qemu-iotests/090
> > @@ -41,6 +41,7 @@ trap "_cleanup; exit \$status" 0 1 2 3 15
> >   _supported_fmt qcow2
> >   _supported_proto file nfs
> >   _supported_os Linux
> > +_unsupported_imgopts 'refcount_width=1[^0-9]'
> ...so why not do it for all tests?

Because the ones I didn't give reasons for are the ones I spotted first, 
so they seemed obvious to me. ;-) (I wondered the same thing when 
looking through the patches before submitting them, but decided to just 
leave it at that)

I'll add a comment for every test.

> At any rate, the patch makes sense, so whether or not you tweak comments,
>
> Reviewed-by: Eric Blake <address@hidden>
>
> I'm assuming that later in the series you add a test that explicitly
> covers the error message given when a refcount_order=0 (width=1) image
> is attempted to be used with snapshots, since that will fail (internal
> snapshots are simply not possible without a refcount that can't exceed 1).

Well, as you yourself explained, they are indeed possible if done right 
(immediate COW). But that'll go into another series.

Max