[PATCH v2 2/3] lib: Repurpose virDomainSaveParams() with no VIR_DOMAIN_SAVE_PARAM_FILE

[Claudio Fontana]

On 5/13/22 9:54 AM, Michal Privoznik wrote:
> When no VIR_DOMAIN_SAVE_PARAM_FILE typed param is set when
> calling virDomainSaveParams() then in turn virQEMUFileOpenAs()
> tries to open a NULL path.
> 
> We have two options now:
> 1) require the typed param, which in turn may be promoted to a
>    regular argument, or
> 
> 2) use this opportunity to make the API behave like
>    virDomainManagedSave() and use typed params to pass extra
>    arguments, instead of having to invent new managed save API
>    with typed params.
> 
> Let's go with option 2, as it is more future proof.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  src/libvirt-domain.c   |  2 ++
>  src/qemu/qemu_driver.c | 33 ++++++++++++++++++++-------------
>  2 files changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index d1d62daa71..208c2438fe 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -1007,6 +1007,8 @@ virDomainSaveFlags(virDomainPtr domain, const char *to,
>   * @flags: bitwise-OR of virDomainSaveRestoreFlags
>   *
>   * This method extends virDomainSaveFlags by adding parameters.
> + * If VIR_DOMAIN_SAVE_PARAM_FILE is not provided then a managed save is
> + * performed (see virDomainManagedSave).
>   *
>   * Returns 0 in case of success and -1 in case of failure.
>   *
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 7d8c7176d9..0b31c73bb9 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -2772,6 +2772,7 @@ qemuDomainManagedSavePath(virQEMUDriver *driver, virDomainObj *vm)
>  static int
>  qemuDomainManagedSaveHelper(virQEMUDriver *driver,
>                              virDomainObj *vm,
> +                            const char *dxml,
>                              unsigned int flags)
>  {
>      g_autoptr(virQEMUDriverConfig) cfg = NULL;
> @@ -2799,7 +2800,7 @@ qemuDomainManagedSaveHelper(virQEMUDriver *driver,
>      VIR_INFO("Saving state of domain '%s' to '%s'", vm->def->name, path);
>  
>      if (qemuDomainSaveInternal(driver, vm, path, compressed,
> -                                 compressor, NULL, flags) < 0)
> +                                 compressor, dxml, flags) < 0)
>          return -1;
>  
>      vm->hasManagedSave = true;
> @@ -2853,17 +2854,18 @@ qemuDomainSave(virDomainPtr dom, const char *path)
>  
>  static int
>  qemuDomainSaveParams(virDomainPtr dom,
> -                     virTypedParameterPtr params, int nparams,
> +                     virTypedParameterPtr params,
> +                     int nparams,
>                       unsigned int flags)
>  {
> +    virQEMUDriver *driver = dom->conn->privateData;
> +    g_autoptr(virQEMUDriverConfig) cfg = NULL;
> +    virDomainObj *vm = NULL;
> +    g_autoptr(virCommand) compressor = NULL;
>      const char *to = NULL;
>      const char *dxml = NULL;
> -    virQEMUDriver *driver = dom->conn->privateData;
>      int compressed;
> -    g_autoptr(virCommand) compressor = NULL;
>      int ret = -1;
> -    virDomainObj *vm = NULL;
> -    g_autoptr(virQEMUDriverConfig) cfg = NULL;
>  
>      virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE |
>                    VIR_DOMAIN_SAVE_RUNNING |
> @@ -2884,18 +2886,23 @@ qemuDomainSaveParams(virDomainPtr dom,
>                                  VIR_DOMAIN_SAVE_PARAM_DXML, &dxml) < 0)
>          return -1;
>  
> -    cfg = virQEMUDriverGetConfig(driver);
> -    if ((compressed = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat,
> -                                                         &compressor,
> -                                                         "save", false)) < 0)
> -        goto cleanup;
> -
>      if (!(vm = qemuDomainObjFromDomain(dom)))
>          goto cleanup;
>  
>      if (virDomainSaveParamsEnsureACL(dom->conn, vm->def) < 0)
>          goto cleanup;
>  
> +    if (!to) {
> +        /* If no save path was provided then this behaves as managed save. */
> +        return qemuDomainManagedSaveHelper(driver, vm, dxml, flags);
> +    }

Is this sufficient? In some cases I am seeing that omitted string parameters are passed as the empty string.
In my multifd series I encountered this with the optional --parallel-compression argument,

that in this case would mean:

if (!to || !to[0])

Thanks,

Claudio

> +
> +    cfg = virQEMUDriverGetConfig(driver);
> +    if ((compressed = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat,
> +                                                         &compressor,
> +                                                         "save", false)) < 0)
> +        goto cleanup;
> +
>      if (virDomainObjCheckActive(vm) < 0)
>          goto cleanup;
>  
> @@ -2925,7 +2932,7 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags)
>      if (virDomainManagedSaveEnsureACL(dom->conn, vm->def) < 0)
>          goto cleanup;
>  
> -    ret = qemuDomainManagedSaveHelper(driver, vm, flags);
> +    ret = qemuDomainManagedSaveHelper(driver, vm, NULL, flags);
>  
>   cleanup:
>      virDomainObjEndAPI(&vm);
>