Re: [PATCH 3/3] lib: Require path in virDomainRestoreParams()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, May 12, 2022 at 05:17:37PM +0200, Michal Privoznik wrote:
> After seeing previous commit one might think that
> virDomainRestoreParams() would get the similar treatment. Well,
> it can't. The problem here is: without any indication what domain
> to restore we don't really know what domain to restore (shocking,
> right?). Therefore, we have to require path to restore domain
> from, at which point, we can save callers couple of lines and let
> them pass the path as a regular argument instead of requiring it
> in typed params.

Currently for managed save restore, it happens magically
during virDomainCreate.

We don't have a way to pass parameter so that though,
so can parallelize, etc.

We could make a virDomainCreateParams to handle this,
or we could make virDOmainRestore handle this by
adding a 'domain-uuid' parameter as an alternative
to the path.

IOW, it is possibly a good thing for future proofing that
the path is a typed param rather than positional param.

> 
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
>  include/libvirt/libvirt-domain.h |  1 +
>  src/driver-hypervisor.h          |  1 +
>  src/libvirt-domain.c             | 25 +++++++++++++++++++++----
>  src/qemu/qemu_driver.c           |  9 +++------
>  src/remote/remote_protocol.x     |  1 +
>  src/remote_protocol-structs      |  1 +
>  src/rpc/gendispatch.pl           |  2 +-
>  7 files changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/include/libvirt/libvirt-domain.h b/include/libvirt/libvirt-domain.h
> index 24846046aa..08082627e5 100644
> --- a/include/libvirt/libvirt-domain.h
> +++ b/include/libvirt/libvirt-domain.h
> @@ -1574,6 +1574,7 @@ int                     virDomainRestoreFlags   (virConnectPtr conn,
>                                                   const char *dxml,
>                                                   unsigned int flags);
>  int                     virDomainRestoreParams  (virConnectPtr conn,
> +                                                 const char *path,
>                                                   virTypedParameterPtr params,
>                                                   int nparams,
>                                                   unsigned int flags);
> diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h
> index 69516e8fea..81c20a749e 100644
> --- a/src/driver-hypervisor.h
> +++ b/src/driver-hypervisor.h
> @@ -258,6 +258,7 @@ typedef int
>  
>  typedef int
>  (*virDrvDomainRestoreParams)(virConnectPtr conn,
> +                             const char *path,
>                               virTypedParameterPtr params,
>                               int nparams,
>                               unsigned int flags);
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 208c2438fe..481886eb02 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -1186,11 +1186,14 @@ virDomainRestoreFlags(virConnectPtr conn, const char *from, const char *dxml,
>  /**
>   * virDomainRestoreParams:
>   * @conn: pointer to the hypervisor connection
> + * @path: path to the input file
>   * @params: restore parameters
>   * @nparams: number of restore parameters
>   * @flags: bitwise-OR of virDomainSaveRestoreFlags
>   *
>   * This method extends virDomainRestoreFlags by adding parameters.
> + * Please note that VIR_DOMAIN_SAVE_PARAM_FILE is not supported for this API as
> + * @path serves the same purpose.
>   *
>   * Returns 0 in case of success and -1 in case of failure.
>   *
> @@ -1198,25 +1201,39 @@ virDomainRestoreFlags(virConnectPtr conn, const char *from, const char *dxml,
>   */
>  int
>  virDomainRestoreParams(virConnectPtr conn,
> -                       virTypedParameterPtr params, int nparams,
> +                       const char *path,
> +                       virTypedParameterPtr params,
> +                       int nparams,
>                         unsigned int flags)
>  {
> -    VIR_DEBUG("conn=%p, params=%p, nparams=%d, flags=0x%x",
> -              conn, params, nparams, flags);
> +    VIR_DEBUG("conn=%p, path=%s, params=%p, nparams=%d, flags=0x%x",
> +              conn, path, params, nparams, flags);
>      VIR_TYPED_PARAMS_DEBUG(params, nparams);
>  
>      virResetLastError();
>  
>      virCheckConnectReturn(conn, -1);
>      virCheckReadOnlyGoto(conn->flags, error);
> +    virCheckNonNullArgGoto(path, error);
>  
>      VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SAVE_RUNNING,
>                               VIR_DOMAIN_SAVE_PAUSED,
>                               error);
>  
>      if (conn->driver->domainRestoreParams) {
> -        if (conn->driver->domainRestoreParams(conn, params, nparams, flags) < 0)
> +        g_autofree char *absolute_path = NULL;
> +
> +        /* We must absolutize the file path as the restore is done out of process */
> +        if (!(absolute_path = g_canonicalize_filename(path, NULL))) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("could not build absolute input file path"));
>              goto error;
> +        }
> +
> +        if (conn->driver->domainRestoreParams(conn, absolute_path,
> +                                              params, nparams, flags) < 0)
> +            goto error;
> +
>          return 0;
>      }
>  
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 0b31c73bb9..debf96db19 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -5947,22 +5947,19 @@ qemuDomainRestore(virConnectPtr conn,
>  
>  static int
>  qemuDomainRestoreParams(virConnectPtr conn,
> -                        virTypedParameterPtr params, int nparams,
> +                        const char *path,
> +                        virTypedParameterPtr params,
> +                        int nparams,
>                          unsigned int flags)
>  {
> -    const char *path = NULL;
>      const char *dxml = NULL;
>      int ret = -1;
>  
>      if (virTypedParamsValidate(params, nparams,
> -                               VIR_DOMAIN_SAVE_PARAM_FILE, VIR_TYPED_PARAM_STRING,
>                                 VIR_DOMAIN_SAVE_PARAM_DXML, VIR_TYPED_PARAM_STRING,
>                                 NULL) < 0)
>          return -1;
>  
> -    if (virTypedParamsGetString(params, nparams,
> -                                VIR_DOMAIN_SAVE_PARAM_FILE, &path) < 0)
> -        return -1;
>      if (virTypedParamsGetString(params, nparams,
>                                  VIR_DOMAIN_SAVE_PARAM_DXML, &dxml) < 0)
>          return -1;
> diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x
> index 085631c11b..2c7327c1e4 100644
> --- a/src/remote/remote_protocol.x
> +++ b/src/remote/remote_protocol.x
> @@ -1000,6 +1000,7 @@ struct remote_domain_restore_flags_args {
>  };
>  
>  struct remote_domain_restore_params_args {
> +    remote_nonnull_string path;
>      remote_typed_param params<REMOTE_DOMAIN_SAVE_PARAMS_MAX>;
>      unsigned int flags;
>  };
> diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs
> index 4ffdce5679..75e86d48f4 100644
> --- a/src/remote_protocol-structs
> +++ b/src/remote_protocol-structs
> @@ -580,6 +580,7 @@ struct remote_domain_restore_flags_args {
>          u_int                      flags;
>  };
>  struct remote_domain_restore_params_args {
> +        remote_nonnull_string      path;
>          struct {
>                  u_int              params_len;
>                  remote_typed_param * params_val;
> diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl
> index a64ff3e73f..6acefa6b98 100755
> --- a/src/rpc/gendispatch.pl
> +++ b/src/rpc/gendispatch.pl
> @@ -640,7 +640,7 @@ elsif ($mode eq "server") {
>  
>                      # NB: if your new API starts with remote_typed_params, enter it here if you need
>                      # the conn arg to be passed first!
> -                    if ($call->{ProcName} eq "NodeSetMemoryParameters" || $call->{ProcName} eq "DomainRestoreParams") {
> +                    if ($call->{ProcName} eq "NodeSetMemoryParameters") {
>                          push(@args_list, $conn_var);
>                      }
>                      push(@args_list, "$1");
> -- 
> 2.35.1
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux