Re: [PATCH 3/4] qemu: support set parallel migration compression method

On Fri, Jan 20, 2023 at 16:47:42 +0800, Jiang Jiacheng wrote: > Add 'qemuMigrationParamsSetParallelCompression' to support set > parallel migration compression method. Depending on whether '--parallel' > is set, we invoke different functions to select compression > method from the same param 'VIR_MIGRATE_PARAM_COMPRESSION'. > > Signed-off-by: Jiang Jiacheng <jiangjiacheng(a)huawei.com&gt; > --- > src/qemu/qemu_migration_params.c | 76 +++++++++++++++++++++++++++++++- > 1 file changed, 74 insertions(+), 2 deletions(-) > > diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c > index 380fc7dccd..4980761712 100644 > --- a/src/qemu/qemu_migration_params.c > +++ b/src/qemu/qemu_migration_params.c > @@ -71,6 +71,8 @@ struct _qemuMigrationParams { > typedef enum { > QEMU_MIGRATION_COMPRESS_XBZRLE = 0, > QEMU_MIGRATION_COMPRESS_MT, > + QEMU_MIGRATION_COMPRESS_ZLIB, > + QEMU_MIGRATION_COMPRESS_ZSTD, > > QEMU_MIGRATION_COMPRESS_LAST > } qemuMigrationCompressMethod; > @@ -79,6 +81,8 @@ VIR_ENUM_IMPL(qemuMigrationCompressMethod, > QEMU_MIGRATION_COMPRESS_LAST, > "xbzrle", > "mt", > + "zlib", > + "zstd", > ); > > VIR_ENUM_IMPL(qemuMigrationCapability, > @@ -524,6 +528,60 @@ qemuMigrationParamsSetTPString(qemuMigrationParams *migParams, > migParams->params[param].value.s); > } > > +static int > +qemuMigrationParamsSetParallelCompression(virTypedParameterPtr params, > + int nparams, > + qemuMigrationParams *migParams) > +{ > + size_t i; > + int method; > + const char *value = NULL; > + int rc; > + This function is very similar to qemuMigrationParamsSetCompression and you need to handle the same values in both functions. Merge them into a single one that would handle all compression methods and whether they are allowed or not depending on the API flags.

> + for (i = 0; i < nparams; i++) { > + if (STRNEQ(params[i].field, VIR_MIGRATE_PARAM_COMPRESSION)) > + continue; > + if (migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_COMPRESSION].set) { > + virReportError(VIR_ERR_INVALID_ARG, "%s", > + _("Only one compression method could be specified with " > + "multifd compression")); > + return -1; > + } > + > + method = qemuMigrationCompressMethodTypeFromString(params[i].value.s); > + if (method < 2) { Even though method is declared as int, it is an enum in reality. We store it in int because the API for converting a string to the corresponding enum value can return -1. Thus you should always use enum items in comparisons rather than magic integer values. And exact checks are much better than < or > as they don't break when new method gets introduced. And I think it's better to let the for loop translate VIR_MIGRATE_PARAM_COMPRESSION into migParams->compMethods and do the checks afterwards.

> + virReportError(VIR_ERR_INVALID_ARG, > + _("Unsupported compression method '%s' with multifd migration"), > + params[i].value.s); > + return -1; > + } > + > + migParams->compMethods |= 1ULL << method; > + > + if ((rc = virTypedParamsGetString(params, nparams, > + VIR_MIGRATE_PARAM_COMPRESSION, &value)) < 0) > + return -1; > + > + migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_COMPRESSION].value.s = g_strdup(value); > + migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_COMPRESSION].set = !!rc; > + } > + > + if (migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_ZLIB_LEVEL].set && > + !(migParams->compMethods & (1ULL << QEMU_MIGRATION_COMPRESS_ZLIB))) { > + virReportError(VIR_ERR_INVALID_ARG, "%s", > + _("Turn zlib compression on to tune it")); > + return -1; > + } > + > + if (migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_ZSTD_LEVEL].set && > + !(migParams->compMethods & (1ULL << QEMU_MIGRATION_COMPRESS_ZSTD))) { > + virReportError(VIR_ERR_INVALID_ARG, "%s", > + _("Turn zstd compression on to tune it")); > + return -1; > + } > + > + return 0; > +} > > > static int > @@ -548,6 +606,13 @@ qemuMigrationParamsSetCompression(virTypedParameterPtr params, > return -1; > } > > + if (method > 1) { > + virReportError(VIR_ERR_INVALID_ARG, > + _("Trun parallel migration on to use compression method '%s'"), > + params[i].value.s); > + return -1; > + } > + > if (migParams->compMethods & (1ULL << method)) { > virReportError(VIR_ERR_INVALID_ARG, > _("Compression method '%s' is specified twice"), > @@ -566,6 +631,8 @@ qemuMigrationParamsSetCompression(virTypedParameterPtr params, > cap = QEMU_MIGRATION_CAP_COMPRESS; > break; > > + case QEMU_MIGRATION_COMPRESS_ZLIB: > + case QEMU_MIGRATION_COMPRESS_ZSTD: > case QEMU_MIGRATION_COMPRESS_LAST: > default: > continue; > @@ -691,8 +758,13 @@ qemuMigrationParamsFromFlags(virTypedParameterPtr params, > return NULL; > } > > - if (qemuMigrationParamsSetCompression(params, nparams, flags, migParams) < 0) > - return NULL; > + if (flags & VIR_MIGRATE_PARALLEL) { > + if (qemuMigrationParamsSetParallelCompression(params, nparams, migParams) < 0) > + return NULL; > + } else { > + if (qemuMigrationParamsSetCompression(params, nparams, flags, migParams) < 0) > + return NULL; > + } As I said just handle all compression methods and options in the existing qemuMigrationParamsSetCompression function. > > return g_steal_pointer(&migParams); > } Jirka