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

Cole Robinson posted 1 patch 2 weeks ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/a7812cc0b5cf4ea67eee720a020872f55d383c4d.1602459876.git.crobinso@redhat.com
src/qemu/qemu_migration.c | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)

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

Posted by Cole Robinson 2 weeks ago
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@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;
 
     VIR_DEBUG("Precreate disk type=%s", virStorageTypeToString(disk->src->type));
 
@@ -204,6 +204,9 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
         *volName = '\0';
         volName++;
 
+        if (!(conn = virGetConnectStorage()))
+            goto cleanup;
+
         if (!(pool = virStoragePoolLookupByTargetPath(conn, basePath)))
             goto cleanup;
         format = virStorageFileFormatTypeToString(disk->src->format);
@@ -212,6 +215,9 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
         break;
 
     case VIR_STORAGE_TYPE_VOLUME:
+        if (!(conn = virGetConnectStorage()))
+            goto cleanup;
+
         if (!(pool = virStoragePoolLookupByName(conn, disk->src->srcpool->pool)))
             goto cleanup;
         format = virStorageFileFormatTypeToString(disk->src->format);
@@ -270,6 +276,7 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
     VIR_FREE(volStr);
     virObjectUnref(vol);
     virObjectUnref(pool);
+    virObjectUnref(conn);
     return ret;
 }
 
