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

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