diff mbox series

[v1,4/7] io: pass a struct iovec into qio_channel_websock_encode

Message ID 20171010154328.8419-5-berrange@redhat.com
State New
Headers show
Series Limit websockets memory usage & other bug fixes | expand

Commit Message

Daniel P. Berrangé Oct. 10, 2017, 3:43 p.m. UTC 
Instead of requiring use of another Buffer, pass a struct iovec
into qio_channel_websock_encode, which gives callers more
flexibility in how they process data.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 io/channel-websock.c | 69 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 42 insertions(+), 27 deletions(-)

Comments

Eric Blake Oct. 10, 2017, 5:18 p.m. UTC | #1 
On 10/10/2017 10:43 AM, Daniel P. Berrange wrote:
> Instead of requiring use of another Buffer, pass a struct iovec
> into qio_channel_websock_encode, which gives callers more
> flexibility in how they process data.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  io/channel-websock.c | 69 ++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 42 insertions(+), 27 deletions(-)
> 

> +static void qio_channel_websock_encode(QIOChannelWebsock *ioc,
> +                                       uint8_t opcode,
> +                                       const struct iovec *iov,
> +                                       size_t niov,
> +                                       size_t size)
>  {

Is size redundant with iov_size(iov, niov)?  Or is a caller allowed to
pass a smaller size (tail end of iov is not encoded) or larger size
(encoding stops at end of iov, even if size is not exhausted)?  I'd lean
towards the former (one less parameter), especially since all callers in
this patch could have passed iov_size(&iov, 1) for the same effect.

> -    trace_qio_channel_websock_encode(ioc, opcode, header_size, buffer->offset);
> -    buffer_reserve(output, header_size + buffer->offset);
> -    buffer_append(output, header.buf, header_size);
> -    buffer_append(output, buffer->buffer, buffer->offset);
> +    trace_qio_channel_websock_encode(ioc, opcode, header_size, size);
> +    buffer_reserve(&ioc->encoutput, header_size + size);
> +    buffer_append(&ioc->encoutput, header.buf, header_size);
> +    for (i = 0; i < niov && size != 0; i++) {
> +        size_t want = iov->iov_len;
> +        if (want > size) {
> +            want = size;
> +        }
> +        buffer_append(&ioc->encoutput, iov->iov_base, want);
> +        size -= want;
> +    }

Umm, where are you incrementing iov? It appears you only tested with
niov == 1.
Daniel P. Berrangé Oct. 10, 2017, 5:36 p.m. UTC | #2 
On Tue, Oct 10, 2017 at 12:18:26PM -0500, Eric Blake wrote:
> On 10/10/2017 10:43 AM, Daniel P. Berrange wrote:
> > Instead of requiring use of another Buffer, pass a struct iovec
> > into qio_channel_websock_encode, which gives callers more
> > flexibility in how they process data.
> > 
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  io/channel-websock.c | 69 ++++++++++++++++++++++++++++++++--------------------
> >  1 file changed, 42 insertions(+), 27 deletions(-)
> > 
> 
> > +static void qio_channel_websock_encode(QIOChannelWebsock *ioc,
> > +                                       uint8_t opcode,
> > +                                       const struct iovec *iov,
> > +                                       size_t niov,
> > +                                       size_t size)
> >  {
> 
> Is size redundant with iov_size(iov, niov)?  Or is a caller allowed to
> pass a smaller size (tail end of iov is not encoded) or larger size
> (encoding stops at end of iov, even if size is not exhausted)?  I'd lean
> towards the former (one less parameter), especially since all callers in
> this patch could have passed iov_size(&iov, 1) for the same effect.

You've already noticed the code in later patch which passes a smaller
size. This lets me avoid having to duplicate the iovec array and
set smaller iov_len

> 
> > -    trace_qio_channel_websock_encode(ioc, opcode, header_size, buffer->offset);
> > -    buffer_reserve(output, header_size + buffer->offset);
> > -    buffer_append(output, header.buf, header_size);
> > -    buffer_append(output, buffer->buffer, buffer->offset);
> > +    trace_qio_channel_websock_encode(ioc, opcode, header_size, size);
> > +    buffer_reserve(&ioc->encoutput, header_size + size);
> > +    buffer_append(&ioc->encoutput, header.buf, header_size);
> > +    for (i = 0; i < niov && size != 0; i++) {
> > +        size_t want = iov->iov_len;
> > +        if (want > size) {
> > +            want = size;
> > +        }
> > +        buffer_append(&ioc->encoutput, iov->iov_base, want);
> > +        size -= want;
> > +    }
> 
> Umm, where are you incrementing iov? It appears you only tested with
> niov == 1.

Opps. Yeah, we need to cope with niov > 0 to satisfy qio_channel_writev
API contract, but the VNC server only ever sends a single iov element
so I didn't hit the bug.

Regards,
Daniel
diff mbox series

Patch

diff --git a/io/channel-websock.c b/io/channel-websock.c
index 17f50f5bb1..ad62dda479 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -582,11 +582,14 @@  static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,
 }
 
 
-static void qio_channel_websock_encode_buffer(QIOChannelWebsock *ioc,
-                                              Buffer *output,
-                                              uint8_t opcode, Buffer *buffer)
+static void qio_channel_websock_encode(QIOChannelWebsock *ioc,
+                                       uint8_t opcode,
+                                       const struct iovec *iov,
+                                       size_t niov,
+                                       size_t size)
 {
     size_t header_size;
+    size_t i;
     union {
         char buf[QIO_CHANNEL_WEBSOCK_HEADER_LEN_64_BIT];
         QIOChannelWebsockHeader ws;
@@ -594,25 +597,31 @@  static void qio_channel_websock_encode_buffer(QIOChannelWebsock *ioc,
 
     header.ws.b0 = QIO_CHANNEL_WEBSOCK_HEADER_FIELD_FIN |
         (opcode & QIO_CHANNEL_WEBSOCK_HEADER_FIELD_OPCODE);
-    if (buffer->offset < QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_THRESHOLD_7_BIT) {
-        header.ws.b1 = (uint8_t)buffer->offset;
+    if (size < QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_THRESHOLD_7_BIT) {
+        header.ws.b1 = (uint8_t)size;
         header_size = QIO_CHANNEL_WEBSOCK_HEADER_LEN_7_BIT;
-    } else if (buffer->offset <
-               QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_THRESHOLD_16_BIT) {
+    } else if (size < QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_THRESHOLD_16_BIT) {
         header.ws.b1 = QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_MAGIC_16_BIT;
-        header.ws.u.s16.l16 = cpu_to_be16((uint16_t)buffer->offset);
+        header.ws.u.s16.l16 = cpu_to_be16((uint16_t)size);
         header_size = QIO_CHANNEL_WEBSOCK_HEADER_LEN_16_BIT;
     } else {
         header.ws.b1 = QIO_CHANNEL_WEBSOCK_PAYLOAD_LEN_MAGIC_64_BIT;
-        header.ws.u.s64.l64 = cpu_to_be64(buffer->offset);
+        header.ws.u.s64.l64 = cpu_to_be64(size);
         header_size = QIO_CHANNEL_WEBSOCK_HEADER_LEN_64_BIT;
     }
     header_size -= QIO_CHANNEL_WEBSOCK_HEADER_LEN_MASK;
 
-    trace_qio_channel_websock_encode(ioc, opcode, header_size, buffer->offset);
-    buffer_reserve(output, header_size + buffer->offset);
-    buffer_append(output, header.buf, header_size);
-    buffer_append(output, buffer->buffer, buffer->offset);
+    trace_qio_channel_websock_encode(ioc, opcode, header_size, size);
+    buffer_reserve(&ioc->encoutput, header_size + size);
+    buffer_append(&ioc->encoutput, header.buf, header_size);
+    for (i = 0; i < niov && size != 0; i++) {
+        size_t want = iov->iov_len;
+        if (want > size) {
+            want = size;
+        }
+        buffer_append(&ioc->encoutput, iov->iov_base, want);
+        size -= want;
+    }
 }
 
 
@@ -622,6 +631,7 @@  static ssize_t qio_channel_websock_write_wire(QIOChannelWebsock *, Error **);
 static void qio_channel_websock_write_close(QIOChannelWebsock *ioc,
                                             uint16_t code, const char *reason)
 {
+    struct iovec iov;
     buffer_reserve(&ioc->rawoutput, 2 + (reason ? strlen(reason) : 0));
     *(uint16_t *)(ioc->rawoutput.buffer + ioc->rawoutput.offset) =
         cpu_to_be16(code);
@@ -629,9 +639,10 @@  static void qio_channel_websock_write_close(QIOChannelWebsock *ioc,
     if (reason) {
         buffer_append(&ioc->rawoutput, reason, strlen(reason));
     }
-    qio_channel_websock_encode_buffer(
-        ioc, &ioc->encoutput, QIO_CHANNEL_WEBSOCK_OPCODE_CLOSE,
-        &ioc->rawoutput);
+    iov.iov_base = ioc->rawoutput.buffer;
+    iov.iov_len = ioc->rawoutput.offset;
+    qio_channel_websock_encode(ioc, QIO_CHANNEL_WEBSOCK_OPCODE_CLOSE,
+                               &iov, 1, iov.iov_len);
     buffer_reset(&ioc->rawoutput);
     qio_channel_websock_write_wire(ioc, NULL);
     qio_channel_shutdown(ioc->master, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
@@ -801,9 +812,10 @@  static int qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
         error_setg(errp, "websocket closed by peer");
         if (payload_len) {
             /* echo client status */
-            qio_channel_websock_encode_buffer(
-                ioc, &ioc->encoutput, QIO_CHANNEL_WEBSOCK_OPCODE_CLOSE,
-                &ioc->encinput);
+            struct iovec iov = { .iov_base = ioc->encinput.buffer,
+                                 .iov_len = ioc->encinput.offset };
+            qio_channel_websock_encode(ioc, QIO_CHANNEL_WEBSOCK_OPCODE_CLOSE,
+                                       &iov, 1, iov.iov_len);
             qio_channel_websock_write_wire(ioc, NULL);
             qio_channel_shutdown(ioc->master, QIO_CHANNEL_SHUTDOWN_BOTH, NULL);
         } else {
@@ -816,9 +828,10 @@  static int qio_channel_websock_decode_payload(QIOChannelWebsock *ioc,
         /* ping frames produce an immediate reply, as long as we've not still
          * got a previous ping queued, in which case we drop the new pong */
         if (ioc->ping_remain == 0) {
-            qio_channel_websock_encode_buffer(
-                ioc, &ioc->encoutput, QIO_CHANNEL_WEBSOCK_OPCODE_PONG,
-                &ioc->encinput);
+            struct iovec iov = { .iov_base = ioc->encinput.buffer,
+                                 .iov_len = ioc->encinput.offset };
+            qio_channel_websock_encode(ioc, QIO_CHANNEL_WEBSOCK_OPCODE_PONG,
+                                       &iov, 1, iov.iov_len);
             ioc->ping_remain = ioc->encoutput.offset;
         }
     }   /* pong frames are ignored */
@@ -1120,11 +1133,13 @@  static ssize_t qio_channel_websock_writev(QIOChannel *ioc,
     }
 
  done:
-    if (ioc->rawoutput.offset) {
-        qio_channel_websock_encode_buffer(
-            ioc, &ioc->encoutput, QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME,
-            &ioc->rawoutput);
-        buffer_reset(&ioc->rawoutput);
+    if (wioc->rawoutput.offset) {
+        struct iovec iov = { .iov_base = wioc->rawoutput.buffer,
+                             .iov_len = wioc->rawoutput.offset };
+        qio_channel_websock_encode(wioc,
+                                   QIO_CHANNEL_WEBSOCK_OPCODE_BINARY_FRAME,
+                                   &iov, 1, iov.iov_len);
+        buffer_reset(&wioc->rawoutput);
     }
     ret = qio_channel_websock_write_wire(wioc, errp);
     if (ret < 0 &&