On Fri, Apr 03, 2020 at 02:36:35PM +0200, Peter Krempa wrote: > 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. Alright, so you mean a further refactor of qemuDomainBlockCommitImpl() is needed. Thanks, I'll look into it. pvl
