Re: [Qemu-devel] [patch 1/7] qemu: mutex/thread/cond wrappers

[Avi Kivity]

address@hidden wrote:
> --- qemu.orig/vl.c
> +++ qemu/vl.c
> @@ -36,6 +36,7 @@
>  #include "gdbstub.h"
>  #include "qemu-timer.h"
>  #include "qemu-char.h"
> +#include "qemu-thread.h"
>  #include "cache-utils.h"
>  #include "block.h"
>  #include "audio/audio.h"
> @@ -263,6 +264,8 @@ static QEMUTimer *nographic_timer;
>  
>  uint8_t qemu_uuid[16];
>  
> +QemuMutex qemu_global_mutex;
> +
>  /***********************************************************/
>  /* x86 ISA bus support */
>  
> @@ -3650,7 +3653,14 @@ void main_loop_wait(int timeout)
>          slirp_select_fill(&nfds, &rfds, &wfds, &xfds);
>      }
>  #endif
> +
> +    /*
> +     * main_loop_wait() *must* not assume any global state is consistent across
> +     * select() invocations.
> +     */
> +    qemu_mutex_unlock(&qemu_global_mutex);
>      ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
> +    qemu_mutex_lock(&qemu_global_mutex);
>      if (ret > 0) {
>          IOHandlerRecord **pioh;
>  
> @@ -3708,6 +3718,9 @@ static int main_loop(void)
>  #endif
>      CPUState *env;
>  
> +    qemu_mutex_init(&qemu_global_mutex);
> +    qemu_mutex_lock(&qemu_global_mutex);
> +
>      cur_cpu = first_cpu;
>      next_cpu = cur_cpu->next_cpu ?: first_cpu;
>      for(;;) {
>   

All the bits above seem unnecessary for the this patch.


> +
> +int qemu_mutex_init(QemuMutex *mutex)
> +{
> +    return pthread_mutex_init(&mutex->lock, NULL);
> +}
>   

No one will check the return code, so better to check it here and 
abort() if it's bad.

> +static void add_to_timespec(struct timespec *ts, unsigned int msecs)
> +{
> +    ts->tv_sec = ts->tv_sec + (long)(msecs / 1000);
> +    ts->tv_nsec = (ts->tv_nsec + ((long)msecs % 1000) * 1000000);
> +    if (ts->tv_nsec >= 1000000000) {
> +        ts->tv_nsec -= 1000000000;
> +        ts->tv_sec++;
> +    }
> +}
>   

timespec_add_msec()?  and why milliseconds?  Also, maybe uint64_t is a 
better type.

> +
> +int qemu_mutex_timedlock(QemuMutex *mutex, unsigned int msecs)
> +{
> +    struct timespec ts;
> +
> +    clock_gettime(CLOCK_REALTIME, &ts);
> +    add_to_timespec(&ts, msecs);
> +
> +    return pthread_mutex_timedlock(&mutex->lock, &ts);
> +}
>   

I would have preferred a deadline instead of a timeout, but we'll see on 
the next patches.

> +
> +int qemu_thread_create(QemuThread *thread,
> +                       void *(*start_routine)(void*),
> +                       void *arg)
> +{
> +    return pthread_create(&thread->thread, NULL, start_routine, arg);
> +}
>   

Don't return an error that no one will check.

> +
> +int qemu_thread_signal(QemuThread *thread, int sig)
> +{
> +    if (thread->thread != 0)
> +        return pthread_kill(thread->thread, sig);
> +    return -1; /* XXX: ESCHR */
> +}
>   

Ditto.  If the thread dies, qemu dies.

> +int qemu_thread_equal(QemuThread *thread1, QemuThread *thread2)
> +{
> +   return (thread1->thread == thread2->thread);
> +}
>   

Can compare thing1 to thing2 instead of ->thread.  Not that it matters.


--
error compiling committee.c: too many arguments to function