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

Re: [Xen-devel] [PATCH] Fix xenconsole's "Could not read tty from store"

To: John Levon <levon@xxxxxxxxxxxxxxxxx>, Samuel Thibault <samuel.thibault@xxxxxxxxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] Fix xenconsole's "Could not read tty from store"
From: Keir Fraser <Keir.Fraser@xxxxxxxxxxxx>
Date: Wed, 19 Dec 2007 11:29:21 +0000
Delivery-date: Wed, 19 Dec 2007 03:30:16 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <20071218185211.GA22592@xxxxxxxxxxxxxxxxxxxxxxx>
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/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
Thread-index: AchCMmYrpHrmXK4lEdyUJwAX8io7RQ==
Thread-topic: [Xen-devel] [PATCH] Fix xenconsole's "Could not read tty from store"
User-agent: Microsoft-Entourage/11.3.6.070618
Any conclusion on this one, before I do a second release candidate for
3.2.0?

 -- Keir


On 18/12/07 18:52, "John Levon" <levon@xxxxxxxxxxxxxxxxx> wrote:

> On Tue, Dec 18, 2007 at 06:18:16PM +0000, Samuel Thibault wrote:
> 
>> To my understanding, from the server side tcsetattr should only be
>> performed on the master side (with effect on the slave side too).
> 
> Here's another try, what does this do on Linux? My test program works
> without needing the tcsetattr, as does xenconsoled
> 
> thanks
> john
> 
> 
> # HG changeset patch
> # User john.levon@xxxxxxx
> # Date 1198003657 28800
> # Node ID 6f03f4ec458d4780314c015da214795b7b1cf195
> # Parent  539cbabd97b5ff3d335de151636040bb2f4cd629
> Fix master/slave handling in xenconsoled
> 
> Fix a number of problems with the pty handling:
> 
> - make openpty() implementation work on Solaris
> - set raw on the slave fd, not the master, as the master doesn't
>   have a line discipline pushed on Solaris
> - make sure we don't leak the slave fd returned from openpty()
> - don't use the 'name' argument of openpty() as it's a security risk
> - note behaviour of a zero read of the master on Solaris
> - remove pointless tcget/setattr
> 
> Signed-off-by: John Levon <john.levon@xxxxxxx>
> Signed-off-by: Samuel Thibault <samuel.thibault@xxxxxxxxxx>
> 
> diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
> --- a/tools/console/daemon/io.c
> +++ b/tools/console/daemon/io.c
> @@ -36,10 +36,14 @@
>  #include <stdarg.h>
>  #include <sys/mman.h>
>  #include <sys/time.h>
> +#include <assert.h>
>  #if defined(__NetBSD__) || defined(__OpenBSD__)
>  #include <util.h>
>  #elif defined(__linux__) || defined(__Linux__)
>  #include <pty.h>
> +#endif
> +#if defined(__sun__)
> +#include <stropts.h>
>  #endif
>  
>  #define MAX(a, b) (((a) > (b)) ? (a) : (b))
> @@ -75,7 +79,8 @@ struct domain
>  struct domain
>  {
> int domid;
> - int tty_fd;
> + int master_fd;
> + int slave_fd;
> int log_fd;
> bool is_dead;
> struct buffer buffer;
> @@ -227,77 +232,90 @@ static int create_domain_log(struct doma
> return fd;
>  }
>  
> +static void domain_close_tty(struct domain *dom)
> +{
> + if (dom->master_fd != -1) {
> +  close(dom->master_fd);
> +  dom->master_fd = -1;
> + }
> +
> + if (dom->slave_fd != -1) {
> +  close(dom->slave_fd);
> +  dom->slave_fd = -1;
> + }
> +}
> +
>  #ifdef __sun__
>  /* Once Solaris has openpty(), this is going to be removed. */
> -int openpty(int *amaster, int *aslave, char *name,
> -     struct termios *termp, struct winsize *winp)
> +static int openpty(int *amaster, int *aslave, char *name,
> +                   struct termios *termp, struct winsize *winp)
>  {
> - int mfd, sfd;
> + const char *slave;
> + int mfd = -1, sfd = -1;
>  
> *amaster = *aslave = -1;
> - mfd = sfd = -1;
>  
> mfd = open("/dev/ptmx", O_RDWR | O_NOCTTY);
> if (mfd < 0)
> -  goto err0;
> +  goto err;
>  
> if (grantpt(mfd) == -1 || unlockpt(mfd) == -1)
> -  goto err1;
> +  goto err;
>  
> - /* This does not match openpty specification,
> -  * but as long as this does not hurt, this is acceptable.
> -  */
> - mfd = sfd;
> + if ((slave = ptsname(mfd)) == NULL)
> +  goto err;
>  
> - if (termp != NULL && tcgetattr(sfd, termp) < 0)
> -  goto err1; 
> + if ((sfd = open(slave, O_RDONLY | O_NOCTTY)) == -1)
> +  goto err;
> +
> + if (ioctl(sfd, I_PUSH, "ptem") == -1)
> +  goto err;
>  
> if (amaster)
> *amaster = mfd;
> if (aslave)
> *aslave = sfd;
> - if (name)
> -  strlcpy(name, ptsname(mfd), sizeof(slave));
> if (winp)
> ioctl(sfd, TIOCSWINSZ, winp);
>  
> + assert(name == NULL);
> + assert(termp == NULL);
> +
> return 0;
>  
> -err1:
> +err:
> + if (sfd != -1)
> +  close(sfd);
> close(mfd);
> -err0:
> return -1;
>  }
>  #endif
>  
> -
>  static int domain_create_tty(struct domain *dom)
>  {
> - char slave[80];
> - struct termios term;
> + const char *slave;
> char *path;
> - int master, slavefd;
> int err;
> bool success;
> char *data;
> unsigned int len;
>  
> - if (openpty(&master, &slavefd, slave, &term, NULL) < 0) {
> -  master = -1;
> + assert(dom->slave_fd == -1);
> + assert(dom->master_fd == -1);
> +
> + if (openpty(&dom->master_fd, &dom->slave_fd, NULL, NULL, NULL) < 0) {
> err = errno;
> dolog(LOG_ERR, "Failed to create tty for domain-%d (errno = %i, %s)",
>      dom->domid, err, strerror(err));
> -  return master;
> +  return 0;
> }
>  
> - cfmakeraw(&term);
> - if (tcsetattr(master, TCSAFLUSH, &term) < 0) {
> + if ((slave = ptsname(dom->master_fd)) == NULL) {
> err = errno;
> -  dolog(LOG_ERR, "Failed to set tty attribute  for domain-%d (errno = %i,
> %s)",
> +  dolog(LOG_ERR, "Failed to get slave name for domain-%d (errno = %i, %s)",
>      dom->domid, err, strerror(err));
> goto out;
> }
> -
>  
> if (dom->use_consolepath) {
> success = asprintf(&path, "%s/limit", dom->conspath) !=
> @@ -340,15 +358,15 @@ static int domain_create_tty(struct doma
> goto out;
> }
>  
> - if (fcntl(master, F_SETFL, O_NONBLOCK) == -1)
> + if (fcntl(dom->master_fd, F_SETFL, O_NONBLOCK) == -1)
> goto out;
>  
> - return master;
> - out:
> - close(master);
> - return -1;
> + return 1;
> +out:
> + domain_close_tty(dom);
> + return 0;
>  }
> -
> + 
>  /* Takes tuples of names, scanf-style args, and void **, NULL terminated. */
>  int xs_gather(struct xs_handle *xs, const char *dir, ...)
>  {
> @@ -454,10 +472,8 @@ static int domain_create_ring(struct dom
> dom->local_port = rc;
> dom->remote_port = remote_port;
>  
> - if (dom->tty_fd == -1) {
> -  dom->tty_fd = domain_create_tty(dom);
> -
> -  if (dom->tty_fd == -1) {
> + if (dom->master_fd == -1) {
> +  if (!domain_create_tty(dom)) {
> err = errno;
> xc_evtchn_close(dom->xce_handle);
> dom->xce_handle = -1;
> @@ -535,7 +551,8 @@ static struct domain *create_domain(int
> dom->conspath = s;
> strcat(dom->conspath, "/console");
>  
> - dom->tty_fd = -1;
> + dom->master_fd = -1;
> + dom->slave_fd = -1;
> dom->log_fd = -1;
>  
> dom->is_dead = false;
> @@ -597,14 +614,7 @@ static void remove_domain(struct domain
>  
>  static void cleanup_domain(struct domain *d)
>  {
> - if (d->tty_fd != -1) {
> -  close(d->tty_fd);
> -  d->tty_fd = -1;
> - }
> - if (d->log_fd != -1) {
> -  close(d->log_fd);
> -  d->log_fd = -1;
> - }
> + domain_close_tty(d);
>  
> free(d->buffer.data);
> d->buffer.data = NULL;
> @@ -683,13 +693,17 @@ static void handle_tty_read(struct domai
> if (len > sizeof(msg))
> len = sizeof(msg);
>  
> - len = read(dom->tty_fd, msg, len);
> - if (len < 1) {
> -  close(dom->tty_fd);
> -  dom->tty_fd = -1;
> + len = read(dom->master_fd, msg, len);
> + /*
> +  * Note: on Solaris, len == 0 means the slave closed, and this
> +  * is no problem, but Linux can't handle this usefully, so we
> +  * keep the slave open for the duration.
> +  */
> + if (len < 0) {
> +  domain_close_tty(dom);
>  
> if (domain_is_valid(dom->domid)) {
> -   dom->tty_fd = domain_create_tty(dom);
> +   domain_create_tty(dom);
> } else {
> shutdown_domain(dom);
> }
> @@ -703,8 +717,7 @@ static void handle_tty_read(struct domai
> intf->in_prod = prod;
> xc_evtchn_notify(dom->xce_handle, dom->local_port);
> } else {
> -  close(dom->tty_fd);
> -  dom->tty_fd = -1;
> +  domain_close_tty(dom);
> shutdown_domain(dom);
> }
>  }
> @@ -716,17 +729,16 @@ static void handle_tty_write(struct doma
> if (dom->is_dead)
> return;
>  
> - len = write(dom->tty_fd, dom->buffer.data + dom->buffer.consumed,
> + len = write(dom->master_fd, dom->buffer.data + dom->buffer.consumed,
>    dom->buffer.size - dom->buffer.consumed);
> if (len < 1) {
> dolog(LOG_DEBUG, "Write failed on domain %d: %zd, %d\n",
>      dom->domid, len, errno);
>  
> -  close(dom->tty_fd);
> -  dom->tty_fd = -1;
> +  domain_close_tty(dom);
>  
> if (domain_is_valid(dom->domid)) {
> -   dom->tty_fd = domain_create_tty(dom);
> +   domain_create_tty(dom);
> } else {
> shutdown_domain(dom);
> }
> @@ -895,13 +907,13 @@ void handle_io(void)
> max_fd = MAX(evtchn_fd, max_fd);
> }
>  
> -   if (d->tty_fd != -1) {
> +   if (d->master_fd != -1) {
> if (!d->is_dead && ring_free_bytes(d))
> -     FD_SET(d->tty_fd, &readfds);
> +     FD_SET(d->master_fd, &readfds);
>  
> if (!buffer_empty(&d->buffer))
> -     FD_SET(d->tty_fd, &writefds);
> -    max_fd = MAX(d->tty_fd, max_fd);
> +     FD_SET(d->master_fd, &writefds);
> +    max_fd = MAX(d->master_fd, max_fd);
> }
> }
>  
> @@ -951,10 +963,10 @@ void handle_io(void)
> handle_ring_read(d);
> }
>  
> -   if (d->tty_fd != -1 && FD_ISSET(d->tty_fd, &readfds))
> +   if (d->master_fd != -1 && FD_ISSET(d->master_fd, &readfds))
> handle_tty_read(d);
>  
> -   if (d->tty_fd != -1 && FD_ISSET(d->tty_fd, &writefds))
> +   if (d->master_fd != -1 && FD_ISSET(d->master_fd, &writefds))
> handle_tty_write(d);
>  
> if (d->is_dead)
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel



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