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@xxxxxxxxxx>
> ---
> 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
