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

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?

Regards, Jim