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

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(a)redhat.com&gt; > > Thanks. Is there agreement I should push this before the release? > From my side I'm for it; @Daniel?