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

On Fri, Feb 24, 2023 at 17:27:12 +0800, Jiang Jiacheng wrote: ... ... Spurious whitespace change. The error message should all be on a single line. And I'd move this check a bit further below the two new compatibility checks: [1]. Recent changes (made after you sent this patch) require %1$s format string to be used instead of %s [1] We do not magically enable flags based on other flags. We just report an error if the required flag is not set. The same here. And same here (for both flags and cap); The idea was to consistently use --compressed --comp-method=... --comp-...-... regardless on selected compression method or whether parallel migration is turned on or not. Specifically, --parallel --compressed --comp-method=zlib --comp-zlib-... --parallel --compressed --comp-method=zstd --comp-zstd-... --compressed --comp-method=mt --comp-mt-... --compressed --comp-method=xbzrle,mt --comp-xbzrle-... --comp-mt-... --compressed are all ok, while --compressed --comp-method=zlib --compressed --comp-method=zstd should report an error about missing --parallel option and --parallel --compressed --comp-method=xbzrle --parallel --compressed --comp-method=mt should report an error saying the selected method cannot be used with parallel migration. Actually looking at the current code (confirmed also by an experiment) the --compressed parameter is not required. It's mostly a shortcut for a default method, which is xbzrle for non-parallel migration. The default for parallel migration is documented as "no compression", which would mean --parallel --compressed is equivalent to --parallel I think it would be better to just forbid --compressed with --parallel unless there is a compression method specified with --comp-method to avoid a strange semantics of --compressed not providing any compression at all. This is not needed as the VIR_MIGRATE_PARALLEL flag has to be set explicitly. With the following suggested changes Reviewed-by: Jiri Denemark <jdenemar(a)redhat.com&gt; diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index ee23cec23d..807cccd86e 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -528,6 +528,8 @@ qemuMigrationParamsSetTPString(qemuMigrationParams *migParams, migParams->params[param].value.s); } + + static int qemuMigrationParamsSetCompression(virTypedParameterPtr params, int nparams, @@ -536,19 +538,11 @@ qemuMigrationParamsSetCompression(virTypedParameterPtr params, { size_t i; int method; - qemuMigrationCapability cap; 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 " - "parallel compression")); - return -1; - } - method = qemuMigrationCompressMethodTypeFromString(params[i].value.s); if (method < 0) { virReportError(VIR_ERR_INVALID_ARG, @@ -568,46 +562,47 @@ qemuMigrationParamsSetCompression(virTypedParameterPtr params, method == QEMU_MIGRATION_COMPRESS_XBZRLE) && flags & VIR_MIGRATE_PARALLEL) { virReportError(VIR_ERR_INVALID_ARG, - _("Compression method '%s' isn't supported with parallel migration"), + _("Compression method '%1$s' isn't supported with parallel migration"), params[i].value.s); return -1; } if ((method == QEMU_MIGRATION_COMPRESS_ZLIB || method == QEMU_MIGRATION_COMPRESS_ZSTD) && - flags & VIR_MIGRATE_COMPRESSED) { + !(flags & VIR_MIGRATE_PARALLEL)) { virReportError(VIR_ERR_INVALID_ARG, - _("Compression method '%s' isn't supported with compress migration"), + _("Compression method '%1$s' is only supported with parallel migration"), params[i].value.s); return -1; } + if (migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_COMPRESSION].set) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("Only one compression method could be specified with parallel compression")); + return -1; + } + migParams->compMethods |= 1ULL << method; switch ((qemuMigrationCompressMethod) method) { case QEMU_MIGRATION_COMPRESS_XBZRLE: - cap = QEMU_MIGRATION_CAP_XBZRLE; - flags |= VIR_MIGRATE_COMPRESSED; + ignore_value(virBitmapSetBit(migParams->caps, QEMU_MIGRATION_CAP_XBZRLE)); break; case QEMU_MIGRATION_COMPRESS_MT: - cap = QEMU_MIGRATION_CAP_COMPRESS; - flags |= VIR_MIGRATE_COMPRESSED; + ignore_value(virBitmapSetBit(migParams->caps, QEMU_MIGRATION_CAP_COMPRESS)); break; case QEMU_MIGRATION_COMPRESS_ZLIB: case QEMU_MIGRATION_COMPRESS_ZSTD: - flags |= VIR_MIGRATE_PARALLEL; - cap = QEMU_MIGRATION_CAP_MULTIFD; migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_COMPRESSION].value.s = g_strdup(params[i].value.s); migParams->params[QEMU_MIGRATION_PARAM_MULTIFD_COMPRESSION].set = true; break; case QEMU_MIGRATION_COMPRESS_LAST: default: - continue; + break; } - ignore_value(virBitmapSetBit(migParams->caps, cap)); } if ((migParams->params[QEMU_MIGRATION_PARAM_COMPRESS_LEVEL].set || @@ -641,6 +636,12 @@ qemuMigrationParamsSetCompression(virTypedParameterPtr params, } if (!migParams->compMethods && (flags & VIR_MIGRATE_COMPRESSED)) { + if (flags & VIR_MIGRATE_PARALLEL) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("No compression algorithm selected for parallel migration")); + return -1; + } + migParams->compMethods = 1ULL << QEMU_MIGRATION_COMPRESS_XBZRLE; ignore_value(virBitmapSetBit(migParams->caps, QEMU_MIGRATION_CAP_XBZRLE)); @@ -761,11 +762,6 @@ qemuMigrationParamsDump(qemuMigrationParams *migParams, *flags |= VIR_MIGRATE_COMPRESSED; } - if (migParams->compMethods == 1ULL << QEMU_MIGRATION_COMPRESS_ZLIB || - migParams->compMethods == 1ULL << QEMU_MIGRATION_COMPRESS_ZSTD) { - *flags |= VIR_MIGRATE_PARALLEL; - } - for (i = 0; i < QEMU_MIGRATION_COMPRESS_LAST; ++i) { if ((migParams->compMethods & (1ULL << i)) && virTypedParamsAddString(params, nparams, maxparams,