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

[Jiri Denemark]

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 at huawei.com>
> ---
>  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