On Mon, 30 Aug 2010, Daniel De Graaf wrote:
> On 08/30/2010 01:13 PM, Stefano Stabellini wrote:
> > 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.
>
> The variable exists as a way to detect if the thread has been started so
> that xs_daemon_close can cancel/join if needed; it starts at 0 and is
> set to 1 on thread creation. Since pthread_t is supposed to be opaque,
> we can't test it directly to see if the thread has been created. Setting
> it to zero within the thread will leave the thread hanging because it
> will never be joined and has not been detached.
I understand now, thanks for the explanation.
I reckon the patch is correct and I am going to apply it unless somebody
has any other comments.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home