WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

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

To: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH] xenstore: correctly handle errors from read_message
From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Date: Tue, 31 Aug 2010 10:56:10 +0100
Cc: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxx>, Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxxxxx>
Delivery-date: Tue, 31 Aug 2010 02:57:07 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <4C7BEBA5.8070305@xxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <4C7BC8BA.8030503@xxxxxxxxxxxxx> <alpine.DEB.2.00.1008301809160.18383@kaball-desktop> <4C7BEBA5.8070305@xxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Alpine 2.00 (DEB 1167 2008-08-23)
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

<Prev in Thread] Current Thread [Next in Thread>