On Fri, Mar 04, 2016 at 14:20:54 +0300, Nikolay Shirokovskiy wrote:
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@xxxxxxxxxxxxx>
> ---
> include/libvirt/libvirt-domain.h | 11 +++
> src/qemu/qemu_driver.c | 29 +++++--
> src/qemu/qemu_migration.c | 163 ++++++++++++++++++++++++++++++++-------
> src/qemu/qemu_migration.h | 23 ++++++
> src/qemu/qemu_monitor.c | 2 +-
> src/qemu/qemu_monitor.h | 1 +
> 6 files changed, 195 insertions(+), 34 deletions(-)
>
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 79c25df..b3a176f 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -671,6 +671,8 @@ typedef enum {
> * when supported */
> VIR_MIGRATE_UNSAFE = (1 << 9), /* force migration even if it is considered unsafe */
> VIR_MIGRATE_OFFLINE = (1 << 10), /* offline migrate */
> + /* Migration options could be specified further via VIR_MIGRATE_PARAM_COMPRESSION
> + * otherwise default options will be used */
Either put a short version of the above comment to the comment for
VIR_MIGRATE_COMPRESSED or add such comment to migration APIs (or both),
but the comment here is likely to confuse our documentation generator.
> VIR_MIGRATE_COMPRESSED = (1 << 11), /* compress data during migration */
> VIR_MIGRATE_ABORT_ON_ERROR = (1 << 12), /* abort migration on I/O errors happened during migration */
> VIR_MIGRATE_AUTO_CONVERGE = (1 << 13), /* force convergence */
> @@ -773,6 +775,15 @@ typedef enum {
> */
> # define VIR_MIGRATE_PARAM_MIGRATE_DISKS "migrate_disks"
>
> +/**
> + * VIR_MIGRATE_PARAM_COMPRESSION:
> + *
> + * virDomainMigrate* params multiple field: string list of compression methods
> + * that are used to compress migration traffic.
> + */
What is a "string list"? Is it a single string with method names
separated by something, an array of strings, or something completely
different? Oh, looking at the rest of this patch, it's the case of
allowing multiple instances of the same parameter. The documentation
should say that, e.g., "name of the method used to compress migration
traffic. The parameter may be specified multiple times if more than one
method should be used".
> +
Remove the empty line here.
> +# define VIR_MIGRATE_PARAM_COMPRESSION "compression"
> +
> /* 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 64cbffa..5fcf132 100644
> --- a/src/qemu/qemu_migration.c
> +++ b/src/qemu/qemu_migration.c
> @@ -3299,6 +3299,39 @@ qemuMigrationPrepareIncoming(virDomainObjPtr vm,
> }
>
> static int
> +qemuMigrationSetCompression(virQEMUDriverPtr driver,
> + virDomainObjPtr vm,
> + qemuDomainAsyncJob job,
> + qemuMigrationCompressionPtr compression,
> + unsigned int flags)
> +{
> + /*
> + * if compression methods are not set explicitly use flags to
> + * set default compression methods
> + */
> + if (compression == NULL || compression->methods == 0) {
> + return qemuMigrationSetOption(driver, vm,
> + QEMU_MONITOR_MIGRATION_CAPS_XBZRLE,
> + flags & VIR_MIGRATE_COMPRESSED,
> + job);
> + }
I think it would be cleaner if qemuMigrationCompressionParseParams got
the flags and set QEMU_MIGRATION_COMPESS_XBZRLE in compression->methods
in case no compression parametr is used. Doing that would even fix a bug
in your code, because even if no VIR_MIGRATE_PARAM_COMPRESSION parameter
is used, you still want to explicitly disable any supported compression
capability. In other words, you'd still have to call
qemuMigrationSetOption for both XBZRLE and MULTITHREAD in the if
statement above. Changing qemuMigrationCompressionParseParams to do the
right thing will avoid this code duplication.
> +
> + if (qemuMigrationSetOption(driver, vm,
> + QEMU_MONITOR_MIGRATION_CAPS_XBZRLE,
> + compression->methods & QEMU_MIGRATION_COMPESS_XBZRLE,
> + job) < 0)
> + return -1;
> +
> + if (qemuMigrationSetOption(driver, vm,
> + QEMU_MONITOR_MIGRATION_CAPS_COMPRESS,
> + compression->methods & QEMU_MIGRATION_COMPESS_MULTITHREAD,
> + job) < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +static int
> qemuMigrationPrepareAny(virQEMUDriverPtr driver,
> virConnectPtr dconn,
> const char *cookiein,
...
> @@ -6293,3 +6339,66 @@ qemuMigrationErrorReport(virQEMUDriverPtr driver,
> virSetError(err);
> virFreeError(err);
> }
> +
> +static int
> +qemuMigrationCompressMethodsAdd(qemuMigrationCompressMethods *methods,
> + qemuMigrationCompressMethods method)
> +{
> + if (*methods & method) {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("Compression method is specified twice."));
> + return -1;
> + }
> +
> + *methods |= method;
> + return 0;
> +}
I don't think there is a reason to split the above trivial code into a
separate function. The main reason is that the following function should
be simplified.
> +
> +int
> +qemuMigrationCompressionParseParams(qemuMigrationCompressionPtr compression,
> + virTypedParameterPtr params, int nparams)
> +{
> + size_t i;
> +
> + for (i = 0; i < nparams; i++) {
> + if (STRNEQ(params[i].field, VIR_MIGRATE_PARAM_COMPRESSION))
> + continue;
I think using virTypedParamsGetStringList to get all
VIR_MIGRATE_PARAM_COMPRESSION parameters and then walking through them
in a loop would look a bit nicer.
> +
> + if (STREQ(params[i].value.s, "xbzrle")) {
> + if (qemuMigrationCompressMethodsAdd(&compression->methods,
> + QEMU_MIGRATION_COMPESS_XBZRLE) < 0)
> + return -1;
> + } else if (STREQ(params[i].value.s, "multithread")) {
> + if (qemuMigrationCompressMethodsAdd(&compression->methods,
> + QEMU_MIGRATION_COMPESS_MULTITHREAD) < 0)
> + return -1;
> + } else {
> + virReportError(VIR_ERR_INVALID_ARG, "%s",
> + _("Unsupported compression method"));
> + return -1;
> + }
Anyway, this is very ugly. You should change
qemuMigrationCompressMethods to be usable with our VIR_ENUM_{IMPL,DECL}
macros and use them here and in the *DumpParams function.
> + }
And set QEMU_MIGRATION_COMPESS_XBZRLE method if no param is passed and
flags enabled compression.
> +
> + return 0;
> +}
> +
> +int
> +qemuMigrationCompressionDumpParams(qemuMigrationCompressionPtr compression,
> + virTypedParameterPtr *params,
> + int *nparams,
> + int *maxparams)
> +{
> + if ((compression->methods & QEMU_MIGRATION_COMPESS_XBZRLE) &&
> + virTypedParamsAddString(params, nparams, maxparams,
> + VIR_MIGRATE_PARAM_COMPRESSION,
> + "xbzrle") < 0)
> + return -1;
> +
> + if ((compression->methods & QEMU_MIGRATION_COMPESS_MULTITHREAD) &&
> + virTypedParamsAddString(params, nparams, maxparams,
> + VIR_MIGRATE_PARAM_COMPRESSION,
> + "multithread") < 0)
> + return -1;
For backward compatibility this would just need to keep params empty if
xbzrle was the only compression method specified and use only flags to
express that.
> +
> + return 0;
> +}
> diff --git a/src/qemu/qemu_migration.h b/src/qemu/qemu_migration.h
> index 2c67a02..3cbe944 100644
> --- a/src/qemu/qemu_migration.h
> +++ b/src/qemu/qemu_migration.h
> @@ -25,6 +25,9 @@
> # include "qemu_conf.h"
> # include "qemu_domain.h"
>
> +typedef struct _qemuMigrationCompression qemuMigrationCompression;
> +typedef qemuMigrationCompression *qemuMigrationCompressionPtr;
> +
> /* All supported qemu migration flags. */
> # define QEMU_MIGRATION_FLAGS \
> (VIR_MIGRATE_LIVE | \
> @@ -53,6 +56,8 @@
> VIR_MIGRATE_PARAM_LISTEN_ADDRESS, VIR_TYPED_PARAM_STRING, \
> VIR_MIGRATE_PARAM_MIGRATE_DISKS, VIR_TYPED_PARAM_STRING | \
> VIR_TYPED_PARAM_MULTIPLE, \
> + VIR_MIGRATE_PARAM_COMPRESSION, VIR_TYPED_PARAM_STRING | \
> + VIR_TYPED_PARAM_MULTIPLE, \
> NULL
>
>
> @@ -72,6 +77,22 @@ typedef enum {
> } qemuMigrationJobPhase;
> VIR_ENUM_DECL(qemuMigrationJobPhase)
>
> +typedef enum {
> + QEMU_MIGRATION_COMPESS_XBZRLE = (1 << 0),
> + QEMU_MIGRATION_COMPESS_MULTITHREAD = (1 << 1),
> +} qemuMigrationCompressMethods;
> +
> +struct _qemuMigrationCompression {
> + qemuMigrationCompressMethods methods;
This should rather be unsigned long (or unsigned long long).
> +};
> +
> +int qemuMigrationCompressionParseParams(qemuMigrationCompressionPtr compression,
> + virTypedParameterPtr params, int nparams);
> +int qemuMigrationCompressionDumpParams(qemuMigrationCompressionPtr compression,
> + virTypedParameterPtr *params,
> + int *nparams,
> + int *maxparams);
> +
> int qemuMigrationJobStart(virQEMUDriverPtr driver,
> virDomainObjPtr vm,
> qemuDomainAsyncJob job)
...
Jirka
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list