[libvirt] [PATCH v4 4/5] qemu: migration: add compression options

[Jiri Denemark]

On Fri, Mar 04, 2016 at 14:20:57 +0300, Nikolay Shirokovskiy wrote:
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at virtuozzo.com>
> ---
>  include/libvirt/libvirt-domain.h | 33 ++++++++++++++++
>  src/qemu/qemu_migration.c        | 85 +++++++++++++++++++++++++++++++++++++++-
>  src/qemu/qemu_migration.h        |  7 ++++
>  3 files changed, 124 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index b3a176f..59df373 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -784,6 +784,39 @@ typedef enum {
>  
>  # define VIR_MIGRATE_PARAM_COMPRESSION    "compression"
>  
> +/**
> + * VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL:
> + *
> + * virDomainMigrate* params field: the level of compression for multithread
> + * compression as VIR_TYPED_PARAM_INT. Accepted values * are in range 0-9.

Looks like a leftover '*' --------------------------------^

> + * 0 is no compression, 1 is maximum speed and 9 is maximum compression.
> + */
> +# define VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL    "compression.mt.level"

Then name of the compression method in individual parameters should
match the name passed to VIR_MIGRATE_PARAM_COMPRESSION. That is, either
change the method name to "mt" or use "compression.multithread." prefix
for additional parameters.

> +
> +/**
> + * VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS:
> + *
> + * virDomainMigrate* params field: the number of compression threads for
> + * multithread compression as VIR_TYPED_PARAM_INT.
> + */
> +# define VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS "compression.mt.threads"
> +
> +/**
> + * VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS:
> + *
> + * virDomainMigrate* params field: the number of decompression threads for
> + * multithread compression as VIR_TYPED_PARAM_INT.
> + */
> +# define VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS "compression.mt.dthreads"
> +
> +/**
> + * VIR_MIGRATE_PARAM_COMPRESSION_XBZRLE_CACHE:
> + *
> + * virDomainMigrate* params field: the size of page cache for xbzrle
> + * compression as VIR_TYPED_PARAM_ULLONG.
> + */
> +# define VIR_MIGRATE_PARAM_COMPRESSION_XBZRLE_CACHE "compression.xbzrle.cache"
> +
>  /* Domain migration. */
>  virDomainPtr virDomainMigrate (virDomainPtr domain, virConnectPtr dconn,
>                                 unsigned long flags, const char *dname,
> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> index 5fcf132..88d62a9 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
...
> @@ -6379,6 +6401,41 @@ qemuMigrationCompressionParseParams(qemuMigrationCompressionPtr compression,
>          }
>      }
>  
> +#define VIR_GET_PARAMETER_OPT(PARAM, TYPE, PARENT, VALUE)               \

The name of this macro is long while still generic. Since it is a local
macro, I think you can use a short name, such as GET_PARAM.

> +    if ((rc = virTypedParamsGet ## TYPE(params, nparams,                \
> +                                         PARAM, &PARENT->VALUE)) < 0)   \
> +        return -1;                                                      \
> +                                                                        \
> +    if (rc == 1)                                                        \
> +        PARENT->VALUE ## _set = 1;

Anyway, the macro needs to be enclosed in do { } while () so that you
can use semicolon when calling it.

> +
> +    VIR_GET_PARAMETER_OPT(VIR_MIGRATE_PARAM_COMPRESSION_MT_LEVEL,
> +                          Int, cparams, level)

Semicolon should go here ------------------------^

> +    VIR_GET_PARAMETER_OPT(VIR_MIGRATE_PARAM_COMPRESSION_MT_THREADS,
> +                          Int, cparams, threads)
> +    VIR_GET_PARAMETER_OPT(VIR_MIGRATE_PARAM_COMPRESSION_MT_DTHREADS,
> +                          Int, cparams, dthreads)
> +    VIR_GET_PARAMETER_OPT(VIR_MIGRATE_PARAM_COMPRESSION_XBZRLE_CACHE,
> +                          ULLong, compression, xbzrle_cache)
> +
> +#undef VIR_GET_PARAMETER_OPT
> +
> +    if ((cparams->level_set || cparams->threads_set || cparams->dthreads_set) &&
> +        !(compression->methods & QEMU_MIGRATION_COMPESS_MULTITHREAD)) {
> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
> +                       _("You cannot specify multithread compression "
> +                         "parameters without turning it on."));

We don't use '.' at the end of error messages. And I'd also avoid
pointing at the user with "You".

> +        return -1;
> +    }
> +
> +    if (compression->xbzrle_cache_set &&
> +        !(compression->methods & QEMU_MIGRATION_COMPESS_XBZRLE)) {
> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
> +                       _("You cannot specify xbzrle compression "
> +                         "parameters without turning it on."));

Ditto.

> +        return -1;
> +    }
> +
>      return 0;
>  }
>  
...

Jirka