[Xen-devel] Re: [PATCH] xenstore: correctly handle errors from read_message

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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