On 28.06.19 16:54, Kevin Wolf wrote:
> Am 28.06.2019 um 16:40 hat Denis Plotnikov geschrieben:
>>
>>
>> On 28.06.2019 17:24, Kevin Wolf wrote:
>>> Am 28.06.2019 um 14:56 hat Denis Plotnikov geschrieben:
>>>>
>>>>
>>>> On 28.06.2019 15:06, Kevin Wolf wrote:
>>>>> Am 28.06.2019 um 13:24 hat Denis Plotnikov geschrieben:
>>>>>>
>>>>>>
>>>>>> On 28.06.2019 13:23, Kevin Wolf wrote:
>>>>>>> Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
>>>>>>>> With the patch, qcow2 is able to process image compression type
>>>>>>>> defined in the image header and choose the corresponding method
>>>>>>>> for clusters compressing.
>>>>>>>>
>>>>>>>> Also, it rework the cluster compression code for adding more
>>>>>>>> compression types.
>>>>>>>>
>>>>>>>> Signed-off-by: Denis Plotnikov <dplotni...@virtuozzo.com>
>>>>>>>> ---
>>>>>>>> block/qcow2.c | 103
>>>>>>>> ++++++++++++++++++++++++++++++++++++++++++++------
>>>>>>>> 1 file changed, 92 insertions(+), 11 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>>>>> index c4b5b93408..90f15cc3c9 100644
>>>>>>>> --- a/block/qcow2.c
>>>>>>>> +++ b/block/qcow2.c
>>>>>>>> @@ -400,11 +400,39 @@ static int
>>>>>>>> qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>>>>>>>> break;
>>>>>>>>
>>>>>>>> case QCOW2_EXT_MAGIC_COMPRESSION_TYPE:
>>>>>>>> + /* Compression type always goes with the compression type
>>>>>>>> bit set */
>>>>>>>> + if (!(s->incompatible_features &
>>>>>>>> QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
>>>>>>>> + error_setg(errp,
>>>>>>>> + "compression_type_ext: "
>>>>>>>> + "expect compression type bit set");
>>>>>>>> + return -EINVAL;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + ret = bdrv_pread(bs->file, offset, &s->compression_type,
>>>>>>>> ext.len);
>>>>>>>> + s->compression_type = be32_to_cpu(s->compression_type);
>>>>>>>> +
>>>>>>>> + if (ret < 0) {
>>>>>>>> + error_setg_errno(errp, -ret,
>>>>>>>> + "ERROR: Could not read compression
>>>>>>>> type");
>>>>>>>> + return ret;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> /*
>>>>>>>> - * Setting compression type to
>>>>>>>> BDRVQcow2State->compression_type
>>>>>>>> - * from the image header is going to be here
>>>>>>>> + * The default compression type is not allowed when the
>>>>>>>> extension
>>>>>>>> + * is present. ZLIB is used as the default compression
>>>>>>>> type.
>>>>>>>> + * When compression type extension header is present then
>>>>>>>> + * compression_type should have a value different from
>>>>>>>> the default.
>>>>>>>> */
>>>>>>>> - break;
>>>>>>>> + if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
>>>>>>>> + error_setg(errp,
>>>>>>>> + "compression_type_ext:"
>>>>>>>> + "invalid compression type %d",
>>>>>>>> + QCOW2_COMPRESSION_TYPE_ZLIB);
>>>>>>>> + }
>>>>>>>
>>>>>>> This is a restriction that the spec doesn't make, so strictly speaking
>>>>>>> this implementation wouldn't be compliant to the spec.
>>>>>> The idea is that ZLIB shouldn't appear in the compression type
>>>>>> extension. This allows image backward compatibility with an older qemu
>>>>>> if zlib is used.
>>>>>>
>>>>>> There is no reason to set ZLIB in the extension because an older qemu
>>>>>> knows how to tread ZLIB compressed clusters.
>>>>>>
>>>>>> The restriction aims to guarantee that.
>>>>>>
>>>>>> I tried to describe this case in the specification:
>>>>>> ...
>>>>>> When the compression type bit is not set, and the compression type
>>>>>> header extension is absent, ZLIB compression is used for compressed
>>>>>> clusters.
>>>>>>
>>>>>> Qemu versions older than 4.1 can use images created with compression
>>>>>> type ZLIB without any additional preparations and cannot use images
>>>>>> created with compression types != ZLIB.
>>>>>> ...
>>>>>>
>>>>>> Does it makes sense?
>>>>>
>>>>> This text says that using zlib in the extension is not necessary because
>>>>> it's the default. But it doesn't say that using zlib in the extension is
>>>>> illegal.
>>>>>
>>>>> I agree that there is no good reason to create a compression type
>>>>> extension if you have zlib. But is there a good reason to forbid it?
>>>> I think yes, if we create image with the extension set to zlib we
>>>> prevent an older qemu from using that image. Furthermore, to allow older
>>>> qemu using such images we need to create special conversion procedure
>>>> which has to remove the extension header.
>>>>
>>>> If zlib is a "special compression type" which is always set by default
>>>> without the extension header we'll get rid of such image conversion
>>>> procedure and an older qemu could use it "as is"
>>>>
>>>> Might it work as a good reason?
>>>>
>>>>> It
>>>>> only requires us to add artificial restrictions to code that would work
>>>>> fine without them.
>>>>>
>>>>> Either way, if we want to reject such extensions, the spec needs to say
>>>>> that it's illegal. And if the spec allows such images, we must accept
>>>>> them.
>>>> Yes, it's true
>>>>
>>>> The only reasons that zlib compression type even exists in the
>>>> enumeration is to avoid ambiguity for users.
>>>> For them it may be hard to understand why they can set zstd and cannot
>>>> set zlib as compression type and to really set zlib they have to set no
>>>> compression type to make the default zlib to apply.
>>>>
>>>> When a user set zlib as compression type the image is created as before
>>>> the extension header were introduced.
>>>>
>>>> Reasonable?
>>>>>
>>>>>>> We can discuss whether the code or the spec should be changed. At the
>>>>>>> moment, I don't see a good reason to make the restriction
>>>>>>>
>>>>>>>> +#ifdef DEBUG_EXT
>>>>>>>> + printf("Qcow2: image compression type %s\n",
>>>>>>>> s->compression_type);
>>>>>>>> +#endif
>>>>>>>> + break;
>>>>>>>>
>>>>>>>> case QCOW2_EXT_MAGIC_DATA_FILE:
>>>>>>>> {
>>>>>>>
>>>>>>> We would save most of this code if we added a new field to the header
>>>>>>> instead of adding a header extension. Not saying that we should
>>>>>>> definitely do this, but let's discuss it at least.
>>>>>>
>>>>>> If we add the new field to the header will the older qemu be able to use
>>>>>> it. Or we will add the header only if needed, i.e. if compression_type
>>>>>> != zlib
>>>>>
>>>>> Increasing the header size is backwards compatible. Older qemu versions
>>>>> should handle such images correctly. They would store the unknown part
>>>>> of the header in s->unknown_header_fields and keep it unmodified when
>>>>> updating the image header.
>>>>>
>>>>> We would still add the incompatible feature flag for non-zlib, of
>>>>> course.
>>>> so, we basically need to do the same: store compression type and forbid
>>>> to use because of flag if not zlib.
>>>>
>>>> Sounds like it doesn't differ that much from the extension header approach.
>>>
>>> It provides more or less the same functionality, but would probably make
>>> this patch half the size because all of the code related to reading and
>>> checking the header extension would go away. It also saves a few bytes
>>> in the header cluster (4 bytes vs. 16 bytes).
>> ok, will re-do it that way.
>>
>> Do you agree in general with how zlib compression type is treated?
>
> As I said, I think both ways are justifiable as long as we stay
> consistent between qemu and spec.
>
> I'd prefer to allow zlib in the extension, you'd prefer to forbid it.
> So I'd like to hear opinions from some more people on which way they
> prefer.I don’t think it’s any better to completely forbid it than to just recommend in the spec that software should not set this field to zlib to ensure backwards compatibility. I see the point of forbidding it, but if I were to know nothing of qcow2 and read the spec, I guess I’d find it a bit weird to read “If this field is not present, the compression type is zlib; if it is, it is not zlib, but the specified value.” I’d ask myself why it isn’t simply “The compression type is given by this field, it defaults to zlib.” Max
signature.asc
Description: OpenPGP digital signature
