Re: [libvirt] [PATCH 5/6] libxl: Add refcnt for args->conn during migration
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 > >> --- > >> 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 PS: I still think you could move 4,5 after 6, since to me the causality of changes is more evident. -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 >> --- >> 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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] libxl: Add refcnt for args->conn during migration
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 Also, yeah, as you figured and noted in the cover letter, these two libxl patches would probably be better off in a separate series IMO. Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] libxl: Add refcnt for args->conn during migration
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 > --- > 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? Erik -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH 5/6] libxl: Add refcnt for args->conn during migration
On Tue, Apr 24, 2018 at 02:28 PM +0200, 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 > --- > 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; > -- > 2.13.6 > > -- > libvir-list mailing list > libvir-list@redhat.com > https://www.redhat.com/mailman/listinfo/libvir-list > Reviewed-by: Marc Hartmayer -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
