On 03.07.2019 18:13, Eric Blake wrote: > On 7/3/19 10:01 AM, Denis Plotnikov wrote: > >>>> + * 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 have an artificial restriction one way or the other. > > Method 1 - artificial restriction that incompatible bit must NOT be set > when compression type is zlib > > Method 2 - artificial restriction that older qcow2 programs can't open a > zlib image with incompatible bit set, even though removing the bit makes > the image more portable. > > It's desirable that qemu should NOT set the incompatible bit when it > does not need to (for the sake of portability to more apps), but > MANDATING that it must not set the bit for zlib is a stronger spec > restriction. > > I tend to lean for the spec being looser unless there is a strong reason > for why it must be strict; just because qemu won't be setting the > incompatible bit does not necessarily mean that all other > implementations will try to be that careful; they may have valid reasons > for setting the bit even when using zlib, but only if the spec permits > them to do so. So, how I should implement that? Method 1 or Method 2? How we can decide? Ask what other maintainers think about that? > > >>>> @@ -2434,6 +2493,13 @@ int qcow2_update_header(BlockDriverState *bs) >>>> total_size = bs->total_sectors * BDRV_SECTOR_SIZE; >>>> refcount_table_clusters = s->refcount_table_size >> >>>> (s->cluster_bits - 3); >>>> >>>> + ret = check_compression_type(s, NULL); >>> Why are you ignoring the error here? >> qcow2_update_header() doesn't use the error and just returns an error >> code or 0 > > Are we potentially losing a valuable error message (in which case > qcow2_update_header should maybe be first patched to take an errp > parameter), or is it always going to succeed (in which case &error_abort > would document our intention that we know it can't fail), or is it > really a case where it may fail, but we don't care about losing the > message? Passing NULL is not wrong (as you say, we aren't plumbed to > pass the message back up to the caller), but does raise enough > suspicions as to be worth auditing the code while in the area.
Could we do it after adding this series since it already implemented without the error? > > >>>> + 104 - 107: compression_type >>>> + Defines the compression method used for compressed >>>> clusters. >>>> + A single compression type is applied to all >>>> compressed image >>>> + clusters. >>>> + The compression type is set on image creation only. >>>> + The default compression type is zlib. >>> Where is the documentation that a value of 0 corresponds to zlib? >> Should I do it right here or it's better to add a separate chapter in >> the docs/interop/qcow2.txt ? > > Right here. ok > > >>>> +++ 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 ? > > Because it shows that we are running a new enough qemu that knows about > the compression field (even when it is absent). ok, if everybody agree with that I'll implement whatever is better > -- Best, Denis