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

[Max Reitz]

On 08.08.19 02:09, Eric Blake wrote:
> On 8/7/19 6:12 PM, Max Reitz wrote:
> 
>>>  
>>> +static int check_compression_type(BDRVQcow2State *s, Error **errp)
>>> +{
>>> +    switch (s->compression_type) {
>>> +    case QCOW2_COMPRESSION_TYPE_ZLIB:
>>> +        break;
>>> +
>>> +    default:
>>> +        error_setg(errp, "qcow2: unknown compression type: %u",
>>> +                   s->compression_type);
>>> +        return -ENOTSUP;
>>> +    }
>>> +
>>> +    /*
>>> +     * if the compression type differs from QCOW2_COMPRESSION_TYPE_ZLIB
>>> +     * the incompatible feature flag must be set
>>> +     */
>>> +
>>> +    if (s->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB &&
>>> +        !(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
>>> +            error_setg(errp, "qcow2: Invalid compression type setting");
>>> +            return -EINVAL;
>>
>> (1) Why is this block indented twice?
>>
>> (2) Do we need this at all?  Sure, it’s a corruption, but do we need to
>> reject the image because of it?
> 
> Yes, because otherwise there is a high risk of some application
> misinterpreting the contents (whether an older qemu that silently
> ignores unrecognized headers, and so assumes it can decode compressed
> clusters with zlib even though the decode will only succeed with zstd,
> or can write a compressed cluster with zlib which then causes corruption
> when the newer qemu tries to read it with zstd).  The whole point of an
> incompatible bit is to reject opening an image that can't be interpreted
> correctly, and where writing may break later readers.

Fair enough.

>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>
>> Overall, I don’t see the purpose of this function.  I don’t see any need
>> to call it in qcow2_update_header().  And without “does non-zlib
>> compression imply the respective incompatible flag?” check, you can just
>> inline the rest (checking that we recognize the compression type) into
>> qcow2_do_open().
>>
> 
> Inlining may indeed be possible; the real question is whether the
> function expands later in the series to the point that inlining no
> longer makes sense.

A quick search says no.

Max

signature.asc
Description: OpenPGP digital signature