On 27.08.2015 23:38, Jim Fehlig wrote:
> 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;
>
Yup, with this piece it looks good. ACK
Er, we are currently in a freeze, but after all: a) this is a bugfix, b)
this has been sitting on the list quite a while. So I'd say push it.
Michal
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list