[Qemu-devel] Re: [PATCH 2/3] Always use nonblocking mode for qemu_chr_open_fd.

[Anthony Liguori <anthony@codemonkey.ws>]

Gerd Hoffmann wrote:
> From 076b5cff55b57a64e5f8daf32186d924945f78b4 Mon Sep 17 00:00:00 2001
> From: Gerd Hoffmann <kraxel@redhat.com>
> Date: Wed, 23 Jul 2008 16:35:40 +0200
> Subject: [PATCH 03/21] Attempt to detect unconnected ptys.
>
> This patch moves the pty char device imlementation away from the generic
> filehandle code.  It tries to detect as good as possible whenever there
> is someone connected to the slave pty device and only send data down the
> road in case someone is listening.  Unfortunaly we have to poll via
> timer once in a while to check the status because we have to use read()
> on the master pty to figure the status (returns -EIO when unconnected).
>
> Poll intervall for an idle guest is one second, when the guest sends
> data to the virtual device linked to the pty we check more frequently.
>
> The point for all of this is to avoid qemu blocking and not responding
> any more.  Writing to the master pty handle succeeds even when nobody is
> connected to (and reading from) to the slave end of the pty.  The kernel
> just bufferes the writes.  And as soon as the kernel buffer is full the
> write() call blocks forever ...
>   

You've got whitespace damage.  You're mixing tabs and spaces.  Please 
only use spaces.

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  vl.c |  158 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 154 insertions(+), 4 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 23b5243..faf7b95 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2474,21 +2474,171 @@ void cfmakeraw (struct termios *termios_p)
>  #endif
>  
>  #if defined(__linux__) || defined(__sun__)
> +
> +typedef struct {
> +    int fd;
> +    int connected;
> +    int polling;
> +    int read_bytes;
> +    QEMUTimer *timer;
> +} PtyCharDriver;
> +
> +static void pty_chr_update_read_handler(CharDriverState *chr);
> +static void pty_chr_state(CharDriverState *chr, int connected);
> +
> +static int pty_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
> +{
> +    PtyCharDriver *s = chr->opaque;
> +
> +    if (!s->connected) {
> +	/* guest sends data, check for (re-)connect */
> +	pty_chr_update_read_handler(chr);
> +	return 0;
> +    }
> +    return unix_write(s->fd, buf, len);
> +}
> +
> +static int pty_chr_read_poll(void *opaque)
> +{
> +    CharDriverState *chr = opaque;
> +    PtyCharDriver *s = chr->opaque;
> +
> +    s->read_bytes = qemu_chr_can_read(chr);
> +    return s->read_bytes;
> +}
> +
> +static void pty_chr_read(void *opaque)
> +{
> +    CharDriverState *chr = opaque;
> +    PtyCharDriver *s = chr->opaque;
> +    int size, len;
> +    uint8_t buf[1024];
> +
> +    len = sizeof(buf);
> +    if (len > s->read_bytes)
> +        len = s->read_bytes;
> +    if (len == 0)
> +        return;
> +    size = read(s->fd, buf, len);
> +    if ((size == -1 && EIO == errno) ||
> +	(size == 0)) {
>   

errno == EIO

> +	qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> +	s->polling = 0;
> +	pty_chr_state(chr, 0);
> +        return;
> +    }
> +    if (size > 0) {
> +	pty_chr_state(chr, 1);
> +        qemu_chr_read(chr, buf, size);
> +    }
> +}
> +
> +static void pty_chr_update_read_handler(CharDriverState *chr)
> +{
> +    PtyCharDriver *s = chr->opaque;
> +
> +    qemu_set_fd_handler2(s->fd, pty_chr_read_poll,
> +			 pty_chr_read, NULL, chr);
> +    s->polling = 1;
> +    /*
> +     * Short timeout here: just need wait long enougth that qemu makes
> +     * it through the poll loop once.  When reconnected we want a
> +     * short timeout so we notice it almost instantly.  Otherwise
> +     * read() gives us -EIO instantly, making pty_chr_state() reset the
> +     * timeout to the normal (much longer) poll interval before the
> +     * timer triggers.
> +     */
> +    qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + 10);
>   

