Re: [libvirt] [PATCH v4 1/5] migration: add compress method option

On Fri, Mar 04, 2016 at 14:20:54 +0300, Nikolay Shirokovskiy wrote: 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. 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, ... 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