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.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
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.0On 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@suse.com>
> ---
> 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 :|
On 3/19/25 05:54, Daniel P. Berrangé wrote:
> 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 ?
No. I added this patch to the series after receiving feedback from testers that
included "Providing a relative path to the location of the saved VM does not
work". E.g. something like
# virsh restore --bypass-cache test/test-vm.sav
error: Failed to restore domain from test/test-vm.sav
error: Failed to open file 'test/test-vm.sav': No such file or directory"
> 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.
Agreed. Relative paths from (remote) clients are ambiguous on the server. I'm
fine dropping this patch from the series.
> 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"
I'll take a stab at improving the documentation.
Regards,
Jim
On Wed, Mar 19, 2025 at 05:43:20PM -0600, Jim Fehlig via Devel wrote:
>On 3/19/25 05:54, Daniel P. Berrangé wrote:
>> 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 ?
>
>No. I added this patch to the series after receiving feedback from testers that
>included "Providing a relative path to the location of the saved VM does not
>work". E.g. something like
>
># virsh restore --bypass-cache test/test-vm.sav
>error: Failed to restore domain from test/test-vm.sav
>error: Failed to open file 'test/test-vm.sav': No such file or directory"
>
>> 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.
>
>Agreed. Relative paths from (remote) clients are ambiguous on the server. I'm
>fine dropping this patch from the series.
>
>> 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"
>
>I'll take a stab at improving the documentation.
>
So this actually breaks an existing usage, albeit from a simple user
(e.g. myself). And because later patches switch from
virDomain{Save,Restore}Flags to virDomain{Save,Restore}Params, this
might be seen as a regression. If we do not want to canonicalize the
paths, then we should error out on relative ones. Without that the
current behaviour is not only what's described above (and how I noticed
it), but also the following two commands:
virsh save domain asdf.img
virsh save --image-format raw asdf.img
will behave differently. The first one saves the image in CWD of virsh,
the second one will save the same image in CWD of virtqemud (or
respective daemon), without any error or indication of that happening.
Consequently, `virsh restore` will restore from a different file based
on whether there are extra flags/parameters or not.
Either we need to disallow relative paths or canonicalize them on the
client, and ideally before the release.
>Regards,
>Jim
>
On Tue, Mar 25, 2025 at 10:49:49AM +0100, Martin Kletzander wrote:
> On Wed, Mar 19, 2025 at 05:43:20PM -0600, Jim Fehlig via Devel wrote:
> > On 3/19/25 05:54, Daniel P. Berrangé wrote:
> > > 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 ?
> >
> > No. I added this patch to the series after receiving feedback from testers that
> > included "Providing a relative path to the location of the saved VM does not
> > work". E.g. something like
> >
> > # virsh restore --bypass-cache test/test-vm.sav
> > error: Failed to restore domain from test/test-vm.sav
> > error: Failed to open file 'test/test-vm.sav': No such file or directory"
> >
> > > 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.
> >
> > Agreed. Relative paths from (remote) clients are ambiguous on the server. I'm
> > fine dropping this patch from the series.
> >
> > > 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"
> >
> > I'll take a stab at improving the documentation.
> >
>
> So this actually breaks an existing usage, albeit from a simple user
> (e.g. myself). And because later patches switch from
> virDomain{Save,Restore}Flags to virDomain{Save,Restore}Params, this
> might be seen as a regression. If we do not want to canonicalize the
> paths, then we should error out on relative ones. Without that the
> current behaviour is not only what's described above (and how I noticed
> it), but also the following two commands:
>
> virsh save domain asdf.img
> virsh save --image-format raw asdf.img
>
> will behave differently. The first one saves the image in CWD of virsh,
> the second one will save the same image in CWD of virtqemud (or
> respective daemon), without any error or indication of that happening.
> Consequently, `virsh restore` will restore from a different file based
> on whether there are extra flags/parameters or not.
Urgh, right, I missed that we already have canonicalization in
the virDomainSave method.
>
> Either we need to disallow relative paths or canonicalize them on the
> client, and ideally before the release.
Yeah, reluctantly, we need to preserve that behaviour in the newer
virDomainSaveParams, to give consistent behaviour.
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 Tue, Mar 25, 2025 at 10:15:12AM +0000, Daniel P. Berrangé wrote:
>On Tue, Mar 25, 2025 at 10:49:49AM +0100, Martin Kletzander wrote:
>> On Wed, Mar 19, 2025 at 05:43:20PM -0600, Jim Fehlig via Devel wrote:
>> > On 3/19/25 05:54, Daniel P. Berrangé wrote:
>> > > 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 ?
>> >
>> > No. I added this patch to the series after receiving feedback from testers that
>> > included "Providing a relative path to the location of the saved VM does not
>> > work". E.g. something like
>> >
>> > # virsh restore --bypass-cache test/test-vm.sav
>> > error: Failed to restore domain from test/test-vm.sav
>> > error: Failed to open file 'test/test-vm.sav': No such file or directory"
>> >
>> > > 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.
>> >
>> > Agreed. Relative paths from (remote) clients are ambiguous on the server. I'm
>> > fine dropping this patch from the series.
>> >
>> > > 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"
>> >
>> > I'll take a stab at improving the documentation.
>> >
>>
>> So this actually breaks an existing usage, albeit from a simple user
>> (e.g. myself). And because later patches switch from
>> virDomain{Save,Restore}Flags to virDomain{Save,Restore}Params, this
>> might be seen as a regression. If we do not want to canonicalize the
>> paths, then we should error out on relative ones. Without that the
>> current behaviour is not only what's described above (and how I noticed
>> it), but also the following two commands:
>>
>> virsh save domain asdf.img
>> virsh save --image-format raw asdf.img
>>
>> will behave differently. The first one saves the image in CWD of virsh,
>> the second one will save the same image in CWD of virtqemud (or
>> respective daemon), without any error or indication of that happening.
>> Consequently, `virsh restore` will restore from a different file based
>> on whether there are extra flags/parameters or not.
>
>Urgh, right, I missed that we already have canonicalization in
>the virDomainSave method.
>
>>
>> Either we need to disallow relative paths or canonicalize them on the
>> client, and ideally before the release.
>
>Yeah, reluctantly, we need to preserve that behaviour in the newer
>virDomainSaveParams, to give consistent behaviour.
>
Yes, that behaviour is a relic of the past that we now need to live
with. But that's the reason I'd give my
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
>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 3/25/25 06:49, Martin Kletzander wrote:
> On Tue, Mar 25, 2025 at 10:15:12AM +0000, Daniel P. Berrangé wrote:
>> On Tue, Mar 25, 2025 at 10:49:49AM +0100, Martin Kletzander wrote:
>>> On Wed, Mar 19, 2025 at 05:43:20PM -0600, Jim Fehlig via Devel wrote:
>>> > On 3/19/25 05:54, Daniel P. Berrangé wrote:
>>> > > 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 ?
>>> >
>>> > No. I added this patch to the series after receiving feedback from testers
>>> that
>>> > included "Providing a relative path to the location of the saved VM does not
>>> > work". E.g. something like
>>> >
>>> > # virsh restore --bypass-cache test/test-vm.sav
>>> > error: Failed to restore domain from test/test-vm.sav
>>> > error: Failed to open file 'test/test-vm.sav': No such file or directory"
>>> >
>>> > > 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.
>>> >
>>> > Agreed. Relative paths from (remote) clients are ambiguous on the server. I'm
>>> > fine dropping this patch from the series.
>>> >
>>> > > 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"
>>> >
>>> > I'll take a stab at improving the documentation.
>>> >
>>>
>>> So this actually breaks an existing usage, albeit from a simple user
>>> (e.g. myself). And because later patches switch from
>>> virDomain{Save,Restore}Flags to virDomain{Save,Restore}Params, this
>>> might be seen as a regression. If we do not want to canonicalize the
>>> paths, then we should error out on relative ones. Without that the
>>> current behaviour is not only what's described above (and how I noticed
>>> it), but also the following two commands:
>>>
>>> virsh save domain asdf.img
>>> virsh save --image-format raw asdf.img
>>>
>>> will behave differently. The first one saves the image in CWD of virsh,
>>> the second one will save the same image in CWD of virtqemud (or
>>> respective daemon), without any error or indication of that happening.
>>> Consequently, `virsh restore` will restore from a different file based
>>> on whether there are extra flags/parameters or not.
>>
>> Urgh, right, I missed that we already have canonicalization in
>> the virDomainSave method.
I forgot about that as well when deciding to drop this patch from the series.
>>
>>>
>>> Either we need to disallow relative paths or canonicalize them on the
>>> client, and ideally before the release.
>>
>> Yeah, reluctantly, we need to preserve that behaviour in the newer
>> virDomainSaveParams, to give consistent behaviour.
>>
>
> Yes, that behaviour is a relic of the past that we now need to live
> with. But that's the reason I'd give my
>
> Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
Thanks. Is there agreement I should push this before the release?
Regards,
Jim
On Tue, Mar 25, 2025 at 08:41:41AM -0600, Jim Fehlig wrote:
>On 3/25/25 06:49, Martin Kletzander wrote:
>> On Tue, Mar 25, 2025 at 10:15:12AM +0000, Daniel P. Berrangé wrote:
>>> On Tue, Mar 25, 2025 at 10:49:49AM +0100, Martin Kletzander wrote:
>>>> On Wed, Mar 19, 2025 at 05:43:20PM -0600, Jim Fehlig via Devel wrote:
>>>> > On 3/19/25 05:54, Daniel P. Berrangé wrote:
>>>> > > 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 ?
>>>> >
>>>> > No. I added this patch to the series after receiving feedback from testers
>>>> that
>>>> > included "Providing a relative path to the location of the saved VM does not
>>>> > work". E.g. something like
>>>> >
>>>> > # virsh restore --bypass-cache test/test-vm.sav
>>>> > error: Failed to restore domain from test/test-vm.sav
>>>> > error: Failed to open file 'test/test-vm.sav': No such file or directory"
>>>> >
>>>> > > 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.
>>>> >
>>>> > Agreed. Relative paths from (remote) clients are ambiguous on the server. I'm
>>>> > fine dropping this patch from the series.
>>>> >
>>>> > > 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"
>>>> >
>>>> > I'll take a stab at improving the documentation.
>>>> >
>>>>
>>>> So this actually breaks an existing usage, albeit from a simple user
>>>> (e.g. myself). And because later patches switch from
>>>> virDomain{Save,Restore}Flags to virDomain{Save,Restore}Params, this
>>>> might be seen as a regression. If we do not want to canonicalize the
>>>> paths, then we should error out on relative ones. Without that the
>>>> current behaviour is not only what's described above (and how I noticed
>>>> it), but also the following two commands:
>>>>
>>>> virsh save domain asdf.img
>>>> virsh save --image-format raw asdf.img
>>>>
>>>> will behave differently. The first one saves the image in CWD of virsh,
>>>> the second one will save the same image in CWD of virtqemud (or
>>>> respective daemon), without any error or indication of that happening.
>>>> Consequently, `virsh restore` will restore from a different file based
>>>> on whether there are extra flags/parameters or not.
>>>
>>> Urgh, right, I missed that we already have canonicalization in
>>> the virDomainSave method.
>
>I forgot about that as well when deciding to drop this patch from the series.
>
>>>
>>>>
>>>> Either we need to disallow relative paths or canonicalize them on the
>>>> client, and ideally before the release.
>>>
>>> Yeah, reluctantly, we need to preserve that behaviour in the newer
>>> virDomainSaveParams, to give consistent behaviour.
>>>
>>
>> Yes, that behaviour is a relic of the past that we now need to live
>> with. But that's the reason I'd give my
>>
>> Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
>
>Thanks. Is there agreement I should push this before the release?
>
From my side I'm for it; @Daniel?
>Regards,
>Jim
>
On Tue, Mar 25, 2025 at 04:33:03PM +0100, Martin Kletzander wrote:
> On Tue, Mar 25, 2025 at 08:41:41AM -0600, Jim Fehlig wrote:
> > On 3/25/25 06:49, Martin Kletzander wrote:
> > > On Tue, Mar 25, 2025 at 10:15:12AM +0000, Daniel P. Berrangé wrote:
> > > > On Tue, Mar 25, 2025 at 10:49:49AM +0100, Martin Kletzander wrote:
> > > > > On Wed, Mar 19, 2025 at 05:43:20PM -0600, Jim Fehlig via Devel wrote:
> > > > > > On 3/19/25 05:54, Daniel P. Berrangé wrote:
> > > > > > > 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 ?
> > > > > >
> > > > > > No. I added this patch to the series after receiving feedback from testers
> > > > > that
> > > > > > included "Providing a relative path to the location of the saved VM does not
> > > > > > work". E.g. something like
> > > > > >
> > > > > > # virsh restore --bypass-cache test/test-vm.sav
> > > > > > error: Failed to restore domain from test/test-vm.sav
> > > > > > error: Failed to open file 'test/test-vm.sav': No such file or directory"
> > > > > >
> > > > > > > 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.
> > > > > >
> > > > > > Agreed. Relative paths from (remote) clients are ambiguous on the server. I'm
> > > > > > fine dropping this patch from the series.
> > > > > >
> > > > > > > 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"
> > > > > >
> > > > > > I'll take a stab at improving the documentation.
> > > > > >
> > > > >
> > > > > So this actually breaks an existing usage, albeit from a simple user
> > > > > (e.g. myself). And because later patches switch from
> > > > > virDomain{Save,Restore}Flags to virDomain{Save,Restore}Params, this
> > > > > might be seen as a regression. If we do not want to canonicalize the
> > > > > paths, then we should error out on relative ones. Without that the
> > > > > current behaviour is not only what's described above (and how I noticed
> > > > > it), but also the following two commands:
> > > > >
> > > > > virsh save domain asdf.img
> > > > > virsh save --image-format raw asdf.img
> > > > >
> > > > > will behave differently. The first one saves the image in CWD of virsh,
> > > > > the second one will save the same image in CWD of virtqemud (or
> > > > > respective daemon), without any error or indication of that happening.
> > > > > Consequently, `virsh restore` will restore from a different file based
> > > > > on whether there are extra flags/parameters or not.
> > > >
> > > > Urgh, right, I missed that we already have canonicalization in
> > > > the virDomainSave method.
> >
> > I forgot about that as well when deciding to drop this patch from the series.
> >
> > > >
> > > > >
> > > > > Either we need to disallow relative paths or canonicalize them on the
> > > > > client, and ideally before the release.
> > > >
> > > > Yeah, reluctantly, we need to preserve that behaviour in the newer
> > > > virDomainSaveParams, to give consistent behaviour.
> > > >
> > >
> > > Yes, that behaviour is a relic of the past that we now need to live
> > > with. But that's the reason I'd give my
> > >
> > > Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
> >
> > Thanks. Is there agreement I should push this before the release?
> >
>
> From my side I'm for it; @Daniel?
Yes, go for it.
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 :|
© 2016 - 2026 Red Hat, Inc.