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

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

[John Baboval]

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

Includes changes recommended by Eric Blake and Paolo Bonzini:

Parameter documentation includes a (since 1.6)

Documentation for stale @type parameter was removed

maxqdepth parameter is now optional

Patch passes style checks

qemu_chr_parse_serial sets has_maxqdepth as appropriate

 include/sysemu/char.h |    2 ++
 qapi-schema.json      |    7 +++++--
 qemu-char.c           |   46 +++++++++++++++++++++++++++++++++++++++++++++-
 qemu-options.hx       |    4 ++--
 4 files changed, 54 insertions(+), 5 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..53a6d12 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
-# @type: What kind of device this is.
+#
+# @maxqdepth: #optional The maximum depth of the underlying tty
+#             output queue (Unix) (Since 1.6)
 #
 # Since: 1.4
 ##
-{ 'type': 'ChardevHostdev', 'data': { 'device' : 'str' } }
+{ 'type': 'ChardevHostdev', 'data': { 'device'    : 'str',
+                                      '*maxqdepth' : 'int' } }
 
 ##
 # @ChardevSocket:
diff --git a/qemu-char.c b/qemu-char.c
index 64e824d..cfc0793 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,23 @@ 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 +1456,18 @@ 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 +1496,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 +3203,11 @@ 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);
+    if (backend->serial->maxqdepth != -1) {
+        backend->serial->has_maxqdepth = true;
+    }
 }
 
 static void qemu_chr_parse_parallel(QemuOpts *opts, ChardevBackend *backend,
@@ -3575,6 +3611,9 @@ QemuOptsList qemu_chardev_opts = {
         },{
             .name = "size",
             .type = QEMU_OPT_SIZE,
+        },{
+            .name = "maxqdepth",
+            .type = QEMU_OPT_NUMBER,
         },
         { /* end of list */ }
     },
@@ -3653,6 +3692,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 +3700,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"
-- 
1.7.9.5

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1581 bytes --]

On 05/07/2013 04:39 PM, John Baboval wrote:
> 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.
> 

> +++ 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
> -# @type: What kind of device this is.
> +#
> +# @maxqdepth: #optional The maximum depth of the underlying tty
> +#             output queue (Unix) (Since 1.6)
>  #
>  # Since: 1.4
>  ##
> -{ 'type': 'ChardevHostdev', 'data': { 'device' : 'str' } }
> +{ 'type': 'ChardevHostdev', 'data': { 'device'    : 'str',
> +                                      '*maxqdepth' : 'int' } }
>  

Thanks; this interface change looks better (I'd still like to see
someone working on introspection, but it doesn't have to be you).  I'll
still leave the implementation details to others more qualified for that
part of the review.  In particular, since you are claiming this optional
attribute is Linux-only, that means we'd need introspection to know
whether a given qemu build supports the field (compiled on Linux) or not
(compiled on mingw), not just whether the qemu is new enough (1.6) or
older (1.4).

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]

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

[John Baboval]

On 05/10/2013 11:28 PM, Eric Blake wrote:
> On 05/07/2013 04:39 PM, John Baboval wrote:
>> 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.
>>
>> +++ 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
>> -# @type: What kind of device this is.
>> +#
>> +# @maxqdepth: #optional The maximum depth of the underlying tty
>> +#             output queue (Unix) (Since 1.6)
>>   #
>>   # Since: 1.4
>>   ##
>> -{ 'type': 'ChardevHostdev', 'data': { 'device' : 'str' } }
>> +{ 'type': 'ChardevHostdev', 'data': { 'device'    : 'str',
>> +                                      '*maxqdepth' : 'int' } }
>>   
> Thanks; this interface change looks better (I'd still like to see
> someone working on introspection, but it doesn't have to be you).  I'll
> still leave the implementation details to others more qualified for that
> part of the review.  In particular, since you are claiming this optional
> attribute is Linux-only, that means we'd need introspection to know
> whether a given qemu build supports the field (compiled on Linux) or not
> (compiled on mingw), not just whether the qemu is new enough (1.6) or
> older (1.4).
>
I believe (though I haven't tested on Windows) that the option at least 
does no harm there. You can still set it - It's just a no-op.