[PATCH] qemu: migration: don't open storage driver too early

[Peter Krempa]

On Mon, Oct 12, 2020 at 10:10:49 +0200, Michal Privoznik wrote:
> On 10/12/20 10:01 AM, Peter Krempa wrote:
> > On Mon, Oct 12, 2020 at 09:39:33 +0200, Michal Privoznik wrote:
> > > On 10/12/20 1:44 AM, Cole Robinson wrote:
> > > > If storage migration is requested, and the destination storage does
> > > > not exist on the remote host, qemu's migration support will call
> > > > into the libvirt storage driver to precreate the destination storage.
> > > > 
> > > > The storage driver virConnectPtr is opened too early though, adding
> > > > an unnecessary dependency on the storage driver for several cases
> > > > that don't require it. This currently requires kubevirt to install
> > > > the storage driver even though they aren't actually using it.
> > > > 
> > > > Push the virGetConnectStorage calls to right before the cases they are
> > > > actually needed.
> > > > 
> > > > Signed-off-by: Cole Robinson <crobinso at redhat.com>
> > > > ---
> > > >    src/qemu/qemu_migration.c | 17 ++++++++++-------
> > > >    1 file changed, 10 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> > > > index 2000c86640..99a6b41483 100644
> > > > --- a/src/qemu/qemu_migration.c
> > > > +++ b/src/qemu/qemu_migration.c
> > > > @@ -169,8 +169,7 @@ qemuMigrationSrcRestoreDomainState(virQEMUDriverPtr driver, virDomainObjPtr vm)
> > > >    static int
> > > > -qemuMigrationDstPrecreateDisk(virConnectPtr conn,
> > > > -                              virDomainDiskDefPtr disk,
> > > > +qemuMigrationDstPrecreateDisk(virDomainDiskDefPtr disk,
> > > >                                  unsigned long long capacity)
> > > >    {
> > > >        int ret = -1;
> > > > @@ -181,6 +180,7 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
> > > >        g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
> > > >        const char *format = NULL;
> > > >        unsigned int flags = 0;
> > > > +    virConnectPtr conn = NULL;
> > > 
> > > Wee tiny nitpick. If you're touching this might switch it g_auto:
> > > 
> > > g_autoptr(virConnect) conn = NULL;
> > > 
> > > and drop the explicit unref.
> > 
> > In this specific case I'd stay with explicit cleanup. If you look at the
> > cleanup section we have a 'vol', 'pool' and 'conn' pointer. They are
> > interconnected. While libvirt internally does refcounting, cleaning them
> > up in random order might seem wrong.
> > 
> 
> Wouldn't the same 'random' order appear again when we switch to g_auto() for
> the function anyways? Also, how to order cleanup if we have two objects,
> both holding a reference to each other (e.g. virDomainObj and qemuMonitor).

I'm actually against senseless conversion to g_auto. There might be code
paths which require explict ordering of cleanups and those obviously
must not be converted.

Obviously the rules for that are almost impossible to codify, but boil
down to carefully considering each case specifically.

> Isn't refcounting designed exactly for cases like these?

Yes it is. As noted in this case it will work.

I'm okay with using g_auto* in this patch provided that the rest of the
function is refactored prior to using it so that we don't have a mixed
cleanup specifically in case of these interconnected objects (yes, I'm
okay with mixed cleanup in case the objects are not interconnected).