Perhaps what you really want here is a bottom half?

> +}
> +
> +static void pty_chr_state(CharDriverState *chr, int connected)
> +{
> +    PtyCharDriver *s = chr->opaque;
> +
> +    if (!connected) {
> +	if (s->connected) {
> +	    fprintf(stderr,"%s: %s disconnected\n", __FUNCTION__, ptsname(s->fd));
>   

Obviously this debugging has to go.

> +	    s->connected = 0;
> +	}
> +	/* (re-)connect poll interval for idle guests: once per second.
> +	 * We check more frequently in case the guests sends data to
> +	 * the virtual device linked to our pty. */
> +	qemu_mod_timer(s->timer, qemu_get_clock(rt_clock) + 1000);
> +    } else {
> +	if (!s->connected) {
> +	    fprintf(stderr,"%s: %s connected\n", __FUNCTION__, ptsname(s->fd));
> +	    s->connected = 1;
> +	    qemu_chr_reset(chr);
> +	}
> +    }
> +}
> +
> +void pty_chr_timer(void *opaque)
> +{
> +    struct CharDriverState *chr = opaque;    
> +    PtyCharDriver *s = chr->opaque;
> +
> +    if (s->connected) {
> +	/* All fine, nothing to do. */
> +	return;
> +    }
> +    if (s->polling) {
> +	/* Ran for a while without getting EIO for reads,
> +	 * probably someone connected to the slave pty. */
> +	pty_chr_state(chr, 1);
> +	return;
> +    }
> +    /* Try reading again ... */
> +#if 0
> +    fprintf(stderr,"%s: %s checking ...\n", __FUNCTION__, ptsname(s->fd));
> +#endif
> +    pty_chr_update_read_handler(chr);
> +}
> +
> +static void pty_chr_close(struct CharDriverState *chr)
> +{
> +    PtyCharDriver *s = chr->opaque;
> +
> +    qemu_set_fd_handler2(s->fd, NULL, NULL, NULL, NULL);
> +    close(s->fd);
> +    qemu_free(s);
> +}
> +
>  static CharDriverState *qemu_chr_open_pty(void)
>  {
> +    CharDriverState *chr;
> +    PtyCharDriver *s;
>      struct termios tty;
> -    int master_fd, slave_fd;
> +    int slave_fd;
> +    
> +    chr = qemu_mallocz(sizeof(CharDriverState));
> +    if (!chr)
> +        return NULL;
> +    s = qemu_mallocz(sizeof(PtyCharDriver));
> +    if (!s) {
> +        free(chr);
>   

qemu_free()

Regards,

Anthony Liguori

> +        return NULL;
> +    }
>  
> -    if (openpty(&master_fd, &slave_fd, NULL, NULL, NULL) < 0) {
> +    if (openpty(&s->fd, &slave_fd, NULL, NULL, NULL) < 0) {
>          return NULL;
>      }
>  
>      /* Set raw attributes on the pty. */
>      cfmakeraw(&tty);
>      tcsetattr(slave_fd, TCSAFLUSH, &tty);
> +    close(slave_fd);
> +    
> +    fprintf(stderr, "char device redirected to %s\n", ptsname(s->fd));
> +    
> +    chr->opaque = s;
> +    chr->chr_write = pty_chr_write;
> +    chr->chr_update_read_handler = pty_chr_update_read_handler;
> +    chr->chr_close = pty_chr_close;
>  
> -    fprintf(stderr, "char device redirected to %s\n", ptsname(master_fd));
> -    return qemu_chr_open_fd(master_fd, master_fd);
> +    s->timer = qemu_new_timer(rt_clock, pty_chr_timer, chr);
> +    
> +    return chr;
>  }
>  
>  static void tty_serial_init(int fd, int speed,
> -- 1.5.4.1