Re: [Qemu-devel] [PATCH] Add 'maxqdepth' as an option to tty character devices.

[Paolo Bonzini <pbonzini@redhat.com>]

Il 06/05/2013 15:43, John Baboval ha scritto:
> From: "John V. Baboval" <john.baboval@virtualcomputer.com>
> 
> This parameter will cause writes to tty backed chardevs to return
> -EAGAIN if the backing tty has buffered more than the specified
> number of characters. When data is sent, the TIOCOUTQ ioctl is invoked
> to determine the current TTY output buffer depth.
> 
> Background:
> 
> Some devices use DTR/DSR as flow control. (eg. Check/Receipt
> printers with some POS software). When the device de-asserts
> DTR, the guest OS notifies the application and new data is blocked.
> When running on a QEMU serial port backed by a TTY, though the guest
> stops transmitting, all the characters in the TTY output buffer are
> still sent. The device buffer overflows and data is lost. In this
> case the user could set maxqdepth=1.
> 
> Signed-off-by: John Baboval <john.baboval@citrix.com>
> ---
>  include/sysemu/char.h |    2 ++
>  qapi-schema.json      |    5 ++++-
>  qemu-char.c           |   40 +++++++++++++++++++++++++++++++++++++++-
>  qemu-options.hx       |    4 ++--
>  4 files changed, 47 insertions(+), 4 deletions(-)
> 
> diff --git a/include/sysemu/char.h b/include/sysemu/char.h
> index 5e42c90..a94c1fb 100644
> --- a/include/sysemu/char.h
> +++ b/include/sysemu/char.h
> @@ -43,6 +43,7 @@ typedef struct {
>  
>  #define CHR_IOCTL_SERIAL_SET_TIOCM   13
>  #define CHR_IOCTL_SERIAL_GET_TIOCM   14
> +#define CHR_IOCTL_SERIAL_TIOCOUTQ    15
>  
>  #define CHR_TIOCM_CTS	0x020
>  #define CHR_TIOCM_CAR	0x040
> @@ -77,6 +78,7 @@ struct CharDriverState {
>      int fe_open;
>      int explicit_fe_open;
>      int avail_connections;
> +    uint32_t maxqdepth;
>      QemuOpts *opts;
>      QTAILQ_ENTRY(CharDriverState) next;
>  };
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 7797400..029e7c9 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3182,11 +3182,14 @@
>  #
>  # @device: The name of the special file for the device,
>  #          i.e. /dev/ttyS0 on Unix or COM1: on Windows
> +# @maxqdepth: The maximum depth of the underlying tty
> +              output queue (Unix) 
>  # @type: What kind of device this is.
>  #
>  # Since: 1.4
>  ##
> -{ 'type': 'ChardevHostdev', 'data': { 'device' : 'str' } }
> +{ 'type': 'ChardevHostdev', 'data': { 'device'    : 'str',
> +                                      'maxqdepth' : 'int' } }

This needs to be optional for backwards compatibility.  You can check
serial->has_maxqdepth and use a default value of -1 if it is true...

>  
>  ##
>  # @ChardevSocket:
> diff --git a/qemu-char.c b/qemu-char.c
> index 64e824d..e2e4217 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -782,6 +782,7 @@ typedef struct FDCharDriver {
>      GIOChannel *fd_in, *fd_out;
>      guint fd_in_tag;
>      int max_size;
> +    int tiocoutq_failed;
>      QTAILQ_ENTRY(FDCharDriver) node;
>  } FDCharDriver;
>  
> @@ -1260,6 +1261,22 @@ static CharDriverState *qemu_chr_open_pty(const char *id,
>      return chr;
>  }
>  
> +static int tty_serial_write(CharDriverState *chr, const uint8_t *buf, int len)
> +{
> +    FDCharDriver *s = chr->opaque;
> +    uint32_t inflight = 0;
> +
> +    qemu_chr_fe_ioctl(chr, CHR_IOCTL_SERIAL_TIOCOUTQ, &inflight);
> +    if (inflight >= chr->maxqdepth)
> +        return -EAGAIN;
> +
> +    if (inflight + len > chr->maxqdepth) {
> +        len = chr->maxqdepth - inflight;
> +    }
> +
> +    return io_channel_send(s->fd_out, buf, len);
> +}
> +
>  static void tty_serial_init(int fd, int speed,
>                              int parity, int data_bits, int stop_bits)
>  {
> @@ -1438,6 +1455,16 @@ static int tty_serial_ioctl(CharDriverState *chr, int cmd, void *arg)
>              ioctl(g_io_channel_unix_get_fd(s->fd_in), TIOCMSET, &targ);
>          }
>          break;
> +    case CHR_IOCTL_SERIAL_TIOCOUTQ:
> +        {
> +            if (!s->tiocoutq_failed)
> +                s->tiocoutq_failed = ioctl(g_io_channel_unix_get_fd(s->fd_in),
> +                                           TIOCOUTQ, arg);
> +
> +            if (s->tiocoutq_failed)
> +                *(unsigned int *)arg = 0;            
> +        }
> +        break;
>      default:
>          return -ENOTSUP;
>      }
> @@ -1466,6 +1493,7 @@ static CharDriverState *qemu_chr_open_tty_fd(int fd)
>  
>      tty_serial_init(fd, 115200, 'N', 8, 1);
>      chr = qemu_chr_open_fd(fd, fd);
> +    chr->chr_write = tty_serial_write;
>      chr->chr_ioctl = tty_serial_ioctl;
>      chr->chr_close = qemu_chr_close_tty;
>      return chr;
> @@ -3172,6 +3200,8 @@ static void qemu_chr_parse_serial(QemuOpts *opts, ChardevBackend *backend,
>      }
>      backend->serial = g_new0(ChardevHostdev, 1);
>      backend->serial->device = g_strdup(device);
> +    backend->serial->maxqdepth =
> +        qemu_opt_get_number(opts, "maxqdepth", -1);

