Re: [Qemu-devel] [PATCH 08/11] drive-backup: added support for data compression

[Pavel Butsykin]

On 13.06.2016 23:18, Eric Blake wrote:
> On 05/31/2016 03:15 AM, Denis V. Lunev wrote:
> > From: Pavel Butsykin <address@hidden>
> >
> > The idea is simple - backup is "written-once" data. It is written block
> > by block and it is large enough. It would be nice to save storage
> > space and compress it.
> >
> > The patch adds a flag to the qmp/hmp drive-backup command which enables
> > block compression. Compression should be implemented in the format driver
> > to enable this feature.
> >
> > There are some limitations of the format driver to allow compressed writes.
> > We can write data only once. Though for backup this is perfectly fine.
> > These limitations are maintained by the driver and the error will be
> > reported if we are doing something wrong.
>
> > @@ -3309,6 +3315,7 @@ void qmp_drive_backup(const char *device, const char 
> > *target,
> >                         bool has_mode, enum NewImageMode mode,
> >                         bool has_speed, int64_t speed,
> >                         bool has_bitmap, const char *bitmap,
> > +                      bool has_compress, bool compress,
> >                         bool has_on_source_error, BlockdevOnError 
> > on_source_error,
> >                         bool has_on_target_error, BlockdevOnError 
> > on_target_error,
> >                         Error **errp)
>
> Might be nice to simplify this signature once my qapi 'box' patches land.

Yes, it would certainly be better. But your patches are not applied on
the current, maybe you have your own branch?

> > +++ b/qapi/block-core.json
> > @@ -888,6 +888,9 @@
> >   #          Must be present if sync is "incremental", must NOT be present
> >   #          otherwise. (Since 2.4)
> >   #
> > +# @compress: #optional the compression data blocks (if the target format
> > +#            supports it; default: false).
>
> Missing a '(since 2.7)' notation.
>
> > +++ b/qmp-commands.hx
> > @@ -1186,7 +1186,8 @@ EQMP
> >       {
> >           .name       = "drive-backup",
> >           .args_type  = "sync:s,device:B,target:s,speed:i?,mode:s?,format:s?,"
> > -                      "bitmap:s?,on-source-error:s?,on-target-error:s?",
> > +                      "bitmap:s?,compress:b?,"
> > +                      "on-source-error:s?,on-target-error:s?",
> >           .mhandler.cmd_new = qmp_marshal_drive_backup,
> >       },
> >
> > @@ -1220,6 +1221,8 @@ Arguments:
> >   - "mode": whether and how QEMU should create a new image
> >             (NewImageMode, optional, default 'absolute-paths')
> >   - "speed": the maximum speed, in bytes per second (json-int, optional)
> > +- "compress": the compression data blocks (if the target format supports it).
> > +              (json-bool, optional, default false)
>
> Reads awkwardly; maybe "compress": true to compress data, if the target
> format supports it. (json-bool, optional, default false)