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