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@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.
>
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;
Thanks for taking time to review this series!
Regards,
Jim
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list