@@ -304,13 +311,10 @@ qemuMigrationDstPrecreateStorage(virDomainObjPtr vm,
 {
     int ret = -1;
     size_t i = 0;
-    virConnectPtr conn;
 
     if (!nbd || !nbd->ndisks)
         return 0;
 
-    if (!(conn = virGetConnectStorage()))
-        return -1;
 
     for (i = 0; i < nbd->ndisks; i++) {
         virDomainDiskDefPtr disk;
@@ -349,13 +353,12 @@ qemuMigrationDstPrecreateStorage(virDomainObjPtr vm,
 
         VIR_DEBUG("Proceeding with disk source %s", NULLSTR(diskSrcPath));
 
-        if (qemuMigrationDstPrecreateDisk(conn, disk, nbd->disks[i].capacity) < 0)
+        if (qemuMigrationDstPrecreateDisk(disk, nbd->disks[i].capacity) < 0)
             goto cleanup;
     }
 
     ret = 0;
  cleanup:
-    virObjectUnref(conn);
     return ret;
 }
 
-- 
2.28.0

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

Posted by Daniel P. Berrangé 2 weeks ago
On Sun, Oct 11, 2020 at 07:44:36PM -0400, 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.

This pushes the connection open attempts inside a loop body. So if the
VM has multiple disks, then we're going to be repeatedly opening and
closing the connection which is not desirable.

I think we need a global connection across all disks, which is lazy
opened on first access.

> 
> Signed-off-by: Cole Robinson <crobinso@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;
>  
>      VIR_DEBUG("Precreate disk type=%s", virStorageTypeToString(disk->src->type));
>  
> @@ -204,6 +204,9 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
>          *volName = '\0';
>          volName++;
>  
> +        if (!(conn = virGetConnectStorage()))
> +            goto cleanup;
> +
>          if (!(pool = virStoragePoolLookupByTargetPath(conn, basePath)))
>              goto cleanup;
>          format = virStorageFileFormatTypeToString(disk->src->format);
> @@ -212,6 +215,9 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
>          break;
>  
>      case VIR_STORAGE_TYPE_VOLUME:
> +        if (!(conn = virGetConnectStorage()))
> +            goto cleanup;
> +
>          if (!(pool = virStoragePoolLookupByName(conn, disk->src->srcpool->pool)))
>              goto cleanup;
>          format = virStorageFileFormatTypeToString(disk->src->format);
> @@ -270,6 +276,7 @@ qemuMigrationDstPrecreateDisk(virConnectPtr conn,
>      VIR_FREE(volStr);
>      virObjectUnref(vol);
>      virObjectUnref(pool);
> +    virObjectUnref(conn);
>      return ret;
>  }
>  
> @@ -304,13 +311,10 @@ qemuMigrationDstPrecreateStorage(virDomainObjPtr vm,
>  {
>      int ret = -1;
>      size_t i = 0;
> -    virConnectPtr conn;
>  
>      if (!nbd || !nbd->ndisks)
>          return 0;
>  
> -    if (!(conn = virGetConnectStorage()))
> -        return -1;
>  
>      for (i = 0; i < nbd->ndisks; i++) {
>          virDomainDiskDefPtr disk;
> @@ -349,13 +353,12 @@ qemuMigrationDstPrecreateStorage(virDomainObjPtr vm,
>  
>          VIR_DEBUG("Proceeding with disk source %s", NULLSTR(diskSrcPath));
>  
> -        if (qemuMigrationDstPrecreateDisk(conn, disk, nbd->disks[i].capacity) < 0)
> +        if (qemuMigrationDstPrecreateDisk(disk, nbd->disks[i].capacity) < 0)
>              goto cleanup;
>      }
>  
>      ret = 0;
>   cleanup:
> -    virObjectUnref(conn);
>      return ret;
>  }

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

Posted by Michal Privoznik 2 weeks ago
On 10/12/20 10:50 AM, Daniel P. Berrangé wrote:
> On Sun, Oct 11, 2020 at 07:44:36PM -0400, 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.
> 
> This pushes the connection open attempts inside a loop body. So if the
> VM has multiple disks, then we're going to be repeatedly opening and
> closing the connection which is not desirable.
> 
> I think we need a global connection across all disks, which is lazy
> opened on first access.

Doesn't virGetConnectStorage() cache the connection?

Michal

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

Posted by Daniel P. Berrangé 2 weeks ago
On Mon, Oct 12, 2020 at 10:55:52AM +0200, Michal Privoznik wrote:
> On 10/12/20 10:50 AM, Daniel P. Berrangé wrote:
> > On Sun, Oct 11, 2020 at 07:44:36PM -0400, 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.
> > 
> > This pushes the connection open attempts inside a loop body. So if the
> > VM has multiple disks, then we're going to be repeatedly opening and
> > closing the connection which is not desirable.
> > 
> > I think we need a global connection across all disks, which is lazy
> > opened on first access.
> 
> Doesn't virGetConnectStorage() cache the connection?

Yes & no.

You can call virSetConnectStorage(conn) to populate the cache with a
pre-opened connection. If you haven't done that, then a new connection
will be opened every time. The ltter is what this code will be doing.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

Posted by Cole Robinson 2 weeks ago
On 10/12/20 5:05 AM, Daniel P. Berrangé wrote:
> On Mon, Oct 12, 2020 at 10:55:52AM +0200, Michal Privoznik wrote:
>> On 10/12/20 10:50 AM, Daniel P. Berrangé wrote:
>>> On Sun, Oct 11, 2020 at 07:44:36PM -0400, 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.
>>>
>>> This pushes the connection open attempts inside a loop body. So if the
>>> VM has multiple disks, then we're going to be repeatedly opening and
>>> closing the connection which is not desirable.
>>>
>>> I think we need a global connection across all disks, which is lazy
>>> opened on first access.
>>
>> Doesn't virGetConnectStorage() cache the connection?
> 
> Yes & no.
> 
> You can call virSetConnectStorage(conn) to populate the cache with a
> pre-opened connection. If you haven't done that, then a new connection
> will be opened every time. The ltter is what this code will be doing.
> 

What is the use case for not caching the connection by default?

virSetConnect* has very few users, but virGetConnect* is sprinkled in
many places, including multiple uses in generic code in domain_conf.c.
Can this be done as part of virGetConnect*?

- Cole

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

Posted by Daniel P. Berrangé 2 weeks ago
On Mon, Oct 12, 2020 at 09:12:50AM -0400, Cole Robinson wrote:
> On 10/12/20 5:05 AM, Daniel P. Berrangé wrote:
> > On Mon, Oct 12, 2020 at 10:55:52AM +0200, Michal Privoznik wrote:
> >> On 10/12/20 10:50 AM, Daniel P. Berrangé wrote:
> >>> On Sun, Oct 11, 2020 at 07:44:36PM -0400, 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.
> >>>
> >>> This pushes the connection open attempts inside a loop body. So if the
> >>> VM has multiple disks, then we're going to be repeatedly opening and
> >>> closing the connection which is not desirable.
> >>>
> >>> I think we need a global connection across all disks, which is lazy
> >>> opened on first access.
> >>
> >> Doesn't virGetConnectStorage() cache the connection?
> > 
> > Yes & no.
> > 
> > You can call virSetConnectStorage(conn) to populate the cache with a
> > pre-opened connection. If you haven't done that, then a new connection
> > will be opened every time. The ltter is what this code will be doing.
> > 
> 
> What is the use case for not caching the connection by default?

It is a matter of figuring out an invalidation strategy, so we don't
keep the other daemons running continuously when no client is actually
triggering their usage.

> virSetConnect* has very few users, but virGetConnect* is sprinkled in
> many places, including multiple uses in generic code in domain_conf.c.
> Can this be done as part of virGetConnect*?


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

Posted by Michal Privoznik 2 weeks ago
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@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.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

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

Posted by Peter Krempa 2 weeks ago
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@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.

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

Posted by Michal Privoznik 2 weeks ago
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@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

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

Posted by Peter Krempa 2 weeks ago
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@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).