Re: [Qemu-devel] [PATCH v1 1/3] qcow2: introduce compression type feature

[Kevin Wolf]

Am 03.07.2019 um 17:01 hat Denis Plotnikov geschrieben:
> On 03.07.2019 17:14, Eric Blake wrote:
> > On 7/3/19 6:00 AM, Denis Plotnikov wrote:
> >> diff --git a/block/qcow2.c b/block/qcow2.c
> >> index 3ace3b2209..921eb67b80 100644
> >> --- a/block/qcow2.c
> >> +++ b/block/qcow2.c
> >> @@ -1202,6 +1202,47 @@ static int qcow2_update_options(BlockDriverState 
> >> *bs, QDict *options,
> >>       return ret;
> >>   }
> >>   
> >> +static int check_compression_type(BDRVQcow2State *s, Error **errp)
> >> +{
> >> +    bool is_set;
> >> +    int ret = 0;
> >> +
> >> +    switch (s->compression_type) {
> >> +    case QCOW2_COMPRESSION_TYPE_ZLIB:
> >> +        break;
> >> +
> >> +    default:
> >> +        if (errp) {
> > Useless check for errp being non-NULL.  What's worse, if the caller
> > passes in NULL because they don't care about the error, then your code
> > behaves differently.  Just call error_setg() and return -ENOTSUP
> > unconditionally.
> ok
> > 
> >> +            error_setg(errp, "qcow2: unknown compression type: %u",
> >> +                       s->compression_type);
> >> +            return -ENOTSUP;
> >> +        }
> >> +    }
> >> +
> >> +    /*
> >> +     * with QCOW2_COMPRESSION_TYPE_ZLIB the corresponding incompatible
> >> +     * feature flag must be absent, with other compression types the
> >> +     * incompatible feature flag must be set
> > Is there a strong reason for forbid the incompatible feature flag with
> > ZLIB?
> The main reason is to guarantee image opening for older qemu if 
> compression type is zlib.
> > Sure, it makes the image impossible to open with older qemu, but
> > it doesn't get in the way of newer qemu opening it. I'll have to see how
> > your spec changes documented this, to see if you could instead word it
> > as 'for ZLIB, the incompatible feature flag may be absent'.
> I just can't imagine when and why we would want to set incompatible 
> feature flag with zlib. Suppose we have zlib with the flag set, then
> older qemu can't open the image although it is able to work with the 
> zlib compression type. For now, I don't understand why we should make 
> such an artificial restriction.

We don't want to create such images, but we want to keep our code as
simple as possible.

As your patch shows, forbidding the case is more work than just allowing
it. So if external software can create such images, and it would just
automatically work in QEMU, then why do the extra work to articifially
forbid this?

(Actually, it's likely that on the first header update, QEMU would just
end up dropping the useless flag, so we even "fix" such images.)

> >> diff --git a/qapi/block-core.json b/qapi/block-core.json
> >> index 7ccbfff9d0..6aa8b99993 100644
> >> --- a/qapi/block-core.json
> >> +++ b/qapi/block-core.json
> >> @@ -78,6 +78,8 @@
> >>   #
> >>   # @bitmaps: A list of qcow2 bitmap details (since 4.0)
> >>   #
> >> +# @compression-type: the image cluster compression method (since 4.1)
> >> +#
> >>   # Since: 1.7
> >>   ##
> >>   { 'struct': 'ImageInfoSpecificQCow2',
> >> @@ -89,7 +91,8 @@
> >>         '*corrupt': 'bool',
> >>         'refcount-bits': 'int',
> >>         '*encrypt': 'ImageInfoSpecificQCow2Encryption',
> >> -      '*bitmaps': ['Qcow2BitmapInfo']
> >> +      '*bitmaps': ['Qcow2BitmapInfo'],
> >> +      '*compression-type': 'Qcow2CompressionType'
> > Why is this field optional? Can't we always populate it, even for older
> > images?
> Why we should if we don't care ?

I was trying too check what the condition is under which the field will
be present in the output, but I couldn't find any code for it.

So it looks like this patch never makes use of the field and it's dead
code?

Kevin