[libvirt] [PATCH 5/6] libxl: Add refcnt for args->conn during migration

[John Ferlan]

On 05/03/2018 08:34 AM, Erik Skultety wrote:
> On Tue, Apr 24, 2018 at 08:28:08AM -0400, John Ferlan wrote:
>> Since the @dconn reference via args->conn will be used via a thread
>> or callback, let's make sure memory associated with it isn't free'd
>> unexpectedly before we use it. The Unref will be done when the object
>> is Dispose'd.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/libxl/libxl_migration.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c
>> index 7fe352306c..d37a4a687a 100644
>> --- a/src/libxl/libxl_migration.c
>> +++ b/src/libxl/libxl_migration.c
>> @@ -239,6 +239,7 @@ libxlMigrationDstArgsDispose(void *obj)
>>
>>      libxlMigrationCookieFree(args->migcookie);
>>      VIR_FREE(args->socks);
>> +    virObjectUnref(args->conn);
>>      virObjectUnref(args->vm);
>>  }
>>
>> @@ -608,7 +609,7 @@ libxlDomainMigrationDstPrepareTunnel3(virConnectPtr dconn,
>>      if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
>>          goto error;
>>
>> -    args->conn = dconn;
>> +    args->conn = virObjectRef(dconn);
>>      args->vm = virObjectRef(vm);
>>      args->flags = flags;
>>      args->migcookie = mig;
>> @@ -763,7 +764,7 @@ libxlDomainMigrationDstPrepare(virConnectPtr dconn,
>>      if (!(args = virObjectNew(libxlMigrationDstArgsClass)))
>>          goto error;
>>
>> -    args->conn = dconn;
>> +    args->conn = virObjectRef(dconn);
>>      args->vm = virObjectRef(vm);
>>      args->flags = flags;
>>      args->socks = socks;
> 
> Do you actually have a use case, where the conn object would be destroyed
> before the migration finishes in a way that this could cause a SIGSEGV?
> 

Nope - just fear and protection any such possibility.  We're saving a
pointer to some object in an object and possibly dereferencing it at
some point in time in the future during libxlDoMigrateDstReceive.  Other
places where we do similar things w/ conn we also add a Ref to it.

And yes, patch 4 and 5 could be separate, perhaps more so this patch
than patch 4 because of how patch 6 changes @vm refcnt.

It shouldn't be possible to have a failure condition for either Ref, but
those are always "famous last words" and really difficult to diagnose
after the fact.  It doesn't hurt to Ref and then Unref AFAIK.

John