On Wed, Mar 05, 2025 at 03:48:10PM -0700, Jim Fehlig via Devel wrote:
> When invoking virDomainSaveParams with a relative path, the image is
> saved to the daemon's CWD. Similarly, when providing virDomainRestoreParams
> with a relative path, it attempts to restore from the daemon's CWD. In most
> configurations, the daemon's CWD is set to '/'. Ensure a relative path is
> converted to absolute before invoking the driver domain{Save,Restore}Params
> functions.
Are you aware of any common usage of these APIs with a relative path ?
Although we've not documented it, my general view is that all paths given
to libvirt are expected to be absolute, everywhere - whether API parameters
like these, or config file parmeters, or XML elements/attributes.
In the perfect world we would canonicalize all relative paths on the
client side, but doing that is such an enourmous & complex job it is
not likely to be practical. We'd be better off just documenting use
of relative paths as "out of scope" and / or "undefined behaviour"
>
> Signed-off-by: Jim Fehlig <jfehlig@xxxxxxxx>
> ---
> src/libvirt-domain.c | 89 ++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 73 insertions(+), 16 deletions(-)
>
> diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c
> index 18451ebdb9..0ee7c6f45b 100644
> --- a/src/libvirt-domain.c
> +++ b/src/libvirt-domain.c
> @@ -1023,6 +1023,11 @@ virDomainSaveParams(virDomainPtr domain,
> unsigned int flags)
> {
> virConnectPtr conn;
> + virTypedParameterPtr params_copy = NULL;
> + int nparams_copy = 0;
> + const char *to = NULL;
> + g_autofree char *absolute_to = NULL;
> + int ret = -1;
>
> VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%d, flags=0x%x",
> params, nparams, flags);
> @@ -1033,23 +1038,46 @@ virDomainSaveParams(virDomainPtr domain,
> virCheckDomainReturn(domain, -1);
> conn = domain->conn;
>
> - virCheckReadOnlyGoto(conn->flags, error);
> + virCheckReadOnlyGoto(conn->flags, done);
>
> VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SAVE_RUNNING,
> VIR_DOMAIN_SAVE_PAUSED,
> - error);
> + done);
> +
> + /* We must absolutize the file path as the save is done out of process */
> + virTypedParamsCopy(¶ms_copy, params, nparams);
> + nparams_copy = nparams;
> + if (virTypedParamsGetString(params_copy, nparams_copy,
> + VIR_DOMAIN_SAVE_PARAM_FILE, &to) < 0)
> + goto done;
> +
> + if (to) {
> + if (!(absolute_to = g_canonicalize_filename(to, NULL))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("could not build absolute output file path"));
> + goto done;
> + }
> +
> + if (virTypedParamsReplaceString(¶ms_copy, &nparams_copy,
> + VIR_DOMAIN_SAVE_PARAM_FILE,
> + absolute_to) < 0)
> + goto done;
> + }
>
> if (conn->driver->domainSaveParams) {
> - if (conn->driver->domainSaveParams(domain, params, nparams, flags) < 0)
> - goto error;
> - return 0;
> + if (conn->driver->domainSaveParams(domain, params_copy, nparams_copy, flags) < 0)
> + goto done;
> + ret = 0;
> + } else {
> + virReportUnsupportedError();
> }
>
> - virReportUnsupportedError();
> + done:
> + if (ret < 0)
> + virDispatchError(domain->conn);
> + virTypedParamsFree(params_copy, nparams_copy);
>
> - error:
> - virDispatchError(domain->conn);
> - return -1;
> + return ret;
> }
>
>
> @@ -1206,6 +1234,12 @@ virDomainRestoreParams(virConnectPtr conn,
> virTypedParameterPtr params, int nparams,
> unsigned int flags)
> {
> + virTypedParameterPtr params_copy = NULL;
> + int nparams_copy = 0;
> + const char *from = NULL;
> + g_autofree char *absolute_from = NULL;
> + int ret = -1;
> +
> VIR_DEBUG("conn=%p, params=%p, nparams=%d, flags=0x%x",
> conn, params, nparams, flags);
> VIR_TYPED_PARAMS_DEBUG(params, nparams);
> @@ -1213,23 +1247,46 @@ virDomainRestoreParams(virConnectPtr conn,
> virResetLastError();
>
> virCheckConnectReturn(conn, -1);
> - virCheckReadOnlyGoto(conn->flags, error);
> + virCheckReadOnlyGoto(conn->flags, done);
>
> VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_SAVE_RUNNING,
> VIR_DOMAIN_SAVE_PAUSED,
> - error);
> + done);
>
> if (conn->driver->domainRestoreParams) {
> + /* We must absolutize the file path as the save is done out of process */
> + virTypedParamsCopy(¶ms_copy, params, nparams);
> + nparams_copy = nparams;
> + if (virTypedParamsGetString(params_copy, nparams_copy,
> + VIR_DOMAIN_SAVE_PARAM_FILE, &from) < 0)
> + goto done;
> +
> + if (from) {
> + if (!(absolute_from = g_canonicalize_filename(from, NULL))) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("could not build absolute output file path"));
> + goto done;
> + }
> +
> + if (virTypedParamsReplaceString(¶ms_copy, &nparams_copy,
> + VIR_DOMAIN_SAVE_PARAM_FILE,
> + absolute_from) < 0)
> + goto done;
> + }
> +
> if (conn->driver->domainRestoreParams(conn, params, nparams, flags) < 0)
> - goto error;
> - return 0;
> + goto done;
> + ret = 0;
> }
>
> virReportUnsupportedError();
>
> - error:
> - virDispatchError(conn);
> - return -1;
> + done:
> + if (ret < 0)
> + virDispatchError(conn);
> + virTypedParamsFree(params_copy, nparams_copy);
> +
> + return ret;
> }
>
>
> --
> 2.43.0
>
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 :|
