Re: [PATCH V4 01/18] lib: virDomain{Save,Restore}Params: Ensure absolute path

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(a)redhat.com&gt;