Re: [Qemu-devel] [RFC][PATCH v2 12/17] guest agent: worker thread class

[Michael Roth <mdroth@linux.vnet.ibm.com>]

On 04/21/2011 03:44 AM, Jes Sorensen wrote:
> On 04/18/11 17:02, Michael Roth wrote:
>> diff --git a/qga/guest-agent-worker.c b/qga/guest-agent-worker.c
>> new file mode 100644
>> index 0000000..e3295da
>> --- /dev/null
>> +++ b/qga/guest-agent-worker.c
>> @@ -0,0 +1,173 @@
>> +/*
>> + * QEMU Guest Agent worker thread interfaces
>> + *
>> + * Copyright IBM Corp. 2011
>> + *
>> + * Authors:
>> + *  Michael Roth<mdroth@linux.vnet.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + */
>> +#include<glib.h>
>> +#include<stdlib.h>
>> +#include<stdio.h>
>> +#include<stdbool.h>
>> +#include<pthread.h>
>> +#include<errno.h>
>> +#include<string.h>
>> +#include "guest-agent.h"
>> +#include "../error.h"
>
> Oh dear! do not do that please! Fix the Makefile to include the
> appropriate path.
>
>> +struct GAWorker {
>> +    pthread_t thread;
>> +    ga_worker_func execute;
>> +    pthread_mutex_t input_mutex;
>> +    pthread_cond_t input_avail_cond;
>> +    void *input;
>> +    bool input_avail;
>> +    pthread_mutex_t output_mutex;
>> +    pthread_cond_t output_avail_cond;
>
> You really should use QemuMutex and friends here.
>
>> +    void *output;
>> +    Error *output_error;
>> +    bool output_avail;
>> +};
>> +
>> +static void *worker_run(void *worker_p)
>> +{
>> +    GAWorker *worker = worker_p;
>> +    Error *err;
>> +    void *input, *output;
>> +
>> +    while (1) {
>> +        /* wait for input */
>> +        pthread_mutex_lock(&worker->input_mutex);
>
> qemu_mutex_lock()
>
>> +        while (!worker->input_avail) {
>> +            pthread_cond_wait(&worker->input_avail_cond,&worker->input_mutex);
>> +        }
>
> again
>
>> +        input = worker->input;
>> +        worker->input_avail = false;
>> +        pthread_mutex_unlock(&worker->input_mutex);
>
> and again.... I'll stop. Basically there really should be no references
> to pthread_*

This is on the guest side of things where I'm trying to use GLib 
wherever possible to keep things somewhat portable: logging/list 
utilities/io events/etc. And I *really* wanted to use GThreads here, but 
the problem is that GThread does not have any sane means to kill off a 
thread when a timeout occurs: there's no analogue to pthread_cancel(), 
and to use signals you need to break the abstraction to get the 
underlying pid. The new QemuThread stuff is using GThread underneath the 
covers so same limitation there.

pthreads provides these things and is fairly portable however, so I 
opted to make it an explicit dependency on the guest side. So 
glib+pthreads are the current dependencies.

>
> Jes