Re: [Qemu-devel] [PATCH 4/4] ccid: make threads joinable

[Alon Levy]

On Tue, Dec 06, 2011 at 06:05:55PM +0100, Paolo Bonzini wrote:
> Destroying a mutex that another thread might have just unlocked
> is racy.  It usually works, but you cannot do that in general and
> can lead to deadlocks or segfaults.  Change ccid to use joinable
> threads instead.
> 

Looks good to me.

Reviewed-by: Alon Levy <address@hidden>

> (Also, qemu_mutex_init/qemu_cond_init were missing).
> 
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  hw/ccid-card-emulated.c |   26 +++++++++++---------------
>  1 files changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/ccid-card-emulated.c b/hw/ccid-card-emulated.c
> index 9fe9db5..2d2ebce 100644
> --- a/hw/ccid-card-emulated.c
> +++ b/hw/ccid-card-emulated.c
> @@ -120,6 +120,7 @@ struct EmulatedState {
>      uint8_t  atr_length;
>      QSIMPLEQ_HEAD(event_list, EmulEvent) event_list;
>      QemuMutex event_list_mutex;
> +    QemuThread event_thread_id;
>      VReader *reader;
>      QSIMPLEQ_HEAD(guest_apdu_list, EmulEvent) guest_apdu_list;
>      QemuMutex vreader_mutex; /* and guest_apdu_list mutex */
> @@ -127,8 +128,7 @@ struct EmulatedState {
>      QemuCond handle_apdu_cond;
>      int      pipe[2];
>      int      quit_apdu_thread;
> -    QemuMutex apdu_thread_quit_mutex;
> -    QemuCond apdu_thread_quit_cond;
> +    QemuThread apdu_thread_id;
>  };
>  
>  static void emulated_apdu_from_guest(CCIDCardState *base,
> @@ -271,9 +271,6 @@ static void *handle_apdu_thread(void* arg)
>          }
>          qemu_mutex_unlock(&card->vreader_mutex);
>      }
> -    qemu_mutex_lock(&card->apdu_thread_quit_mutex);
> -    qemu_cond_signal(&card->apdu_thread_quit_cond);
> -    qemu_mutex_unlock(&card->apdu_thread_quit_mutex);
>      return NULL;
>  }
>  
> @@ -489,7 +486,6 @@ static uint32_t parse_enumeration(char *str,
>  static int emulated_initfn(CCIDCardState *base)
>  {
>      EmulatedState *card = DO_UPCAST(EmulatedState, base, base);
> -    QemuThread thread_id;
>      VCardEmulError ret;
>      EnumTable *ptable;
>  
> @@ -541,9 +537,10 @@ static int emulated_initfn(CCIDCardState *base)
>          printf("%s: failed to initialize vcard\n", EMULATED_DEV_NAME);
>          return -1;
>      }
> -    qemu_thread_create(&thread_id, event_thread, card, QEMU_THREAD_DETACHED);
> -    qemu_thread_create(&thread_id, handle_apdu_thread, card,
> -                       QEMU_THREAD_DETACHED);
> +    qemu_thread_create(&card->event_thread_id, event_thread, card,
> +                       QEMU_THREAD_JOINABLE);
> +    qemu_thread_create(&card->apdu_thread_id, handle_apdu_thread, card,
> +                       QEMU_THREAD_JOINABLE);
>      return 0;
>  }
>  
> @@ -553,15 +550,14 @@ static int emulated_exitfn(CCIDCardState *base)
>      VEvent *vevent = vevent_new(VEVENT_LAST, NULL, NULL);
>  
>      vevent_queue_vevent(vevent); /* stop vevent thread */
> -    qemu_mutex_lock(&card->apdu_thread_quit_mutex);
> +    qemu_thread_join(&card->event_thread_id);
> +
>      card->quit_apdu_thread = 1; /* stop handle_apdu thread */
>      qemu_cond_signal(&card->handle_apdu_cond);
> -    qemu_cond_wait(&card->apdu_thread_quit_cond,
> -                      &card->apdu_thread_quit_mutex);
> -    /* handle_apdu thread stopped, can destroy all of it's mutexes */
> +    qemu_thread_join(&card->apdu_thread_id);
> +
> +    /* threads exited, can destroy all condvars/mutexes */
>      qemu_cond_destroy(&card->handle_apdu_cond);
> -    qemu_cond_destroy(&card->apdu_thread_quit_cond);
> -    qemu_mutex_destroy(&card->apdu_thread_quit_mutex);
>      qemu_mutex_destroy(&card->handle_apdu_mutex);
>      qemu_mutex_destroy(&card->vreader_mutex);
>      qemu_mutex_destroy(&card->event_list_mutex);
> -- 
> 1.7.7.1
>