[libvirt] [PATCH 1/6] Remove all linked list handling from remote client event loop

[Jiri Denemark]

On Fri, Nov 11, 2011 at 16:21:59 +0000, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange at redhat.com>
> 
> Directly messing around with the linked list is potentially
> dangerous. Introduce some helper APIs to deal with list
> manipulating the list
> 
> * src/rpc/virnetclient.c: Create linked list handlers
> ---
>  src/rpc/virnetclient.c |  207 +++++++++++++++++++++++++++++++++---------------
>  1 files changed, 144 insertions(+), 63 deletions(-)
> 
> diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
> index 4b7d4a9..463dc5a 100644
> --- a/src/rpc/virnetclient.c
> +++ b/src/rpc/virnetclient.c
> @@ -110,6 +110,97 @@ static void virNetClientIncomingEvent(virNetSocketPtr sock,
>                                        int events,
>                                        void *opaque);
>  
> +/* Append a call to the end of the list */
> +static void virNetClientCallQueue(virNetClientCallPtr *head,
> +                                  virNetClientCallPtr call)
> +{
> +    virNetClientCallPtr tmp = *head;
> +    while (tmp && tmp->next) {
> +        tmp = tmp->next;
> +    }
> +    if (tmp)
> +        tmp->next = call;
> +    else
> +        *head = call;
> +    call->next = NULL;
> +}
> +
> +#if 0
> +/* Obtain a call from the head of the list */
> +static virNetClientCallPtr virNetClientCallServe(virNetClientCallPtr *head)
> +{
> +    virNetClientCallPtr tmp = *head;
> +    if (tmp)
> +        *head = tmp->next;
> +    else
> +        *head = NULL;
> +    tmp->next = NULL;
> +    return tmp;
> +}
> +#endif
> +
> +/* Remove a call from anywhere in the list */
> +static void virNetClientCallRemove(virNetClientCallPtr *head,
> +                                   virNetClientCallPtr call)
> +{
> +    virNetClientCallPtr tmp = *head;
> +    virNetClientCallPtr prev = NULL;
> +    while (tmp) {
> +        if (tmp == call) {
> +            if (prev)
> +                prev->next = tmp->next;
> +            else
> +                *head = tmp->next;
> +            tmp->next = NULL;
> +            return;
> +        }
> +        prev = tmp;
> +        tmp = tmp->next;
> +    }
> +}
> +
> +/* Predicate returns true if matches */
> +typedef bool (*virNetClientCallPredicate)(virNetClientCallPtr call, void *opaque);
> +
> +/* Remove a list of calls from the list based on a predicate */
> +static void virNetClientCallRemovePredicate(virNetClientCallPtr *head,
> +                                            virNetClientCallPredicate pred,
> +                                            void *opaque)
> +{
> +    virNetClientCallPtr tmp = *head;
> +    virNetClientCallPtr prev = NULL;
> +    while (tmp) {
> +        virNetClientCallPtr next = tmp->next;
> +        tmp->next = NULL; /* Temp unlink */
> +        if (pred(tmp, opaque)) {
> +            if (prev)
> +                prev->next = next;
> +            else
> +                *head = next;
> +        } else {
> +            tmp->next = next; /* Reverse temp unlink */
> +            prev = tmp;
> +        }
> +        tmp = next;
> +    }
> +}
> +
> +/* Returns true if the predicate matches at least one call in the list */
> +static bool virNetClientCallMatchPredicate(virNetClientCallPtr head,
> +                                           virNetClientCallPredicate pred,
> +                                           void *opaque)
> +{
> +    virNetClientCallPtr tmp = head;
> +    while (tmp) {
> +        if (pred(tmp, opaque)) {
> +            return true;
> +        }
> +        tmp = tmp->next;
> +    }
> +    return false;
> +}
> +
> +
>  static void virNetClientEventFree(void *opaque)
>  {
>      virNetClientPtr client = opaque;
> @@ -896,6 +987,42 @@ virNetClientIOHandleInput(virNetClientPtr client)
>  }
>  
>  
> +static bool virNetClientIOEventLoopPollEvents(virNetClientCallPtr call,
> +                                              void *opaque)
> +{
> +    struct pollfd *fd = opaque;
> +
> +    if (call->mode == VIR_NET_CLIENT_MODE_WAIT_RX)
> +        fd->events |= POLLIN;
> +    if (call->mode == VIR_NET_CLIENT_MODE_WAIT_TX)
> +        fd->events |= POLLOUT;
> +
> +    return true;
> +}

This should return false otherwise we only set fd->events according to the
first call in the queue. We need to go through all calls.

> +
> +
> +static bool virNetClientIOEventLoopRemoveDone(virNetClientCallPtr call,
> +                                              void *opaque)
> +{
> +    virNetClientCallPtr thiscall = opaque;
> +
> +    if (call == thiscall)
> +        return false;
> +
> +    if (call->mode != VIR_NET_CLIENT_MODE_COMPLETE)
> +        return false;
> +
> +    /*
> +     * ...they won't actually wakeup until
> +     * we release our mutex a short while
> +     * later...
> +     */
> +    VIR_DEBUG("Waking up sleeping call %p", call);
> +    virCondSignal(&call->cond);
> +
> +    return true;
> +}
> +
>  /*
>   * Process all calls pending dispatch/receive until we
>   * get a reply to our own call. Then quit and pass the buck
> @@ -911,8 +1038,6 @@ static int virNetClientIOEventLoop(virNetClientPtr client,
>      fds[1].fd = client->wakeupReadFD;
>  
>      for (;;) {
> -        virNetClientCallPtr tmp = client->waitDispatch;
> -        virNetClientCallPtr prev;
>          char ignore;
>          sigset_t oldmask, blockedsigs;
>          int timeout = -1;
> @@ -928,14 +1053,11 @@ static int virNetClientIOEventLoop(virNetClientPtr client,
>          fds[1].events = fds[1].revents = 0;
>  
>          fds[1].events = POLLIN;
> -        while (tmp) {
> -            if (tmp->mode == VIR_NET_CLIENT_MODE_WAIT_RX)
> -                fds[0].events |= POLLIN;
> -            if (tmp->mode == VIR_NET_CLIENT_MODE_WAIT_TX)
> -                fds[0].events |= POLLOUT;
>  
> -            tmp = tmp->next;
> -        }
> +        /* Calculate poll events for calls */
> +        virNetClientCallMatchPredicate(client->waitDispatch,
> +                                       virNetClientIOEventLoopPollEvents,
> +                                       &fds[0]);
>  
>          /* We have to be prepared to receive stream data
>           * regardless of whether any of the calls waiting
...

And we should avoid lines longer than 80 character, which is mostly not a
problem of this patch but some of the other patches in this series don't
follow this rule (mainly in function prototypes).

ACK with the small issue fixed.

Jirka