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

On 18.03.2016 16:47, Jiri Denemark wrote: >> 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. > 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.

>> + >> + 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. > 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?

>> + >> + 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. 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?

> >> + } > > 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. 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.