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 :|