... and also set has_maxqdepth here.

Thanks,

Paolo

>  }
>  
>  static void qemu_chr_parse_parallel(QemuOpts *opts, ChardevBackend *backend,
> @@ -3575,6 +3605,9 @@ QemuOptsList qemu_chardev_opts = {
>          },{
>              .name = "size",
>              .type = QEMU_OPT_SIZE,
> +        },{
> +            .name = "maxqdepth",
> +            .type = QEMU_OPT_NUMBER,
>          },
>          { /* end of list */ }
>      },
> @@ -3653,6 +3686,7 @@ static CharDriverState *qmp_chardev_open_serial(ChardevHostdev *serial,
>                                                  Error **errp)
>  {
>  #ifdef HAVE_CHARDEV_TTY
> +    CharDriverState *chr;
>      int fd;
>  
>      fd = qmp_chardev_open_file_source(serial->device, O_RDWR, errp);
> @@ -3660,7 +3694,11 @@ static CharDriverState *qmp_chardev_open_serial(ChardevHostdev *serial,
>          return NULL;
>      }
>      qemu_set_nonblock(fd);
> -    return qemu_chr_open_tty_fd(fd);
> +    chr = qemu_chr_open_tty_fd(fd);
> +    if (chr) {
> +        chr->maxqdepth = serial->maxqdepth;
> +    }
> +    return chr;
>  #else
>      error_setg(errp, "character device backend type 'serial' not supported");
>      return NULL;
> diff --git a/qemu-options.hx b/qemu-options.hx
> index e86cc24..c522f13 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -1792,8 +1792,8 @@ DEF("chardev", HAS_ARG, QEMU_OPTION_chardev,
>  #endif
>  #if defined(__linux__) || defined(__sun__) || defined(__FreeBSD__) \
>          || defined(__NetBSD__) || defined(__OpenBSD__) || defined(__DragonFly__)
> -    "-chardev serial,id=id,path=path[,mux=on|off]\n"
> -    "-chardev tty,id=id,path=path[,mux=on|off]\n"
> +    "-chardev serial,id=id,path=path[,mux=on|off][,maxqdepth=count]\n"
> +    "-chardev tty,id=id,path=path[,mux=on|off][,maxqdepth=count]\n"
>  #endif
>  #if defined(__linux__) || defined(__FreeBSD__) || defined(__DragonFly__)
>      "-chardev parallel,id=id,path=path[,mux=on|off]\n"
>