Re: [Xen-devel] [PATCH] libxenstore: fix threading bug which cause xend startup hang

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

Nice catch!

On Fri, 2010-09-10 at 17:54 +0100, Ian Jackson wrote: 
> @@ -662,21 +676,27 @@ char **xs_read_watch(struct xs_handle *h
>       struct xs_stored_msg *msg;
>       char **ret, *strings, c = 0;
>       unsigned int num_strings, i;
> -     int read_from_thread;
> -
> -     read_from_thread = read_thread_exists(h);
> -
> -     /* Read from comms channel ourselves if there is no reader thread. */
> -     if (!read_from_thread && (read_message(h) == -1))
> -             return NULL;
>  
>       mutex_lock(&h->watch_mutex);
>  
> -     /* Wait on the condition variable for a watch to fire. */
>  #ifdef USE_PTHREAD
> -     while (list_empty(&h->watch_list) && read_from_thread && h->fd != -1)
> +     /* Wait on the condition variable for a watch to fire.
> +      * If the reader thread doesn't exist yet, then that's because
> +      * we haven't called xs_watch.  Presumably the application
> +      * will do so later; in the meantime we just block.
> +      */
> +     while (list_empty(&h->watch_list) && h->fd != -1)
>               condvar_wait(&h->watch_condvar, &h->watch_mutex);
> -#endif
> +#else /* !defined(USE_PTHREAD) */
> +     /* Read from comms channel ourselves if there are no threads
> +      * and therefore no reader thread. */
> +
> +     assert(!read_thread_exists(h)); /* not threadsafe but worth a check */
> +     if ((read_message(h) == -1))
> +             return NULL;
> +
> +#endif /* !defined(USE_PTHREAD) */
> +
>       if (list_empty(&h->watch_list)) {
>               mutex_unlock(&h->watch_mutex);
>               errno = EINVAL;

read_reply contains a very similar pattern to the above. I assume it is
safe on that occasion because xs_talkv (the only caller of read_reply)
holds request_mutex?

If so then:
Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

> @@ -900,6 +920,10 @@ char *xs_debug_command(struct xs_handle 
>  
>  static int read_message(struct xs_handle *h)
>  {
> +     /* IMPORTANT: It is forbidden to call this function without
> +      * acquiring the request lock and checking that h->read_thr_exists
> +      * is false.  See "Lock discipline" in struct xs_handle, above. */
> +         

Took me ages to figure realise this didn't apply ifndef USE_PTHREAD,
which is pretty obvious. Time to call it a day I think.

Ian.



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel