Re: [libvirt PATCH 3/3] qemu: block: external snapshot-delete implementation for straightforward cases

On Fri, Apr 03, 2020 at 14:34:29 +0200, Pavel Mores wrote: > On Tue, Mar 31, 2020 at 12:06:15PM +0200, Peter Krempa wrote: > > On Tue, Mar 31, 2020 at 11:18:29 +0200, Pavel Mores wrote: > > > > > + } > > > + > > > + if (flags & VIR_DOMAIN_SNAPSHOT_DELETE_CHILDREN) > > > + topPath = disk->src->path; > > > + else > > > + topPath = snapdisk->src->path; > > > > You must not use paths for doing this lookup. Paths at best work for > > local files only and this would make the code not future proof. > > > > Also you want to verify that: > > - images you want to merge are actually in the backing chain > > - the backing chain looks as snapshots describe it (e.g you unplug vda > > and plug a different chain back) > > > > Doing the validation above will necessarily give you a > > virStorageSourcePtr for the appropriate member of the backing chain and > > that one should be used as argument for the commit operation. > > I'm afraid I'm not following this. qemuDomainBlockCommitImpl(), just like > qemuDomainBlockCommit() take paths so that's what I'm passing them. What do > you mean, I should use virStorageSourcePtr as argument for the commit > operation? I'm saying you should not use paths at all. Use virStorageSource directly. If you look inside qemuDomainBlockCommitImpl there are calls which take path and look up the virStorageSource. Specifically e.g. for NBD disks path may be NULL, thus must not be used if your code has to work for everything. It's okay only to use them when passed by user, but not internally.