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

[Michal Privoznik]

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). Isn't refcounting designed exactly for cases like these?

Michal