[PATCH 0/3] virDomain{Save,Restore}Params: More fixes
*** BLURB HERE *** Michal Prívozník (3): qemu: Separate out save code from qemuDomainManagedSave() lib: Repurpose virDomainSaveParams() with no VIR_DOMAIN_SAVE_PARAM_FILE lib: Require path in virDomainRestoreParams() include/libvirt/libvirt-domain.h | 1 + src/driver-hypervisor.h | 1 + src/libvirt-domain.c | 27 +++++-- src/qemu/qemu_driver.c | 120 ++++++++++++++++++------------- src/remote/remote_protocol.x | 1 + src/remote_protocol-structs | 1 + src/rpc/gendispatch.pl | 2 +- 7 files changed, 97 insertions(+), 56 deletions(-) -- 2.35.1
The code that actually does managed save within qemuDomainManagedSave() is going to be reused shortly. Move it out into a separate helper. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 82 ++++++++++++++++++++++++------------------ 1 file changed, 48 insertions(+), 34 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4b690a520b..7d8c7176d9 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2760,6 +2760,53 @@ qemuDomainSaveInternal(virQEMUDriver *driver, } +static char * +qemuDomainManagedSavePath(virQEMUDriver *driver, virDomainObj *vm) +{ + g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); + + return g_strdup_printf("%s/%s.save", cfg->saveDir, vm->def->name); +} + + +static int +qemuDomainManagedSaveHelper(virQEMUDriver *driver, + virDomainObj *vm, + unsigned int flags) +{ + g_autoptr(virQEMUDriverConfig) cfg = NULL; + g_autoptr(virCommand) compressor = NULL; + g_autofree char *path = NULL; + int compressed; + + if (virDomainObjCheckActive(vm) < 0) + return -1; + + if (!vm->persistent) { + virReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot do managed save for transient domain")); + return -1; + } + + cfg = virQEMUDriverGetConfig(driver); + if ((compressed = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, + &compressor, + "save", false)) < 0) + return -1; + + path = qemuDomainManagedSavePath(driver, vm); + + VIR_INFO("Saving state of domain '%s' to '%s'", vm->def->name, path); + + if (qemuDomainSaveInternal(driver, vm, path, compressed, + compressor, NULL, flags) < 0) + return -1; + + vm->hasManagedSave = true; + return 0; +} + + static int qemuDomainSaveFlags(virDomainPtr dom, const char *path, const char *dxml, unsigned int flags) @@ -2860,23 +2907,12 @@ qemuDomainSaveParams(virDomainPtr dom, return ret; } -static char * -qemuDomainManagedSavePath(virQEMUDriver *driver, virDomainObj *vm) -{ - g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); - - return g_strdup_printf("%s/%s.save", cfg->saveDir, vm->def->name); -} static int qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) { virQEMUDriver *driver = dom->conn->privateData; - g_autoptr(virQEMUDriverConfig) cfg = NULL; - int compressed; - g_autoptr(virCommand) compressor = NULL; virDomainObj *vm; - g_autofree char *name = NULL; int ret = -1; virCheckFlags(VIR_DOMAIN_SAVE_BYPASS_CACHE | @@ -2889,29 +2925,7 @@ qemuDomainManagedSave(virDomainPtr dom, unsigned int flags) if (virDomainManagedSaveEnsureACL(dom->conn, vm->def) < 0) goto cleanup; - if (virDomainObjCheckActive(vm) < 0) - goto cleanup; - - if (!vm->persistent) { - virReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot do managed save for transient domain")); - goto cleanup; - } - - cfg = virQEMUDriverGetConfig(driver); - if ((compressed = qemuSaveImageGetCompressionProgram(cfg->saveImageFormat, - &compressor, - "save", false)) < 0) - goto cleanup; - - name = qemuDomainManagedSavePath(driver, vm); - - VIR_INFO("Saving state of domain '%s' to '%s'", vm->def->name, name); - - ret = qemuDomainSaveInternal(driver, vm, name, compressed, - compressor, NULL, flags); - if (ret == 0) - vm->hasManagedSave = true; + ret = qemuDomainManagedSaveHelper(driver, vm, flags); cleanup: virDomainObjEndAPI(&vm); -- 2.35.1
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@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); + } + + 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); -- 2.35.1
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. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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
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@redhat.com> --- 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 :|
On 5/12/22 17:22, Daniel P. Berrangé wrote:
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.
Fair enough. But so far I don't think there's anything that virDomainRestoreParams() offers that can't be achieved with already existing APIs. So let me drop this patch and post another one that merely requires path. And if we ever want this to work as virDomainCreateParams we can introduce new typed param (to capture that dom UUID) and just revert my patch. Michal
Hello Michal, this seems to be going backwards to special case arguments instead of putting them into typed parameters. I do not understand where this need comes from, but it does not seem a good direction to me. Thanks, Claudio On 5/12/22 5:17 PM, 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.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- 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");
On 5/12/22 19:09, Claudio Fontana wrote:
Hello Michal,
this seems to be going backwards to special case arguments instead of putting them into typed parameters. I do not understand where this need comes from, but it does not seem a good direction to me.
The need stems from my testing python bindings for these new APIs that I'm writing. When I call the restore API with no arguments, which is equivalent to calling plain virDomainRestoreParams(conn, NULL, 0, 0); then I see the following error: libvirt: QEMU Driver error : Failed to open file '(null)': Bad address Obviously, this is wrong. And indeed, when I think about the API more then the path to restore from is critical, at least for now. But as I suggests in one of my previous replies, I'm going to make path required in typed params for now. If we ever come with an alternative source for restore then that check can be relaxed. Michal
On 5/13/22 9:28 AM, Michal Prívozník wrote:
On 5/12/22 19:09, Claudio Fontana wrote:
Hello Michal,
this seems to be going backwards to special case arguments instead of putting them into typed parameters. I do not understand where this need comes from, but it does not seem a good direction to me.
The need stems from my testing python bindings for these new APIs that I'm writing. When I call the restore API with no arguments, which is equivalent to calling plain virDomainRestoreParams(conn, NULL, 0, 0); then I see the following error:
libvirt: QEMU Driver error : Failed to open file '(null)': Bad address
Obviously, this is wrong. And indeed, when I think about the API more then the path to restore from is critical, at least for now. But as I suggests in one of my previous replies, I'm going to make path required in typed params for now. If we ever come with an alternative source for restore then that check can be relaxed.
Michal
Thanks for the explanation, indeed the FILE parameter is not an optional parameter, but a mandatory one. I assumed mandatory/optional parameters was already handled by the libvirt machinery, but clearly it isn't. Thanks, Claudio
participants (4)
-
Claudio Fontana -
Daniel P. Berrangé -
Michal Privoznik -
Michal Prívozník