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

[Pavel Mores]

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