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

[Denis Plotnikov]

On 23.03.2020 15:26, Vladimir Sementsov-Ogievskiy wrote:
> 23.03.2020 15:22, Denis Plotnikov wrote:
> > 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 <address@hidden>
> > > > Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > > > ---
> > >
> > > [...]
> > >
> > > > @@ -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"
>
> No objections. Then you should pass NULL to qapi_enum_parse instead of 
> errp. (As you can't set errp twice, second try will crash).
ok
> > > > +                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 */ }
> > > >       }
> > > >   };
> > >
> > >
> > > [...]