From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50899) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XoTV6-0008FS-32 for qemu-devel@nongnu.org; Wed, 12 Nov 2014 03:42:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XoTUz-0006RA-SD for qemu-devel@nongnu.org; Wed, 12 Nov 2014 03:42:04 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38062) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XoTUz-0006Qw-L6 for qemu-devel@nongnu.org; Wed, 12 Nov 2014 03:41:57 -0500 Message-ID: <54631D4B.200@redhat.com> Date: Wed, 12 Nov 2014 09:41:47 +0100 From: Max Reitz MIME-Version: 1.0 References: <1415627159-15941-1-git-send-email-mreitz@redhat.com> <1415627159-15941-12-git-send-email-mreitz@redhat.com> <54624E22.5060801@redhat.com> In-Reply-To: <54624E22.5060801@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 11/21] iotests: Prepare for refcount_width option List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: Kevin Wolf , Peter Lieven , Stefan Hajnoczi 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 >> --- >> 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 > > 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