On 03.07.2019 18:46, Kevin Wolf wrote: > 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.) ok, removing the restriction > >>>> 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? ok > > Kevin >
-- Best, Denis