On Mon, 30 Aug 2010, Daniel De Graaf wrote:
> The return value of read_message needs to be checked in order to avoid
> waiting forever for a message if there is an error on the communication
> channel with xenstore. Currently, this is only checked if USE_PTHREAD is
> defined (by checking for read thread exit), and that path is prone to
> deadlock if request_mutex is held while waiting.
>
> Since the failure of read_message leaves the socket in an undefined
> state, close the socket and force all threads waiting on a read to return.
>
> This also fixes xs_read_watch in the case where a read thread is not
> running (in particular, this will happen if !USE_PTHREAD) by having it
> read from the communication channel in the same way as read_reply.
>
> Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
>
> diff -r 3c4c3d48a835 tools/xenstore/xs.c
> --- a/tools/xenstore/xs.c Thu Aug 26 11:16:56 2010 +0100
> +++ b/tools/xenstore/xs.c Mon Aug 30 10:56:11 2010 -0400
> @@ -84,7 +84,7 @@
> #define mutex_lock(m) pthread_mutex_lock(m)
> #define mutex_unlock(m) pthread_mutex_unlock(m)
> #define condvar_signal(c) pthread_cond_signal(c)
> -#define condvar_wait(c,m,hnd) pthread_cond_wait(c,m)
> +#define condvar_wait(c,m) pthread_cond_wait(c,m)
> #define cleanup_push(f, a) \
> pthread_cleanup_push((void (*)(void *))(f), (void *)(a))
> /*
> @@ -111,7 +111,7 @@
> #define mutex_lock(m) ((void)0)
> #define mutex_unlock(m) ((void)0)
> #define condvar_signal(c) ((void)0)
> -#define condvar_wait(c,m,hnd) read_message(hnd)
> +#define condvar_wait(c,m) ((void)0)
> #define cleanup_push(f, a) ((void)0)
> #define cleanup_pop(run) ((void)0)
> #define read_thread_exists(h) (0)
> @@ -337,21 +337,20 @@
>
> read_from_thread = read_thread_exists(h);
>
> -#ifdef USE_PTHREAD
> /* Read from comms channel ourselves if there is no reader thread. */
> if (!read_from_thread && (read_message(h) == -1))
> return NULL;
> -#endif
>
> mutex_lock(&h->reply_mutex);
> - while (list_empty(&h->reply_list) && (!read_from_thread ||
> read_thread_exists(h)))
> - condvar_wait(&h->reply_condvar, &h->reply_mutex, h);
> - if (read_from_thread && !read_thread_exists(h)) {
> +#ifdef USE_PTHREAD
> + while (list_empty(&h->reply_list) && read_from_thread && h->fd != -1)
> + condvar_wait(&h->reply_condvar, &h->reply_mutex);
> +#endif
> + if (list_empty(&h->reply_list)) {
> mutex_unlock(&h->reply_mutex);
> errno = EINVAL;
> return NULL;
> }
> - assert(!list_empty(&h->reply_list));
> msg = list_top(&h->reply_list, struct xs_stored_msg, list);
> list_del(&msg->list);
> assert(list_empty(&h->reply_list));
> @@ -663,13 +662,22 @@
> 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. */
> - while (list_empty(&h->watch_list) && read_thread_exists(h))
> - condvar_wait(&h->watch_condvar, &h->watch_mutex, h);
> - if (!read_thread_exists(h)) {
> +#ifdef USE_PTHREAD
> + while (list_empty(&h->watch_list) && read_from_thread && h->fd != -1)
> + condvar_wait(&h->watch_condvar, &h->watch_mutex);
> +#endif
> + if (list_empty(&h->watch_list)) {
> mutex_unlock(&h->watch_mutex);
> errno = EINVAL;
> return NULL;
> @@ -965,21 +973,27 @@
> static void *read_thread(void *arg)
> {
> struct xs_handle *h = arg;
> + int fd;
>
> while (read_message(h) != -1)
> continue;
>
> - /* Kick anyone waiting for a reply */
> - pthread_mutex_lock(&h->request_mutex);
> - h->read_thr_exists = 0;
> - pthread_mutex_unlock(&h->request_mutex);
> + /* An error return from read_message leaves the socket in an undefined
> + * state; we might have read only the header and not the message after
> + * it, or (more commonly) the other end has closed the connection.
> + * Since further communication is unsafe, close the socket.
> + */
> + fd = h->fd;
> + h->fd = -1;
> + close(fd);
>
I like this patch, however shouldn't you be setting h->read_thr_exists
to 0 here?
If you don't set read_thr_exists to 0 the variable becomes meaningless
and you can go ahead and remove it altogether.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home