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@xxxxxxxx>
> ---
>
> 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.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list