Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; --- 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 */ 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. + */ + +# 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_driver.c b/src/qemu/qemu_driver.c index 4bd4071..b27b8f0 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12061,7 +12061,7 @@ qemuDomainMigratePrepare2(virConnectPtr dconn, ret = qemuMigrationPrepareDirect(driver, dconn, NULL, 0, NULL, NULL, /* No cookies */ uri_in, uri_out, - &def, origname, NULL, 0, NULL, flags); + &def, origname, NULL, 0, NULL, NULL, flags); cleanup: VIR_FREE(origname); @@ -12114,7 +12114,7 @@ qemuDomainMigratePerform(virDomainPtr dom, * Consume any cookie we were able to decode though */ ret = qemuMigrationPerform(driver, dom->conn, vm, - NULL, dconnuri, uri, NULL, NULL, 0, NULL, + NULL, dconnuri, uri, NULL, NULL, 0, NULL, NULL, cookie, cookielen, NULL, NULL, /* No output cookies in v2 */ flags, dname, resource, false); @@ -12287,7 +12287,7 @@ qemuDomainMigratePrepare3(virConnectPtr dconn, cookiein, cookieinlen, cookieout, cookieoutlen, uri_in, uri_out, - &def, origname, NULL, 0, NULL, flags); + &def, origname, NULL, 0, NULL, NULL, flags); cleanup: VIR_FREE(origname); @@ -12316,6 +12316,7 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn, int nmigrate_disks; const char **migrate_disks = NULL; char *origname = NULL; + qemuMigrationCompressionPtr compression = NULL; int ret = -1; virCheckFlagsGoto(QEMU_MIGRATION_FLAGS, cleanup); @@ -12343,6 +12344,12 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn, if (nmigrate_disks < 0) goto cleanup; + if (VIR_ALLOC(compression) < 0) + goto cleanup; + + if (qemuMigrationCompressionParseParams(compression, params, nparams) < 0) + goto cleanup; + if (flags & VIR_MIGRATE_TUNNELLED) { /* this is a logical error; we never should have gotten here with * VIR_MIGRATE_TUNNELLED set @@ -12364,9 +12371,11 @@ qemuDomainMigratePrepare3Params(virConnectPtr dconn, cookieout, cookieoutlen, uri_in, uri_out, &def, origname, listenAddress, - nmigrate_disks, migrate_disks, flags); + nmigrate_disks, migrate_disks, + compression, flags); cleanup: + VIR_FREE(compression); VIR_FREE(migrate_disks); VIR_FREE(origname); virDomainDefFree(def); @@ -12498,7 +12507,7 @@ qemuDomainMigratePerform3(virDomainPtr dom, } return qemuMigrationPerform(driver, dom->conn, vm, xmlin, - dconnuri, uri, NULL, NULL, 0, NULL, + dconnuri, uri, NULL, NULL, 0, NULL, NULL, cookiein, cookieinlen, cookieout, cookieoutlen, flags, dname, resource, true); @@ -12525,6 +12534,7 @@ qemuDomainMigratePerform3Params(virDomainPtr dom, int nmigrate_disks; const char **migrate_disks = NULL; unsigned long long bandwidth = 0; + qemuMigrationCompressionPtr compression = NULL; int ret = -1; virCheckFlags(QEMU_MIGRATION_FLAGS, -1); @@ -12558,6 +12568,12 @@ qemuDomainMigratePerform3Params(virDomainPtr dom, if (nmigrate_disks < 0) goto cleanup; + if (VIR_ALLOC(compression) < 0) + goto cleanup; + + if (qemuMigrationCompressionParseParams(compression, params, nparams) < 0) + goto cleanup; + if (!(vm = qemuDomObjFromDomain(dom))) goto cleanup; @@ -12568,10 +12584,11 @@ qemuDomainMigratePerform3Params(virDomainPtr dom, ret = qemuMigrationPerform(driver, dom->conn, vm, dom_xml, dconnuri, uri, graphicsuri, listenAddress, - nmigrate_disks, migrate_disks, + nmigrate_disks, migrate_disks, compression, cookiein, cookieinlen, cookieout, cookieoutlen, flags, dname, bandwidth, true); cleanup: + VIR_FREE(compression); VIR_FREE(migrate_disks); return ret; } 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); + } + + 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, @@ -3314,6 +3347,7 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, const char *listenAddress, size_t nmigrate_disks, const char **migrate_disks, + qemuMigrationCompressionPtr compression, unsigned long flags) { virDomainObjPtr vm = NULL; @@ -3495,10 +3529,8 @@ qemuMigrationPrepareAny(virQEMUDriverPtr driver, dataFD[1] = -1; /* 'st' owns the FD now & will close it */ } - if (qemuMigrationSetOption(driver, vm, - QEMU_MONITOR_MIGRATION_CAPS_XBZRLE, - flags & VIR_MIGRATE_COMPRESSED, - QEMU_ASYNC_JOB_MIGRATION_IN) < 0) + if (qemuMigrationSetCompression(driver, vm, QEMU_ASYNC_JOB_MIGRATION_IN, + compression, flags) < 0) goto stopjob; if (STREQ_NULLABLE(protocol, "rdma") && @@ -3637,7 +3669,7 @@ qemuMigrationPrepareTunnel(virQEMUDriverPtr driver, ret = qemuMigrationPrepareAny(driver, dconn, cookiein, cookieinlen, cookieout, cookieoutlen, def, origname, - st, NULL, 0, false, NULL, 0, NULL, flags); + st, NULL, 0, false, NULL, 0, NULL, NULL, flags); return ret; } @@ -3679,6 +3711,7 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, const char *listenAddress, size_t nmigrate_disks, const char **migrate_disks, + qemuMigrationCompressionPtr compression, unsigned long flags) { unsigned short port = 0; @@ -3801,7 +3834,8 @@ qemuMigrationPrepareDirect(virQEMUDriverPtr driver, cookieout, cookieoutlen, def, origname, NULL, uri ? uri->scheme : "tcp", port, autoPort, listenAddress, - nmigrate_disks, migrate_disks, flags); + nmigrate_disks, migrate_disks, compression, + flags); cleanup: virURIFree(uri); VIR_FREE(hostname); @@ -4282,7 +4316,8 @@ qemuMigrationRun(virQEMUDriverPtr driver, virConnectPtr dconn, const char *graphicsuri, size_t nmigrate_disks, - const char **migrate_disks) + const char **migrate_disks, + qemuMigrationCompressionPtr compression) { int ret = -1; unsigned int migrate_flags = QEMU_MONITOR_MIGRATE_BACKGROUND; @@ -4364,10 +4399,8 @@ qemuMigrationRun(virQEMUDriverPtr driver, goto cleanup; } - if (qemuMigrationSetOption(driver, vm, - QEMU_MONITOR_MIGRATION_CAPS_XBZRLE, - flags & VIR_MIGRATE_COMPRESSED, - QEMU_ASYNC_JOB_MIGRATION_OUT) < 0) + if (qemuMigrationSetCompression(driver, vm, QEMU_ASYNC_JOB_MIGRATION_OUT, + compression, flags) < 0) goto cleanup; if (qemuMigrationSetOption(driver, vm, @@ -4581,7 +4614,8 @@ static int doNativeMigrate(virQEMUDriverPtr driver, virConnectPtr dconn, const char *graphicsuri, size_t nmigrate_disks, - const char **migrate_disks) + const char **migrate_disks, + qemuMigrationCompressionPtr compression) { qemuDomainObjPrivatePtr priv = vm->privateData; virURIPtr uribits = NULL; @@ -4631,7 +4665,8 @@ static int doNativeMigrate(virQEMUDriverPtr driver, ret = qemuMigrationRun(driver, vm, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource, &spec, dconn, - graphicsuri, nmigrate_disks, migrate_disks); + graphicsuri, nmigrate_disks, migrate_disks, + compression); if (spec.destType == MIGRATION_DEST_FD) VIR_FORCE_CLOSE(spec.dest.fd.qemu); @@ -4655,7 +4690,8 @@ static int doTunnelMigrate(virQEMUDriverPtr driver, virConnectPtr dconn, const char *graphicsuri, size_t nmigrate_disks, - const char **migrate_disks) + const char **migrate_disks, + qemuMigrationCompressionPtr compression) { virNetSocketPtr sock = NULL; int ret = -1; @@ -4692,7 +4728,8 @@ static int doTunnelMigrate(virQEMUDriverPtr driver, ret = qemuMigrationRun(driver, vm, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource, &spec, dconn, - graphicsuri, nmigrate_disks, migrate_disks); + graphicsuri, nmigrate_disks, migrate_disks, + compression); cleanup: if (spec.destType == MIGRATION_DEST_FD) { @@ -4803,12 +4840,12 @@ static int doPeer2PeerMigrate2(virQEMUDriverPtr driver, ret = doTunnelMigrate(driver, vm, st, NULL, 0, NULL, NULL, flags, resource, dconn, - NULL, 0, NULL); + NULL, 0, NULL, NULL); else ret = doNativeMigrate(driver, vm, uri_out, cookie, cookielen, NULL, NULL, /* No out cookie with v2 migration */ - flags, resource, dconn, NULL, 0, NULL); + flags, resource, dconn, NULL, 0, NULL, NULL); /* Perform failed. Make sure Finish doesn't overwrite the error */ if (ret < 0) @@ -4872,6 +4909,7 @@ doPeer2PeerMigrate3(virQEMUDriverPtr driver, const char *listenAddress, size_t nmigrate_disks, const char **migrate_disks, + qemuMigrationCompressionPtr compression, unsigned long long bandwidth, bool useParams, unsigned long flags) @@ -4949,6 +4987,10 @@ doPeer2PeerMigrate3(virQEMUDriverPtr driver, VIR_MIGRATE_PARAM_MIGRATE_DISKS, migrate_disks[i]) < 0) goto cleanup; + if (compression && + qemuMigrationCompressionDumpParams(compression, &params, &nparams, + &maxparams) < 0) + goto cleanup; } if (virDomainObjGetState(vm, NULL) == VIR_DOMAIN_PAUSED) @@ -5034,13 +5076,13 @@ doPeer2PeerMigrate3(virQEMUDriverPtr driver, cookiein, cookieinlen, &cookieout, &cookieoutlen, flags, bandwidth, dconn, graphicsuri, - nmigrate_disks, migrate_disks); + nmigrate_disks, migrate_disks, compression); } else { ret = doNativeMigrate(driver, vm, uri, cookiein, cookieinlen, &cookieout, &cookieoutlen, flags, bandwidth, dconn, graphicsuri, - nmigrate_disks, migrate_disks); + nmigrate_disks, migrate_disks, compression); } /* Perform failed. Make sure Finish doesn't overwrite the error */ @@ -5215,6 +5257,7 @@ static int doPeer2PeerMigrate(virQEMUDriverPtr driver, const char *listenAddress, size_t nmigrate_disks, const char **migrate_disks, + qemuMigrationCompressionPtr compression, unsigned long flags, const char *dname, unsigned long resource, @@ -5328,8 +5371,8 @@ static int doPeer2PeerMigrate(virQEMUDriverPtr driver, if (*v3proto) { ret = doPeer2PeerMigrate3(driver, sconn, dconn, dconnuri, vm, xmlin, dname, uri, graphicsuri, listenAddress, - nmigrate_disks, migrate_disks, resource, - useParams, flags); + nmigrate_disks, migrate_disks, compression, + resource, useParams, flags); } else { ret = doPeer2PeerMigrate2(driver, sconn, dconn, vm, dconnuri, flags, dname, resource); @@ -5366,6 +5409,7 @@ qemuMigrationPerformJob(virQEMUDriverPtr driver, const char *listenAddress, size_t nmigrate_disks, const char **migrate_disks, + qemuMigrationCompressionPtr compression, const char *cookiein, int cookieinlen, char **cookieout, @@ -5401,13 +5445,13 @@ qemuMigrationPerformJob(virQEMUDriverPtr driver, if ((flags & (VIR_MIGRATE_TUNNELLED | VIR_MIGRATE_PEER2PEER))) { ret = doPeer2PeerMigrate(driver, conn, vm, xmlin, dconnuri, uri, graphicsuri, listenAddress, - nmigrate_disks, migrate_disks, + nmigrate_disks, migrate_disks, compression, flags, dname, resource, &v3proto); } else { qemuMigrationJobSetPhase(driver, vm, QEMU_MIGRATION_PHASE_PERFORM2); ret = doNativeMigrate(driver, vm, uri, cookiein, cookieinlen, cookieout, cookieoutlen, - flags, resource, NULL, NULL, 0, NULL); + flags, resource, NULL, NULL, 0, NULL, NULL); } if (ret < 0) goto endjob; @@ -5468,6 +5512,7 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver, const char *graphicsuri, size_t nmigrate_disks, const char **migrate_disks, + qemuMigrationCompressionPtr compression, const char *cookiein, int cookieinlen, char **cookieout, @@ -5493,7 +5538,7 @@ qemuMigrationPerformPhase(virQEMUDriverPtr driver, ret = doNativeMigrate(driver, vm, uri, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource, NULL, graphicsuri, - nmigrate_disks, migrate_disks); + nmigrate_disks, migrate_disks, compression); if (ret < 0) { if (qemuMigrationRestoreDomainState(conn, vm)) { @@ -5535,6 +5580,7 @@ qemuMigrationPerform(virQEMUDriverPtr driver, const char *listenAddress, size_t nmigrate_disks, const char **migrate_disks, + qemuMigrationCompressionPtr compression, const char *cookiein, int cookieinlen, char **cookieout, @@ -5564,7 +5610,7 @@ qemuMigrationPerform(virQEMUDriverPtr driver, return qemuMigrationPerformJob(driver, conn, vm, xmlin, dconnuri, uri, graphicsuri, listenAddress, nmigrate_disks, migrate_disks, - cookiein, cookieinlen, + compression, cookiein, cookieinlen, cookieout, cookieoutlen, flags, dname, resource, v3proto); } else { @@ -5578,14 +5624,14 @@ qemuMigrationPerform(virQEMUDriverPtr driver, return qemuMigrationPerformPhase(driver, conn, vm, uri, graphicsuri, nmigrate_disks, migrate_disks, - cookiein, cookieinlen, + compression, cookiein, cookieinlen, cookieout, cookieoutlen, flags, resource); } else { return qemuMigrationPerformJob(driver, conn, vm, xmlin, NULL, uri, graphicsuri, listenAddress, nmigrate_disks, migrate_disks, - cookiein, cookieinlen, + compression, cookiein, cookieinlen, cookieout, cookieoutlen, flags, dname, resource, v3proto); } @@ -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; +} + +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; + + 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; + } + } + + 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; + + 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; +}; + +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) @@ -134,6 +155,7 @@ int qemuMigrationPrepareDirect(virQEMUDriverPtr driver, const char *listenAddress, size_t nmigrate_disks, const char **migrate_disks, + qemuMigrationCompressionPtr compression, unsigned long flags); int qemuMigrationPerform(virQEMUDriverPtr driver, @@ -146,6 +168,7 @@ int qemuMigrationPerform(virQEMUDriverPtr driver, const char *listenAddress, size_t nmigrate_disks, const char **migrate_disks, + qemuMigrationCompressionPtr compression, const char *cookiein, int cookieinlen, char **cookieout, diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index ace3bb4..5e4461a 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -167,7 +167,7 @@ VIR_ENUM_IMPL(qemuMonitorMigrationStatus, VIR_ENUM_IMPL(qemuMonitorMigrationCaps, QEMU_MONITOR_MIGRATION_CAPS_LAST, - "xbzrle", "auto-converge", "rdma-pin-all", "events") + "xbzrle", "auto-converge", "rdma-pin-all", "events", "compress") VIR_ENUM_IMPL(qemuMonitorVMStatus, QEMU_MONITOR_VM_STATUS_LAST, diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 4467a41..28620b5 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -531,6 +531,7 @@ typedef enum { QEMU_MONITOR_MIGRATION_CAPS_AUTO_CONVERGE, QEMU_MONITOR_MIGRATION_CAPS_RDMA_PIN_ALL, QEMU_MONITOR_MIGRATION_CAPS_EVENTS, + QEMU_MONITOR_MIGRATION_CAPS_COMPRESS, QEMU_MONITOR_MIGRATION_CAPS_LAST } qemuMonitorMigrationCaps; -- 1.8.3.1

