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

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.