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?
Regards, Jim
signature.asc
Description: PGP signature