On 18.03.2016 16:47, Jiri Denemark wrote: Hi, Jiri. Thanx for reviewing. I think we'd better stick with this approach. The reason is that we can get here thru old API where only flags are set and compression parameter is set to NULL on the way. As to explicitly setting state of all compression methods we can resolve flag into qemuMigrationCompressMethods and use same code as in case of custom compression. Well, this way we can remove the the check above at the price of introducing variables and memory managment. Also we are not going to use the extracted list outside of the function. Is it worth the effort? AFAIU I can not use VIR_ENUM_* family because I have flags and not sequential values in enum. I can introduce another family and set of related functions to deal with flags. Some generics to parse flags to virTypedParameterPtr back and forth. Do you think it is time to take this way? I suggest that we don't mix flags and compression parameters here and in parse function too. This way we do not get backward compatibility issues. If VIR_MIGRATE_COMPRESSED is specified we don't touch it and do not convert to compression parameters. This way the old value space is passed transparently by the old means. On dump compression.methods will be 0 and we do not add params unrecognized by older libvirt. If we start to mix compression in flags and parameters we have check on dump that new value space is representable in old value space and probably have to keep this place up to date with compression methods and their tunables. The only drawback I see on this path is that we have somewhat obscure check 'compression == NULL || compression->methods == 0) in qemuMigrationSetCompression(why == 0 check for?). The first check is for the non params API calls where compression is just not setted. The second is for params API but when compression is not specified by newly introduced means and we should fallback to handling flag value.

