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 <mklet...@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 :|
