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

[Erik Skultety]

On Thu, May 03, 2018 at 09:02:35AM -0400, John Ferlan wrote:
>
>
> 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.

Doesn't have any effect on 4, whether you apply 4-5 before 6 or after,
it's an independent fix, but in the end it doesn't really matter, since you've
got my RB for both, so separating them now is irrelevant :).

>
> 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.

It doesn't, I was just curious, because I didn't see anything similar in qemu
migration code, but I wasn't really thorough, so it could have been done prior
to entering the migration code in qemu_driver.c, I'm not going to dig deeper,
it's a simple straightforward Ref/Unref change.

Reviewed-by: Erik Skultety <eskultet at redhat.com>

PS: I still think you could move 4,5 after 6, since to me the causality of
changes is more evident.