Re: [libvirt] [PATCH V2 1/3] libxl: fix ref counting of libxlMigrationDstArgs

Michal Privoznik wrote: > On 07.08.2015 19:53, Jim Fehlig wrote: > >> This patch fixes some flawed logic around ref counting the >> libxlMigrationDstArgs object. >> >> First, when adding sockets to the event loop with >> virNetSocketAddIOCallback(), the generic virObjectFreeCallback() >> was registered as a free function, with libxlMigrationDstArgs as >> its parameter. A reference was also taken on >> libxlMigrationDstArgs for each successful call to >> virNetSocketAddIOCallback(). The rational behind this logic was >> that the libxlMigrationDstArgs object had to out-live the socket >> objects. But virNetSocketAddIOCallback() already takes a >> reference on socket objects, ensuring their life until removed >> from the event loop and unref'ed in virNetSocketEventFree(). We >> only need to ensure libxlMigrationDstArgs lives until >> libxlDoMigrateReceive() finishes, which can be done by simply >> unref'ing libxlMigrationDstArgs at the end of >> libxlDoMigrateReceive(). >> >> The second flaw was unref'ing the sockets in the failure path of >> libxlMigrateReceive() and at the end of libxlDoMigrateReceive(). >> As mentioned above, the sockets are already unref'ed by >> virNetSocketEventFree() when removed from the event loop. >> Attempting to unref the socket a second time resulted in a >> libvirtd crash since the socket was previously unref'ed and >> disposed. >> >> Signed-off-by: Jim Fehlig <jfehlig(a)suse.com&gt; >> --- >> >> V2: Initialize args in libxlDomainMigrationPrepare >> >> src/libxl/libxl_migration.c | 20 ++++++-------------- >> 1 file changed, 6 insertions(+), 14 deletions(-) >> >> diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c >> index aa9547b..f9673c8 100644 >> --- a/src/libxl/libxl_migration.c >> +++ b/src/libxl/libxl_migration.c >> @@ -112,11 +112,11 @@ libxlDoMigrateReceive(void *opaque) >> virNetSocketUpdateIOCallback(socks[i], 0); >> > > This is pre-existing, but since the socket callback is removed right > after, it does not make much sense to update its events to listen for. > Right. Should be removed since I'm touching this code. > >> virNetSocketRemoveIOCallback(socks[i]); >> virNetSocketClose(socks[i]); >> - virObjectUnref(socks[i]); >> > > This will leak the socks[i] object, wouldn't it? Yes, you are right. I verified the virNetSocket was not disposed with the missing unref. > I mean, in > libxlDomainMigrationPrepare() on line 392 virNetSocketNewListenTCP() is > called. This initialize the array with object pointers. Then > virNetSocketAddIOCallback() + virNetSocketRemoveIOCallback() pair keep > the ref counter consistent. This makes me think you should not remove > this line. > > >> socks[i] = NULL; >> } >> args->nsocks = 0; >> VIR_FORCE_CLOSE(recvfd); >> + virObjectUnref(args); >> } >> >> >> @@ -164,11 +164,11 @@ libxlMigrateReceive(virNetSocketPtr sock, >> virNetSocketUpdateIOCallback(socks[i], 0); >> virNetSocketRemoveIOCallback(socks[i]); >> virNetSocketClose(socks[i]); >> - virObjectUnref(socks[i]); >> socks[i] = NULL; >> } >> args->nsocks = 0; >> VIR_FORCE_CLOSE(recvfd); >> + virObjectUnref(args); >> } >> >> static int >> @@ -318,7 +318,7 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, >> virNetSocketPtr *socks = NULL; >> size_t nsocks = 0; >> int nsocks_listen = 0; >> - libxlMigrationDstArgs *args; >> + libxlMigrationDstArgs *args = NULL; >> size_t i; >> int ret = -1; >> >> @@ -420,22 +420,12 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, >> VIR_EVENT_HANDLE_READABLE, >> libxlMigrateReceive, >> args, >> - virObjectFreeCallback) < 0) >> + NULL) < 0) >> continue; >> >> - /* >> - * Successfully added sock to event loop. Take a ref on args to >> - * ensure it is not freed until sock is removed from the event loop. >> - * Ref is dropped in virObjectFreeCallback after being removed >> - * from the event loop. >> - */ >> - virObjectRef(args); >> nsocks_listen++; >> } >> >> - /* Done with args in this function, drop reference */ >> - virObjectUnref(args); >> - >> if (!nsocks_listen) >> goto error; >> >> @@ -448,6 +438,8 @@ libxlDomainMigrationPrepare(virConnectPtr dconn, >> virObjectUnref(socks[i]); >> } >> VIR_FREE(socks); >> + virObjectUnref(args); >> + >> /* Remove virDomainObj from domain list */ >> if (vm) { >> virDomainObjListRemove(driver->domains, vm); >> >> > > Otherwise looking good. > How does it look with the following squashed in? diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index 8db3aea..9609e06 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -112,9 +112,9 @@ libxlDoMigrateReceive(void *opaque) /* Remove all listen socks from event handler, and close them. */ for (i = 0; i < nsocks; i++) { - virNetSocketUpdateIOCallback(socks[i], 0); virNetSocketRemoveIOCallback(socks[i]); virNetSocketClose(socks[i]); + virObjectUnref(socks[i]); socks[i] = NULL; } args->nsocks = 0;