From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37599) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xo6YY-0008Ax-Ld for qemu-devel@nongnu.org; Tue, 11 Nov 2014 03:12:11 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Xo6YS-00039c-GU for qemu-devel@nongnu.org; Tue, 11 Nov 2014 03:12:06 -0500 Received: from mx1.redhat.com ([209.132.183.28]:50804) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Xo6YS-00039T-9H for qemu-devel@nongnu.org; Tue, 11 Nov 2014 03:12:00 -0500 Message-ID: <5461C4C6.8020609@redhat.com> Date: Tue, 11 Nov 2014 09:11:50 +0100 From: Max Reitz MIME-Version: 1.0 References: <1415627159-15941-1-git-send-email-mreitz@redhat.com> <1415627159-15941-3-git-send-email-mreitz@redhat.com> <54610CCD.8010108@redhat.com> In-Reply-To: <54610CCD.8010108@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 02/21] qcow2: Add refcount_width to format-specific info 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-10 at 20:06, Eric Blake wrote: > On 11/10/2014 06:45 AM, Max Reitz wrote: >> Add the bit width of every refcount entry to the format-specific >> information. >> >> This breaks some test outputs, fix them. >> >> Signed-off-by: Max Reitz >> --- >> block/qcow2.c | 4 +++- >> qapi/block-core.json | 5 ++++- >> tests/qemu-iotests/060.out | 1 + >> tests/qemu-iotests/065 | 23 +++++++++++++++-------- >> tests/qemu-iotests/067.out | 10 +++++----- >> tests/qemu-iotests/082.out | 7 +++++++ >> tests/qemu-iotests/089.out | 2 ++ >> 7 files changed, 37 insertions(+), 15 deletions(-) >> >> diff --git a/block/qcow2.c b/block/qcow2.c >> index f57aff9..d70e927 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -2475,7 +2475,8 @@ static ImageInfoSpecific *qcow2_get_specific_inf= o(BlockDriverState *bs) >> }; >> if (s->qcow_version =3D=3D 2) { >> *spec_info->qcow2 =3D (ImageInfoSpecificQCow2){ >> - .compat =3D g_strdup("0.10"), >> + .compat =3D g_strdup("0.10"), >> + .refcount_width =3D s->refcount_bits, > Hmm - is it really worth displaying a constant? Since the 0.10 format > cannot change the width from 16, I'm not sure if it adds anything to th= e > output to display it. After all, there's other things we omit for the > old format when they cannot be altered (such as the state of a lazy > flag). On the other hand, if it makes your changes to later iotests > easier for tests that operate on both image formats, I'm not opposed to= it. Yes, I thought about not displaying it. But whereas "corrupt" or "lazy=20 refcounts" simply do not make sense with compat=3D0.10 images (it's simpl= y=20 impossible), the refcount width does make sense. It's always 16 bits=20 (I'm noticing myself how I keep swapping between "bit" and "bits", but I = just can't help it) but I personally find it interesting enough to=20 display. I'd be fine with dropping it from compat=3D0.10, though. But in retrospect, I'd rather make the other two flags always visible=20 than now drop this entry. However, not displaying a bool if it's always=20 false makes more sense to me than not displaying an integer because it's = always constant. >> +++ b/tests/qemu-iotests/065 >> @@ -88,34 +88,41 @@ class TestQMP(TestImageInfoSpecific): >> class TestQCow2(TestQemuImgInfo): >> '''Testing a qcow2 version 2 image''' >> img_options =3D 'compat=3D0.10' >> - json_compare =3D { 'compat': '0.10' } >> - human_compare =3D [ 'compat: 0.10' ] >> + json_compare =3D { 'compat': '0.10', 'refcount-width': 16 } >> + human_compare =3D [ 'compat: 0.10', 'refcount width: 16' ] > This would be a test that does not change if you decide to not output > the constant. > >> -{"return": [{"io-status": "ok", "device": "disk", "locked": false, "r= emovable": false, "inserted": {"iops_rd": 0, "detect_zeroes": "off", "ima= ge": {"virtual-size": 134217728, "filename": "TEST_DIR/t.qcow2", "cluster= -size": 65536, "format": "qcow2", "actual-size": SIZE, "format-specific":= {"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false, "co= rrupt": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false, "backin= g_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, "encrypted": fa= lse, "bps": 0, "bps_rd": 0, "file": "TEST_DIR/t.qcow2", "encryption_key_m= issing": false}, "type": "unknown"}, {"io-status": "ok", "device": "ide1-= cd0", "locked": false, "removable": true, "tray_open": false, "type": "un= known"}, {"device": "floppy0", "locked": false, "removable": true, "tray_= open": false, "type": "unknown"}, {"device": "sd0", "locked": false, "rem= ovable": true, "tray_open": false, "type": "unknown"}]} >> +{"return": [{"io-status": "ok", "device": "disk", "locked": false, "r= emovable": false, "inserted": {"iops_rd": 0, "detect_zeroes": "off", "ima= ge": {"virtual-size": 134217728, "filename": "TEST_DIR/t.qcow2", "cluster= -size": 65536, "format": "qcow2", "actual-size": SIZE, "format-specific":= {"type": "qcow2", "data": {"compat": "1.1", "lazy-refcounts": false, "re= fcount-width": 16, "corrupt": false}}, "dirty-flag": false}, "iops_wr": 0= , "ro": false, "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_w= r": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "file": "TEST_DIR/t.qco= w2", "encryption_key_missing": false}, "type": "unknown"}, {"io-status": = "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_ope= n": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "re= movable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0",= "locked": false, "removable": true, "tray_open": false, "type": "unknown= "}]} > It would be nice to figure out how to avoid long lines in the testsuite= =2E > But that's a project for another day. Yes, Kevin already proposed adding a "pretty" flag to -qmp which uses=20 the pretty JSON formatter (which breaks lines and indents). > If you can make a strong argument for always outputting the constant > width of 16 for 0.10 formats, then I can live with it, so: You decide whether it's strong enough. :-) My main argument is "If a bool is not displayed one can assume it to be=20 false; if an integer is not displayed which naturally cannot be 0, I=20 will have no idea what it would be, even if it's constant for that image = version". > Reviewed-by: Eric Blake Thanks! Max