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

[Eric Blake]

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.

> 
>> +    }
>> +
>> +    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.

>>  /* Called with s->lock held.  */
>>  static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>>                                        int flags, Error **errp)
>> @@ -1318,6 +1344,35 @@ static int coroutine_fn 
>> qcow2_do_open(BlockDriverState *bs, QDict *options,
>>      s->compatible_features      = header.compatible_features;
>>      s->autoclear_features       = header.autoclear_features;
>>  
>> +    /*
>> +     * Handle compression type
>> +     * Older qcow2 images don't contain the compression type header.
>> +     * Distinguish them by the header length and use
>> +     * the only valid (default) compression type in that case
>> +     */
>> +    if (header.header_length > offsetof(QCowHeader, compression_type)) {
>> +        /* sanity check that we can read a compression type */
>> +        size_t min_len = offsetof(QCowHeader, compression_type) +
>> +                         sizeof(header.compression_type);
>> +        if (header.header_length < min_len) {
>> +            error_setg(errp,
>> +                       "Could not read compression type."
>> +                       "qcow2 header is too short");
> 
> This will read as “Could not read compression type.qcow2 header is too
> short”.  There should be a space after the full stop (and the full stop
> should maybe be a comma instead).

Indeed, error_setg() should generally not contain '.'

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature