Re: [Qemu-devel] [PATCH 02/21] qcow2: Add refcount_width to format-specific info

[Max Reitz]

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 <address@hidden>
> > ---
> >   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_info(BlockDriverState *bs)
> >       };
> >       if (s->qcow_version == 2) {
> >           *spec_info->qcow2 = (ImageInfoSpecificQCow2){
> > -            .compat = g_strdup("0.10"),
> > +            .compat             = g_strdup("0.10"),
> > +            .refcount_width     = 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 the
> 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 
refcounts" simply do not make sense with compat=0.10 images (it's simply 
impossible), the refcount width does make sense. It's always 16 bits 
(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 
display. I'd be fine with dropping it from compat=0.10, though.

But in retrospect, I'd rather make the other two flags always visible 
than now drop this entry. However, not displaying a bool if it's always 
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 = 'compat=0.10'
> > -    json_compare = { 'compat': '0.10' }
> > -    human_compare = [ 'compat: 0.10' ]
> > +    json_compare = { 'compat': '0.10', 'refcount-width': 16 }
> > +    human_compare = [ '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, "removable": false, "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": {"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, "corrupt": false}}, "dirty-flag": false}, "iops_wr": 0, "ro": false, 
> > "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false}, "type": "unknown"}, {"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "sd0", 
> > "locked": false, "removable": true, "tray_open": false, "type": "unknown"}]}
> > +{"return": [{"io-status": "ok", "device": "disk", "locked": false, "removable": false, "inserted": {"iops_rd": 0, "detect_zeroes": "off", "image": {"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, "refcount-width": 16, "corrupt": false}}, "dirty-flag": false}, "iops_wr": 0, 
> > "ro": false, "backing_file_depth": 0, "drv": "qcow2", "iops": 0, "bps_wr": 0, "encrypted": false, "bps": 0, "bps_rd": 0, "file": "TEST_DIR/t.qcow2", "encryption_key_missing": false}, "type": "unknown"}, {"io-status": "ok", "device": "ide1-cd0", "locked": false, "removable": true, "tray_open": false, "type": "unknown"}, {"device": "floppy0", "locked": false, "removable": 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.
>   But that's a project for another day.

Yes, Kevin already proposed adding a "pretty" flag to -qmp which uses 
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 
false; if an integer is not displayed which naturally cannot be 0, I 
will have no idea what it would be, even if it's constant for that image 
version".

> Reviewed-by: Eric Blake <address@hidden>

Thanks!

Max