Re: [PATCH v8 1/4] qcow2: introduce compression type feature

[Denis Plotnikov <dplotnikov@virtuozzo.com>]

On 23.03.2020 11:00, Vladimir Sementsov-Ogievskiy wrote:
> 21.03.2020 17:34, Denis Plotnikov wrote:
>> The patch adds some preparation parts for incompatible compression type
>> feature to qcow2 allowing the use different compression methods for
>> image clusters (de)compressing.
>>
>> It is implied that the compression type is set on the image creation and
>> can be changed only later by image conversion, thus compression type
>> defines the only compression algorithm used for the image, and thus,
>> for all image clusters.
>>
>> The goal of the feature is to add support of other compression methods
>> to qcow2. For example, ZSTD which is more effective on compression 
>> than ZLIB.
>>
>> The default compression is ZLIB. Images created with ZLIB compression 
>> type
>> are backward compatible with older qemu versions.
>>
>> Adding of the compression type breaks a number of tests because now the
>> compression type is reported on image creation and there are some 
>> changes
>> in the qcow2 header in size and offsets.
>>
>> The tests are fixed in the following ways:
>>      * filter out compression_type for many tests
>>      * fix header size, feature table size and backing file offset
>>        affected tests: 031, 036, 061, 080
>>        header_size +=8: 1 byte compression type
>>                         7 bytes padding
>>        feature_table += 48: incompatible feature compression type
>>        backing_file_offset += 56 (8 + 48 -> header_change + 
>> feature_table_change)
>>      * add "compression type" for test output matching when it isn't 
>> filtered
>>        affected tests: 049, 060, 061, 065, 144, 182, 242, 255
>>
>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>
> [...]
>
>> @@ -4859,6 +4949,7 @@ static ImageInfoSpecific 
>> *qcow2_get_specific_info(BlockDriverState *bs,
>>               .data_file          = g_strdup(s->image_data_file),
>>               .has_data_file_raw  = has_data_file(bs),
>>               .data_file_raw      = data_file_is_raw(bs),
>> +            .compression_type   = s->compression_type,
>>           };
>>       } else {
>>           /* if this assertion fails, this probably means a new 
>> version was
>> @@ -5248,6 +5339,22 @@ static int 
>> qcow2_amend_options(BlockDriverState *bs, QemuOpts *opts,
>>                                    "images");
>>                   return -EINVAL;
>>               }
>> +        } else if (!strcmp(desc->name, BLOCK_OPT_COMPRESSION_TYPE)) {
>> +            int compression_type =
>> + qapi_enum_parse(&Qcow2CompressionType_lookup,
>> +                                qemu_opt_get(opts, 
>> BLOCK_OPT_COMPRESSION_TYPE),
>> +                                -1, errp);
>> +
>> +            if (compression_type == -EINVAL) {
>
> You should compare to -1, as qapi_enum_parse returns given default on 
> error.
ok
>
>> +                error_setg(errp, "Unknown compression type");
>
> and errp is already set (ofcourse, if qemu_opt_get returned non NULL, 
> but I hope it is guaranteed by if (!strcmp(desc->name, 
> BLOCK_OPT_COMPRESSION_TYPE)) condition
I wouldn't propagate the error from qapi_enum_parse because it looks 
like "invalid parameter value: foo". I think it's better to print 
"Unknown compression type: foo"
>
>> +                return -ENOTSUP;
>> +            }
>> +
>> +            if (compression_type != s->compression_type) {
>> +                error_setg(errp, "Changing the compression type "
>> +                                 "is not supported");
>> +                return -ENOTSUP;
>> +            }
>>           } else {
>>               /* if this point is reached, this probably means a new 
>> option was
>>                * added without having it covered here */
>> @@ -5516,6 +5623,12 @@ static QemuOptsList qcow2_create_opts = {
>>               .help = "Width of a reference count entry in bits",
>>               .def_value_str = "16"
>>           },
>> +        {
>> +            .name = BLOCK_OPT_COMPRESSION_TYPE,
>> +            .type = QEMU_OPT_STRING,
>> +            .help = "Compression method used for image cluster 
>> compression",
>> +            .def_value_str = "zlib"
>> +        },
>>           { /* end of list */ }
>>       }
>>   };
>
>
> [...]
>
>