Re: [libvirt] PATCH: 17/25: Concurrent client dispatch in libvirtd

[Jim Meyering <jim meyering net>]

"Daniel P. Berrange" <berrange redhat com> wrote:
> On Tue, Jan 13, 2009 at 05:45:43PM +0000, Daniel P. Berrange wrote:
>> Historically libvirtd was single threaded, serializing all
>> requests across clients. An recent patch allowed multiple
>> threads, so multiple clients could run in parallel. A single
>> client was still serialized.
...

I haven't finished this one, but here's partial feedback:

> +void
> +qemudClientMessageQueuePush(struct qemud_client_message **queue,
> +                            struct qemud_client_message *msg)
> +{
> +    struct qemud_client_message *tmp = *queue;
> +
> +    if (tmp) {
> +        while (tmp->next)
> +            tmp = tmp->next;
> +        tmp->next = msg;
> +    } else {
> +        *queue = msg;
> +    }
> +}
> +
> +static struct qemud_client_message *
> +qemudClientMessageQueuePop(struct qemud_client_message **queue)
> +{
> +    struct qemud_client_message *tmp = *queue;
> +
> +    if (tmp)
> +        *queue = tmp->next;
> +    else
> +        *queue = NULL;

If tmp really can be NULL (tested for above),
then you can't dereference it below.

Also, since ...QueuePush appends,
I would have expected ...QueuePop to remove from the end.

> +    tmp->next = NULL;
> +    return tmp;
> +}
>
...
> @@ -1268,55 +1319,64 @@ static void *qemudWorker(void *data)
>          virMutexUnlock(&server->lock);
>
>          /* We own a locked client now... */
> -        client->mode = QEMUD_MODE_IN_DISPATCH;
>          client->refs++;
>
> -        if ((len = remoteDispatchClientRequest (server, client)) == 0)
> -            qemudDispatchClientFailure(server, client);
> +        /* Remove out message from dispatch queue while we use it */

s/out/our/

I'll finish tomorrow.