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

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.

> virNetSocketRemoveIOCallback(socks[i]); > virNetSocketClose(socks[i]); > - virObjectUnref(socks[i]); > This will leak the socks[i] object, wouldn't it?

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.