Re: [PATCH 1/6] Move notif_queue to remote_state

[Pedro Alves <palves@redhat.com>]

On 10/02/2013 02:52 AM, Yao Qi wrote:
> On 10/01/2013 01:08 AM, Pedro Alves wrote:
>>> -static void
>>>> -notif_xfree (struct notif_client *notif)
>> ...
>>>> -  if (notif->pending_event != NULL
>>>> -      && notif->pending_event->dtr != NULL)
>>>> -    notif->pending_event->dtr (notif->pending_event);
>> Hmm, it's not clear to me from reading the patch where is
>> pending_event released after this patch.  Could you help
>> me understand that?
>
> Since notif_queue is a global variable, we've never used QUEUE_free to
> free notif_queue, so notif_xfree is never called, and pending_event on
> each notif_client is not released.
>
> When we move notif_queue to 'remote_notif_state', we have to explicitly
> use QUEUE_free to free notif_queue and elements in it.  We put the
> pointer of each notif_client to notif_queue, so we don't need a function
> to free the notif_client when notif_queue is destroyed.  So the
> notif_queue is allocated like this,
>
> notif_state->notif_queue = QUEUE_alloc (notif_client_p, NULL);
>
> and notif_xfree is not needed any more.  The leak on pending_event is
> still there, but it is fixed by the next patch, which moves
> pending_event to remote_notif_state, and pending_event is freed in
> remote_notif_state_xfree.

I see now, thanks.

BTW:

+struct remote_notif_state
+{
+  /* Notification queue.  */
+  QUEUE(notif_client_p) *notif_queue;
+
+  /* Asynchronous signal handle registered as event loop source for when
+     the remote sent us a notification.  The registered callback
+     will do a ACK sequence to pull the rest of the events out of
+     the remote side into our event queue.  */
+
+  struct async_event_handler *remote_async_get_pending_events_token;

Since remote_async_get_pending_events_token is no longer a global,
it doesn't need the "remote_" prefix (or even remote_async_?) anymore.
WDYT?

The patch is OK with or without that change.

Thanks,
-- 
Pedro Alves