On 18.03.2016 17:45, Jiri Denemark wrote: Ouch, this reveals some more misdesigned stuff. Look, I put qemuMonitorMigrationParameters into qemuMigrationCompression which is a kind of reverse aggregation. I see two options to make things the proper way here. 1. Rename qemuMonitorMigrationParameters, qemuMonitorSetMigrationParameters etc to add compression to the naming somehow. If actual qemu command will one day be extended to support parameters from different category - well we just add another json monitor command that will get/set the new subset of parametes. 2. Rework code so that we will aggregate all migration parameters into qemuMonitorSetMigrationParameters value and call actual qemu command only once. This way either we pack all compression values into one structure and have code that fills migration parameters value appropriately or we stop keeping compression related parameters in qemuMigrationCompression. I don't like any of these. So i prefer the first option.

On Fri, Mar 18, 2016 at 15:45:39 +0100, Jiri Denemark wrote: [...] Also booleans are preferred in comparison to this approach.

On Fri, Mar 04, 2016 at 14:20:57 +0300, Nikolay Shirokovskiy wrote: Looks like a leftover '*' --------------------------------^ 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. ... 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. Anyway, the macro needs to be enclosed in do { } while () so that you can use semicolon when calling it. Semicolon should go here ------------------------^ We don't use '.' at the end of error messages. And I'd also avoid pointing at the user with "You". Ditto. ... Jirka

On Fri, Mar 04, 2016 at 14:20:58 +0300, Nikolay Shirokovskiy wrote: Looks a bit long to me, personally I'd prefer "comp-" prefix, feel free to ignore my preference though :-) Anyway, this is a rare exception when it makes sense not to put the new stuff at the end of the list. All the new options should go just after --compressed options so that they are all together. s/method/methods/ I think the options should contain the method name rather than some abbreviated form of it. The above description is IMHO enough, the rest can go into virsh man page. Same here. Here. And here. It would be more compact and still readable and clear as char **val = virStringSplit(opt, ",", 0); goto out; goto out; goto out; goto out; I'd say something like: --compressed activates compression, the compression method is chosen with --compression-method. When no method is specified, a hypervisor default method will be used. --compressed and --compression-* options should be documented next to each other and ideally in a separate paragraph. This should go into the same place where the options are documented. Jirka