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

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(a)redhat.com&gt; > > > --- > > > 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).

Isn't refcounting designed exactly for cases like these?