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

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