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
|
Home