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
