[Qemu-devel] [RFC][RESEND][PATCH v1 00/15] virtproxy: host/guest communication layer

[Qemu-devel] [RFC][RESEND][PATCH v1 00/15] virtproxy: host/guest communication layer

[Michael Roth]

This set of patches is a prereq for the proposed guest agent (virtagent), so resending these to accompany this morning's virtagent v2 submission.

OVERVIEW:

Virtproxy proxies and multiplexes socket streams over a data channel between a host and a guest (currently network connections, emulated serial, or virtio-serial channels are supported). This allows for services such as guest data collection agents, host/guest file transfer, and event generation/handling to be implemented/deployed as basic socket-based daemons, independently of the actual data channel.

This code is intended to provide a channel-independent abstraction layer for communicating with a QEMU-specific guest agent (in particular, the virtagent RPC guest agent which will follow this in a seperate patchset), but may have general utility beyond this (for instance: ssh/sftp/other guest agents/etc over isa/virtio serial), and so is submitted here as a seperate patchset.

Currently this communication involves 2 daemons (common code): 1 in the guest, and 1 in the host. Each end multiplexes/demultiplexes/proxies connections from the other end. In the future we hope to integrate the host component directly into qemu as a chardev.

BUILD/USAGE INFO:
  make qemu-vp
  ./qemu-vp -h

EXAMPLE USAGE:

 - Proxy http and ssh connections from a host to a guest over a virtio-serial connection:
    # start guest with virtio-serial. for example (RHEL6s13):
    qemu \
    -device virtio-serial \
    -chardev socket,path=/tmp/test0-virtioconsole.sock,server,nowait,id=test0 \
    -device virtconsole,chardev=test0,name=test0 \
    -chardev socket,path=/tmp/test1-virtio-serial.sock,server,nowait,id=test1 \
    -device virtserialport,chardev=test1,name=test1 \
    -chardev socket,path=/tmp/test2-virtio-serial.sock,server,nowait,id=test2 \
    -device virtserialport,chardev=test2,name=test2 \
    ...
    # in the host:
    ./qemu-vp -c unix-connect:/tmp/test2-virtio-serial.sock:- -o http:127.0.0.1:9080 \
              -o ssh:127.0.0.1:9022
    # in the guest:
    ./qemu-vp -c virtserial-open:/dev/virtio-ports/test2:- -i http:127.0.0.1:80 \
              -i ssh:127.0.0.1:22

    # from host, access guest http server
    wget http://locahost:9080
    # from host, access guest ssh server
    ssh localhost -p 9022

 - Proxy http and ssh connections from a host to a guest over a network connection:
    # start guest with network connectivity to host
    # in the guest:
    ./qemu-vp -c tcp-listen:<guest_ip>:9000 -i http:127.0.0.1:80 \
              -i ssh:127.0.0.1:22
    # in the host:
    ./qemu-vp -c tcp-connect:<guest_ip>:9000 -o http:127.0.0.1:9080 \
              -o ssh:127.0.0.1:9022
    ...

By specifying -i and -o options in the host and guest, respectively, the channel can also be used to establish connections from a guest to a host.

KNOWN ISSUES:

 - Deadlocking the guest: In tests over isa-serial ports I've hit cases where the chardev (socket) on the host-side seem to fill up the buffer, likely due to qemu rate-limiting data in accordance with the port's baud rate (which may explain why i hadn't seen this with network-based or virtio-serial data channels. When qemu-vp reads data from client connections it puts it into a VPPacket and tries to send the packet in it's entirety back over the channel. In this particular case that write() blocks (or vp_send_all() spins if we set O_NONBLOCK on the client FD). In the meantime qemu fills up the other end of the socket buffer and ends up spinning in qemu-char:send_all(), basically causing a deadlock between qemu and qemu-vp, and causing the guest to freeze.

   Currently I'm planning on replacing vp_send_all() with a function that simply buffers write()'s, which would allow the use of non-blocking write()'s out to the channel/chardev socket while still retaining wholeness/fifo-ordering of the VPPackets.

 - Sync issues with virtio-serial: This may or may not be related to the issue above, but I noticed some cases where proxied ssh sessions from the guest to the host would "lag" by a few bytes. For instance typing "top" would result in "to" being displayed, and the "p" wouldn't show up till I hit another key. This could be related to how I'm handling the buffering, but I haven't been able to reproduce using a network-based channel.

TODO:

 - Rework vp_send_all() to use buffering to avoid above-mentioned deadlock scenario 
 - Integrate qemu-vp directly into qemu by adding a virtproxy chardev device. For example:
     ./qemu-vp -c unix-connect:/tmp/vp1-virtio-serial.sock:- -o ssh:127.0.0.1:9022
   in the host, would be analogous to:
     qemu \
     -device virtio-serial \
     -chardev virtproxy,oforward=ssh:127.0.0.1:9022,id=vp1 \
     -device virtserialport,chardev=vp1,name=vp1
 - Better channel negotiation to gracefully handle guest reboots/disconnects/etc
 - Add monitor commands to add/remove virtproxy channels/oforwards/iforwards on the fly

 .gitignore  |    1 +
 Makefile    |    4 +-
 configure   |    1 +
 qemu-vp.c   |  618 +++++++++++++++++++++++++++++++++++++++++++++
 virtproxy.c |  799 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 virtproxy.h |   40 +++
 6 files changed, 1462 insertions(+), 1 deletions(-)

[Qemu-devel] [RFC][RESEND][PATCH v1 01/15] virtproxy: base data structures and constants

[Michael Roth]

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 virtproxy.c |  134 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 virtproxy.h |   34 +++++++++++++++
 2 files changed, 168 insertions(+), 0 deletions(-)
 create mode 100644 virtproxy.c
 create mode 100644 virtproxy.h

diff --git a/virtproxy.c b/virtproxy.c
new file mode 100644
index 0000000..f30b859
--- /dev/null
+++ b/virtproxy.c
@@ -0,0 +1,134 @@
+/*
+ * virt-proxy - host/guest communication layer
+ *
+ * Copyright IBM Corp. 2010
+ *
+ * Authors:
+ *  Michael Roth      <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "virtproxy.h"
+
+#define VP_SERVICE_ID_LEN 32    /* max length of service id string */
+#define VP_PKT_DATA_LEN 1024    /* max proxied bytes per VPPacket */
+#define VP_CONN_DATA_LEN 1024   /* max bytes conns can send at a time */
+#define VP_MAGIC 0x1F374059
+
+/* listening fd, one for each service we're forwarding to remote end */
+typedef struct VPOForward {
+    VPDriver *drv;
+    int listen_fd;
+    char service_id[VP_SERVICE_ID_LEN];
+    QLIST_ENTRY(VPOForward) next;
+} VPOForward;
+
+/* service_id->path/port mapping of each service forwarded from remote end */
+typedef struct VPIForward {
+    VPDriver *drv;
+    char service_id[VP_SERVICE_ID_LEN];
+    QemuOpts *socket_opts;
+    QLIST_ENTRY(VPIForward) next;
+} VPIForward;
+
+/* proxied client/server connected states */
+typedef struct VPConn {
+    VPDriver *drv;
+    int client_fd;
+    int server_fd;
+    enum {
+        VP_CONN_CLIENT = 1,
+        VP_CONN_SERVER,
+    } type;
+    enum {
+        VP_STATE_NEW = 1,   /* accept()'d and registered fd */
+        VP_STATE_INIT,      /* sent init pkt to remote end, waiting for ack */
+        VP_STATE_CONNECTED, /* client and server connected */
+    } state;
+    QLIST_ENTRY(VPConn) next;
+} VPConn;
+
+typedef struct VPControlMsg {
+    enum {
+        VP_CONTROL_CONNECT_INIT = 1,
+        VP_CONTROL_CONNECT_ACK,
+        VP_CONTROL_CLOSE,
+    } type;
+    union {
+        /* tell remote end connect to server and map client_fd to it */
+        struct {
+            int client_fd;
+            char service_id[VP_SERVICE_ID_LEN];
+        } connect_init;
+        /* tell remote end we've created the connection to the server,
+         * and give them the corresponding fd to use so we don't have
+         * to do a reverse lookup everytime
+         */
+        struct {
+            int client_fd;
+            int server_fd;
+        } connect_ack;
+        /* tell remote end to close fd in question, presumably because
+         * connection was closed on our end
+         */
+        struct {
+            int client_fd;
+            int server_fd;
+        } close;
+    } args;
+} VPControlMsg;
+
+typedef struct VPPacket {
+    enum {
+        VP_PKT_CONTROL = 1,
+        VP_PKT_CLIENT,
+        VP_PKT_SERVER,
+    } type;
+    union {
+        VPControlMsg msg;
+        struct {
+            int client_fd;
+            int server_fd;
+            int bytes;
+            char data[VP_PKT_DATA_LEN];
+        } proxied;
+    } payload;
+    int magic;
+} __attribute__((__packed__)) VPPacket;
+
+struct VPDriver {
+    int channel_fd;
+    int listen_fd;
+    char buf[sizeof(VPPacket)];
+    int buflen;
+    QLIST_HEAD(, VPOForward) oforwards;
+    QLIST_HEAD(, VPIForward) iforwards;
+    QLIST_HEAD(, VPConn) conns;
+};
+
+static QemuOptsList vp_socket_opts = {
+    .name = "vp_socket_opts",
+    .head = QTAILQ_HEAD_INITIALIZER(vp_socket_opts.head),
+    .desc = {
+        {
+            .name = "path",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "host",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "port",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "ipv4",
+            .type = QEMU_OPT_BOOL,
+        },{
+            .name = "ipv6",
+            .type = QEMU_OPT_BOOL,
+        },
+        { /* end if list */ }
+    },
+};
diff --git a/virtproxy.h b/virtproxy.h
new file mode 100644
index 0000000..0203421
--- /dev/null
+++ b/virtproxy.h
@@ -0,0 +1,34 @@
+/*
+ * virt-proxy - host/guest communication layer
+ *
+ * Copyright IBM Corp. 2010
+ *
+ * Authors:
+ *  Michael Roth      <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#ifndef VIRTPROXY_H
+#define VIRTPROXY_H
+
+#include "qemu-common.h"
+#include "qemu-queue.h"
+
+typedef struct VPDriver VPDriver;
+
+/* wrappers for s/vp/qemu/ functions we need */
+int vp_send_all(int fd, const void *buf, int len1);
+int vp_set_fd_handler2(int fd,
+                         IOCanReadHandler *fd_read_poll,
+                         IOHandler *fd_read,
+                         IOHandler *fd_write,
+                         void *opaque);
+int vp_set_fd_handler(int fd,
+                        IOHandler *fd_read,
+                        IOHandler *fd_write,
+                        void *opaque);
+
+#endif /* VIRTPROXY_H */
-- 
1.7.0.4

[Qemu-devel] [RFC][RESEND][PATCH v1 02/15] virtproxy: qemu-vp, standalone daemon skeleton

[Michael Roth]

Daemon to be run in guest, or on host in standalone mode.
(re-)implements some qemu utility functions used by core virtproxy.c
code via wrapper functions. For built-in virtproxy code we will define
these wrapper functions in terms of qemu's built-in implementations.

Main logic will come in a later patch.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qemu-vp.c |  151 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 151 insertions(+), 0 deletions(-)
 create mode 100644 qemu-vp.c

diff --git a/qemu-vp.c b/qemu-vp.c
new file mode 100644
index 0000000..5075cdc
--- /dev/null
+++ b/qemu-vp.c
@@ -0,0 +1,151 @@
+/*
+ * virt-proxy - host/guest communication daemon
+ *
+ * Copyright IBM Corp. 2010
+ *
+ * Authors:
+ *  Michael Roth      <mdroth@linux.vnet.ibm.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "virtproxy.h"
+
+/* mirror qemu I/O-related code for standalone daemon */
+typedef struct IOHandlerRecord {
+    int fd;
+    IOCanReadHandler *fd_read_poll;
+    IOHandler *fd_read;
+    IOHandler *fd_write;
+    int deleted;
+    void *opaque;
+    /* temporary data */
+    struct pollfd *ufd;
+    QLIST_ENTRY(IOHandlerRecord) next;
+} IOHandlerRecord;
+
+static QLIST_HEAD(, IOHandlerRecord) io_handlers =
+    QLIST_HEAD_INITIALIZER(io_handlers);
+
+int vp_set_fd_handler2(int fd,
+                         IOCanReadHandler *fd_read_poll,
+                         IOHandler *fd_read,
+                         IOHandler *fd_write,
+                         void *opaque)
+{
+    IOHandlerRecord *ioh;
+
+    if (!fd_read && !fd_write) {
+        QLIST_FOREACH(ioh, &io_handlers, next) {
+            if (ioh->fd == fd) {
+                ioh->deleted = 1;
+                break;
+            }
+        }
+    } else {
+        QLIST_FOREACH(ioh, &io_handlers, next) {
+            if (ioh->fd == fd)
+                goto found;
+        }
+        ioh = qemu_mallocz(sizeof(IOHandlerRecord));
+        QLIST_INSERT_HEAD(&io_handlers, ioh, next);
+    found:
+        ioh->fd = fd;
+        ioh->fd_read_poll = fd_read_poll;
+        ioh->fd_read = fd_read;
+        ioh->fd_write = fd_write;
+        ioh->opaque = opaque;
+        ioh->deleted = 0;
+    }
+    return 0;
+}
+
+int vp_set_fd_handler(int fd,
+                        IOHandler *fd_read,
+                        IOHandler *fd_write,
+                        void *opaque)
+{
+    return vp_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
+}
+
+int vp_send_all(int fd, const void *buf, int len1)
+{
+    int ret, len;
+
+    len = len1;
+    while (len > 0) {
+        ret = write(fd, buf, len);
+        if (ret < 0) {
+            if (errno != EINTR && errno != EAGAIN) {
+                warn("write() failed");
+                return -1;
+            }
+        } else if (ret == 0) {
+            break;
+        } else {
+            buf += ret;
+            len -= ret;
+        }
+    }
+    return len1 - len;
+}
+
+static void main_loop_wait(int nonblocking)
+{
+    IOHandlerRecord *ioh;
+    fd_set rfds, wfds, xfds;
+    int ret, nfds;
+    struct timeval tv;
+    int timeout = 1000;
+
+    if (nonblocking) {
+        timeout = 0;
+    }
+
+    /* poll any events */
+    nfds = -1;
+    FD_ZERO(&rfds);
+    FD_ZERO(&wfds);
+    FD_ZERO(&xfds);
+    QLIST_FOREACH(ioh, &io_handlers, next) {
+        if (ioh->deleted)
+            continue;
+        if (ioh->fd_read &&
+            (!ioh->fd_read_poll ||
+             ioh->fd_read_poll(ioh->opaque) != 0)) {
+            FD_SET(ioh->fd, &rfds);
+            if (ioh->fd > nfds)
+                nfds = ioh->fd;
+        }
+        if (ioh->fd_write) {
+            FD_SET(ioh->fd, &wfds);
+            if (ioh->fd > nfds)
+                nfds = ioh->fd;
+        }
+    }
+
+    tv.tv_sec = timeout / 1000;
+    tv.tv_usec = (timeout % 1000) * 1000;
+
+    ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
+
+    if (ret > 0) {
+        IOHandlerRecord *pioh;
+
+        QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
+            if (ioh->deleted) {
+                QLIST_REMOVE(ioh, next);
+                qemu_free(ioh);
+                continue;
+            }
+            if (ioh->fd_read && FD_ISSET(ioh->fd, &rfds)) {
+                ioh->fd_read(ioh->opaque);
+            }
+            if (ioh->fd_write && FD_ISSET(ioh->fd, &wfds)) {
+                ioh->fd_write(ioh->opaque);
+            }
+        }
+    }
+}
-- 
1.7.0.4

[Qemu-devel] [RFC][RESEND][PATCH v1 03/15] virtproxy: add debug functions for virtproxy core

[Michael Roth]

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 virtproxy.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/virtproxy.c b/virtproxy.c
index f30b859..2f8996c 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -13,6 +13,23 @@
 
 #include "virtproxy.h"
 
+#define DEBUG_VP
+
+#ifdef DEBUG_VP
+#define TRACE(msg, ...) do { \
+    fprintf(stderr, "%s:%s():L%d: " msg "\n", \
+            __FILE__, __FUNCTION__, __LINE__, ## __VA_ARGS__); \
+} while(0)
+#else
+#define TRACE(msg, ...) \
+    do { } while (0)
+#endif
+
+#define LOG(msg, ...) do { \
+    fprintf(stderr, "%s:%s(): " msg "\n", \
+            __FILE__, __FUNCTION__, ## __VA_ARGS__); \
+} while(0)
+
 #define VP_SERVICE_ID_LEN 32    /* max length of service id string */
 #define VP_PKT_DATA_LEN 1024    /* max proxied bytes per VPPacket */
 #define VP_CONN_DATA_LEN 1024   /* max bytes conns can send at a time */
-- 
1.7.0.4

[Qemu-devel] [RFC][RESEND][PATCH v1 04/15] virtproxy: list look-up functions conns/oforwards/iforwards

[Michael Roth]

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 virtproxy.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 44 insertions(+), 0 deletions(-)

diff --git a/virtproxy.c b/virtproxy.c
index 2f8996c..fa17722 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -149,3 +149,47 @@ static QemuOptsList vp_socket_opts = {
         { /* end if list */ }
     },
 };
+
+/* get VPConn by fd, "client" denotes whether to look for client or server */
+static VPConn *get_conn(const VPDriver *drv, int fd, bool client)
+{
+    VPConn *c = NULL;
+    int cur_fd;
+
+    QLIST_FOREACH(c, &drv->conns, next) {
+        cur_fd = client ? c->client_fd : c->server_fd;
+        if (cur_fd == fd) {
+            return c;
+        }
+    }
+
+    return NULL;
+}
+
+/* get VPOForward by service_id */
+static VPOForward *get_oforward(const VPDriver *drv, const char *service_id)
+{
+    VPOForward *f = NULL;
+
+    QLIST_FOREACH(f, &drv->oforwards, next) {
+        if (strncmp(f->service_id, service_id, VP_SERVICE_ID_LEN) == 0) {
+            return f;
+        }
+    }
+
+    return NULL;
+}
+
+/* get VPIForward by service_id */
+static VPIForward *get_iforward(const VPDriver *drv, const char *service_id)
+{
+    VPIForward *f = NULL;
+
+    QLIST_FOREACH(f, &drv->iforwards, next) {
+        if (strncmp(f->service_id, service_id, VP_SERVICE_ID_LEN) == 0) {
+            return f;
+        }
+    }
+
+    return NULL;
+}
-- 
1.7.0.4

[Qemu-devel] [RFC][RESEND][PATCH v1 05/15] virtproxy: add accept handler for communication channel

[Michael Roth]

This accept()'s connections to the socket we told virt-proxy to listen
for the channel connection on and sets the appropriate read handler for
the resulting FD.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 virtproxy.c |   37 +++++++++++++++++++++++++++++++++++++
 1 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/virtproxy.c b/virtproxy.c
index fa17722..20532c2 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -166,6 +166,8 @@ static VPConn *get_conn(const VPDriver *drv, int fd, bool client)
     return NULL;
 }
 
+static void vp_channel_accept(void *opaque);
+
 /* get VPOForward by service_id */
 static VPOForward *get_oforward(const VPDriver *drv, const char *service_id)
 {
@@ -193,3 +195,38 @@ static VPIForward *get_iforward(const VPDriver *drv, const char *service_id)
 
     return NULL;
 }
+
+/* accept handler for communication channel
+ *
+ * accept()s connection to communication channel (for sockets), and sets
+ * up the read handler for resulting FD.
+ */
+static void vp_channel_accept(void *opaque)
+{
+    VPDriver *drv = opaque;
+    struct sockaddr_in saddr;
+    struct sockaddr *addr;
+    socklen_t len;
+    int fd;
+
+    TRACE("called with opaque: %p", drv);
+
+    for(;;) {
+        len = sizeof(saddr);
+        addr = (struct sockaddr *)&saddr;
+        fd = qemu_accept(drv->listen_fd, addr, &len);
+
+        if (fd < 0 && errno != EINTR) {
+            TRACE("accept() failed");
+            return;
+        } else if (fd >= 0) {
+            TRACE("accepted connection");
+            break;
+        }
+    }
+
+    drv->channel_fd = fd;
+    vp_set_fd_handler(drv->channel_fd, vp_channel_read, NULL, drv);
+    /* dont accept anymore connections until channel_fd is closed */
+    vp_set_fd_handler(drv->listen_fd, NULL, NULL, NULL);
+}
-- 
1.7.0.4

[Qemu-devel] [RFC][RESEND][PATCH v1 06/15] virtproxy: add read handler for communication channel

[Michael Roth]

Handle data coming in over the channel as VPPackets: Process control
messages and forward data from remote client/server connections to the
appropriate server/client FD on our end.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 virtproxy.c |   83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 83 insertions(+), 0 deletions(-)

diff --git a/virtproxy.c b/virtproxy.c
index 20532c2..c9c3022 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -33,6 +33,7 @@
 #define VP_SERVICE_ID_LEN 32    /* max length of service id string */
 #define VP_PKT_DATA_LEN 1024    /* max proxied bytes per VPPacket */
 #define VP_CONN_DATA_LEN 1024   /* max bytes conns can send at a time */
+#define VP_CHAN_DATA_LEN 4096   /* max bytes channel can send at a time */
 #define VP_MAGIC 0x1F374059
 
 /* listening fd, one for each service we're forwarding to remote end */
@@ -150,6 +151,8 @@ static QemuOptsList vp_socket_opts = {
     },
 };
 
+static void vp_channel_read(void *opaque);
+
 /* get VPConn by fd, "client" denotes whether to look for client or server */
 static VPConn *get_conn(const VPDriver *drv, int fd, bool client)
 {
@@ -230,3 +233,83 @@ static void vp_channel_accept(void *opaque)
     /* dont accept anymore connections until channel_fd is closed */
     vp_set_fd_handler(drv->listen_fd, NULL, NULL, NULL);
 }
+
+/* read handler for communication channel
+ *
+ * de-multiplexes data coming in over the channel. for control messages
+ * we process them here, for data destined for a service or client we
+ * send it to the appropriate FD.
+ */
+static void vp_channel_read(void *opaque)
+{
+    VPDriver *drv = opaque;
+    VPPacket pkt;
+    int count, ret, buf_offset;
+    char buf[VP_CHAN_DATA_LEN];
+    char *pkt_ptr, *buf_ptr;
+
+    TRACE("called with opaque: %p", drv);
+
+    count = read(drv->channel_fd, buf, sizeof(buf));
+
+    if (count == -1) {
+        LOG("read() failed: %s", strerror(errno));
+        return;
+    } else if (count == 0) {
+        /* TODO: channel closed, this probably shouldn't happen for guest-side
+         * serial/virtio-serial connections, but need to confirm and consider
+         * what should happen in this case. as it stands this virtproxy instance
+         * is basically defunct at this point, same goes for "client" instances
+         * of virtproxy where the remote end has hung-up.
+         */
+        LOG("channel connection closed");
+        vp_set_fd_handler(drv->channel_fd, NULL, NULL, drv);
+        drv->channel_fd = -1;
+        if (drv->listen_fd) {
+            vp_set_fd_handler(drv->listen_fd, vp_channel_accept, NULL, drv);
+        }
+        /* TODO: should close/remove/delete all existing VPConns here */
+    }
+
+    if (drv->buflen + count >= sizeof(VPPacket)) {
+        TRACE("initial packet, drv->buflen: %d", drv->buflen);
+        pkt_ptr = (char *)&pkt;
+        memcpy(pkt_ptr, drv->buf, drv->buflen);
+        pkt_ptr += drv->buflen;
+        memcpy(pkt_ptr, buf, sizeof(VPPacket) - drv->buflen);
+        /* handle first packet */
+        ret = vp_handle_packet(drv, &pkt);
+        if (ret != 0) {
+            LOG("error handling packet");
+        }
+        /* handle the rest of the buffer */
+        buf_offset = sizeof(VPPacket) - drv->buflen;
+        drv->buflen = 0;
+        buf_ptr = buf + buf_offset;
+        count -= buf_offset;
+        while (count > 0) {
+            if (count >= sizeof(VPPacket)) {
+                /* handle full packet */
+                TRACE("additional packet, drv->buflen: %d", drv->buflen);
+                memcpy((void *)&pkt, buf_ptr, sizeof(VPPacket));
+                ret = vp_handle_packet(drv, &pkt);
+                if (ret != 0) {
+                    LOG("error handling packet");
+                }
+                count -= sizeof(VPPacket);
+                buf_ptr += sizeof(VPPacket);
+            } else {
+                /* buffer the remainder */
+                TRACE("buffering packet");
+                memcpy(drv->buf, buf_ptr, count);
+                drv->buflen = count;
+                break;
+            }
+        }
+    } else {
+        /* haven't got a full VPPacket yet, buffer for later */
+        buf_ptr = drv->buf + drv->buflen;
+        memcpy(buf_ptr, buf, count);
+        drv->buflen += count;
+    }
+}
-- 
1.7.0.4

[Qemu-devel] [RFC][RESEND][PATCH v1 07/15] virtproxy: add vp_new() VPDriver constructor

[Michael Roth]

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 virtproxy.c |   23 +++++++++++++++++++++++
 virtproxy.h |    3 +++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/virtproxy.c b/virtproxy.c
index c9c3022..cc0ac9a 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -313,3 +313,26 @@ static void vp_channel_read(void *opaque)
         drv->buflen += count;
     }
 }
+
+/* create/init VPDriver object */
+VPDriver *vp_new(int fd, bool listen)
+{
+    VPDriver *drv = NULL;
+
+    drv = qemu_mallocz(sizeof(VPDriver));
+    drv->listen_fd = -1;
+    drv->channel_fd = -1;
+    QLIST_INIT(&drv->oforwards);
+    QLIST_INIT(&drv->conns);
+
+    if (listen) {
+        /* provided FD is to be listened on for channel connection */
+        drv->listen_fd = fd;
+        vp_set_fd_handler(drv->listen_fd, vp_channel_accept, NULL, drv);
+    } else {
+        drv->channel_fd = fd;
+        vp_set_fd_handler(drv->channel_fd, vp_channel_read, NULL, drv);
+    }
+
+    return drv;
+}
diff --git a/virtproxy.h b/virtproxy.h
index 0203421..3df1691 100644
--- a/virtproxy.h
+++ b/virtproxy.h
@@ -31,4 +31,7 @@ int vp_set_fd_handler(int fd,
                         IOHandler *fd_write,
                         void *opaque);
 
+/* virtproxy interface */
+VPDriver *vp_new(int fd, bool listen);
+
 #endif /* VIRTPROXY_H */
-- 
1.7.0.4

[Qemu-devel] [RFC][RESEND][PATCH v1 08/15] virtproxy: interfaces to set/remove/handle VPOForwards

[Michael Roth]

Functions to add listener FDs (oforwards) which set up proxied connections
to associated service, and the corresponding handler function to process
to new connections to said FDs and initialize new client connections to
the associated remote server over the channel

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 virtproxy.c |  103 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 virtproxy.h |    1 +
 2 files changed, 104 insertions(+), 0 deletions(-)

diff --git a/virtproxy.c b/virtproxy.c
index cc0ac9a..6c3611b 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -12,6 +12,7 @@
  */
 
 #include "virtproxy.h"
+#include "qemu_socket.h"
 
 #define DEBUG_VP
 
@@ -314,6 +315,70 @@ static void vp_channel_read(void *opaque)
     }
 }
 
+/* handler to accept() and init new client connections */
+static void vp_oforward_accept(void *opaque)
+{
+    VPOForward *f = opaque;
+    VPDriver *drv = f->drv;
+
+    struct sockaddr_in saddr;
+    struct sockaddr *addr;
+    socklen_t len;
+    int fd, ret;
+    VPConn *conn = NULL;
+    VPPacket pkt;
+    VPControlMsg msg;
+
+    TRACE("called with opaque: %p, drv: %p", f, drv);
+
+    for(;;) {
+        len = sizeof(saddr);
+        addr = (struct sockaddr *)&saddr;
+        fd = qemu_accept(f->listen_fd, addr, &len);
+
+        if (fd < 0 && errno != EINTR) {
+            TRACE("accept() failed");
+            return;
+        } else if (fd >= 0) {
+            TRACE("accepted connection");
+            break;
+        }
+    }
+
+    if (drv->channel_fd == -1) {
+        TRACE("communication channel not open, closing connection");
+        closesocket(fd);
+        return;
+    }
+
+    /* send init packet over channel */
+    memset(&msg, 0, sizeof(VPControlMsg));
+    msg.type = VP_CONTROL_CONNECT_INIT;
+    msg.args.connect_init.client_fd = fd;
+    pstrcpy(msg.args.connect_init.service_id, VP_SERVICE_ID_LEN, f->service_id);
+
+    memset(&pkt, 0, sizeof(VPPacket));
+    pkt.type = VP_PKT_CONTROL;
+    pkt.payload.msg = msg;
+    pkt.magic = VP_MAGIC;
+
+    ret = vp_send_all(drv->channel_fd, &pkt, sizeof(VPPacket));
+    if (ret == -1) {
+        LOG("vp_send_all() failed");
+        return;
+    }
+
+    /* create new VPConn for client */
+    conn = qemu_mallocz(sizeof(VPConn));
+    conn->drv = drv;
+    conn->client_fd = fd;
+    conn->type = VP_CONN_CLIENT;
+    conn->state = VP_STATE_NEW;
+    QLIST_INSERT_HEAD(&drv->conns, conn, next);
+
+    socket_set_nonblock(fd);
+}
+
 /* create/init VPDriver object */
 VPDriver *vp_new(int fd, bool listen)
 {
@@ -336,3 +401,41 @@ VPDriver *vp_new(int fd, bool listen)
 
     return drv;
 }
+
+/* set/modify/remove a service_id -> net/unix listening socket mapping
+ *
+ * "service_id" is a user-defined id for the service. this is what the
+ * client end will tag it's connections with so that the remote end can
+ * route it to the proper socket on the remote end.
+ *
+ * "fd" is a listen()'ing socket we want virtproxy to listen for new
+ * connections of this service type on. set "fd" to -1 to remove the 
+ * existing listening socket for this "service_id"
+ */
+int vp_set_oforward(VPDriver *drv, int fd, const char *service_id)
+{
+    VPOForward *f = get_oforward(drv, service_id);
+
+    if (fd == -1) {
+        if (f != NULL) {
+            vp_set_fd_handler(f->listen_fd, NULL, NULL, NULL);
+            QLIST_REMOVE(f, next);
+            qemu_free(f);
+        }
+        return 0;
+    }
+
+    if (f == NULL) {
+        f = qemu_mallocz(sizeof(VPOForward));
+        f->drv = drv;
+        strncpy(f->service_id, service_id, VP_SERVICE_ID_LEN);
+        QLIST_INSERT_HEAD(&drv->oforwards, f, next);
+    } else {
+        closesocket(f->listen_fd);
+    }
+
+    f->listen_fd = fd;
+    vp_set_fd_handler(f->listen_fd, vp_oforward_accept, NULL, f);
+
+    return 0;
+}
diff --git a/virtproxy.h b/virtproxy.h
index 3df1691..39d5d40 100644
--- a/virtproxy.h
+++ b/virtproxy.h
@@ -33,5 +33,6 @@ int vp_set_fd_handler(int fd,
 
 /* virtproxy interface */
 VPDriver *vp_new(int fd, bool listen);
+int vp_set_oforward(VPDriver *drv, int fd, const char *service_id);
 
 #endif /* VIRTPROXY_H */
-- 
1.7.0.4

[Qemu-devel] [RFC][RESEND][PATCH v1 09/15] virtproxy: add handler for data packets

[Michael Roth]

Process VPPackets coming in from channel and send them to the
appropriate server/client connections.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 virtproxy.c |   42 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/virtproxy.c b/virtproxy.c
index 6c3611b..57ab2b0 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -235,6 +235,48 @@ static void vp_channel_accept(void *opaque)
     vp_set_fd_handler(drv->listen_fd, NULL, NULL, NULL);
 }
 
+/* handle data packets
+ *
+ * process VPPackets containing data and send them to the corresponding
+ * FDs
+ */
+static int vp_handle_data_packet(void *drv, const VPPacket *pkt)
+{
+    int fd, ret;
+
+    TRACE("called with drv: %p", drv);
+
+    if (pkt->type == VP_PKT_CLIENT) {
+        TRACE("recieved client packet, client fd: %d, server fd: %d",
+              pkt->payload.proxied.client_fd, pkt->payload.proxied.server_fd);
+        fd = pkt->payload.proxied.server_fd;
+    } else if (pkt->type == VP_PKT_SERVER) {
+        TRACE("recieved server packet, client fd: %d, server fd: %d",
+              pkt->payload.proxied.client_fd, pkt->payload.proxied.server_fd);
+        fd = pkt->payload.proxied.client_fd;
+    } else {
+        TRACE("unknown packet type");
+        return -1;
+    }
+
+    /* TODO: proxied in non-blocking mode can causes us to spin here
+     * for slow servers/clients. need to use write()'s and maintain
+     * a per-conn write queue that we clear out before sending any
+     * more data to the fd
+     */
+    ret = vp_send_all(fd, (void *)pkt->payload.proxied.data,
+            pkt->payload.proxied.bytes);
+    if (ret == -1) {
+        LOG("error sending data over channel");
+        return -1;
+    } else if (ret != pkt->payload.proxied.bytes) {
+        TRACE("buffer full?");
+        return -1;
+    }
+
+    return 0;
+}
+
 /* read handler for communication channel
  *
  * de-multiplexes data coming in over the channel. for control messages
-- 
1.7.0.4

[Qemu-devel] [RFC][RESEND][PATCH v1 10/15] virtproxy: add handler for control packet

[Michael Roth]

Process control packets coming in over the channel. This entails setting
up/tearing down connections to local services initiated from the other
end of the channel.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 virtproxy.c |  154 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 154 insertions(+), 0 deletions(-)

diff --git a/virtproxy.c b/virtproxy.c
index 57ab2b0..4f56aba 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -235,6 +235,160 @@ static void vp_channel_accept(void *opaque)
     vp_set_fd_handler(drv->listen_fd, NULL, NULL, NULL);
 }
 
+/* handle control packets
+ *
+ * process VPPackets containing control messages
+ */
+static int vp_handle_control_packet(VPDriver *drv, const VPPacket *pkt)
+{
+    const VPControlMsg *msg = &pkt->payload.msg;
+    int ret;
+
+    TRACE("called with drv: %p", drv);
+
+    switch (msg->type) {
+    case VP_CONTROL_CONNECT_INIT: {
+        int client_fd = msg->args.connect_init.client_fd;
+        int server_fd;
+        char service_id[VP_SERVICE_ID_LEN];
+        VPPacket resp_pkt;
+        VPConn *new_conn;
+        VPIForward *iforward;
+
+        pstrcpy(service_id, VP_SERVICE_ID_LEN,
+                 msg->args.connect_init.service_id);
+        TRACE("setting up connection for service id %s", service_id);
+
+        /* create server connection on behalf of remote end */
+        iforward = get_iforward(drv, service_id);
+        if (iforward == NULL) {
+            LOG("no forwarder configured for service id");
+            return -1;
+        }
+
+        qemu_opts_print(iforward->socket_opts, NULL);
+        if (qemu_opt_get(iforward->socket_opts, "host") != NULL) {
+            server_fd = inet_connect_opts(iforward->socket_opts);
+        } else if (qemu_opt_get(iforward->socket_opts, "path") != NULL) {
+            server_fd = unix_connect_opts(iforward->socket_opts);
+        } else {
+            LOG("unable to find listening socket host/addr info");
+            return -1;
+        }
+
+        if (server_fd == -1) {
+            LOG("failed to create connection to service with id %s",
+                service_id);
+        }
+        TRACE("server_fd: %d", server_fd);
+
+        new_conn = qemu_mallocz(sizeof(VPConn));
+        if (!new_conn) {
+            LOG("memory allocation failed");
+            return -1;
+        }
+
+        /* send a connect_ack back over the channel */
+        /* TODO: all fields should be explicitly set so we shouldn't
+         * need to memset. this might hurt if we beef up VPPacket size
+         */
+        memset(&resp_pkt, 0, sizeof(resp_pkt));
+        resp_pkt.type = VP_PKT_CONTROL;
+        resp_pkt.payload.msg.type = VP_CONTROL_CONNECT_ACK;
+        resp_pkt.payload.msg.args.connect_ack.server_fd = server_fd;
+        resp_pkt.payload.msg.args.connect_ack.client_fd = client_fd;
+        resp_pkt.magic = VP_MAGIC;
+
+        /* TODO: can this potentially block or cause a deadlock with
+         * the remote end? need to look into potentially buffering these
+         * if it looks like the remote end is waiting for us to read data
+         * off the channel.
+         */
+        if (drv->channel_fd == -1) {
+            TRACE("channel no longer connected, ignoring packet");
+            return -1;
+        }
+
+        ret = vp_send_all(drv->channel_fd, (void *)&resp_pkt, sizeof(resp_pkt));
+        if (ret == -1) {
+            LOG("error sending data over channel");
+            return -1;
+        }
+        if (ret != sizeof(resp_pkt)) {
+            TRACE("buffer full? %d bytes remaining", ret);
+            return -1;
+        }
+
+        /* add new VPConn to list and set a read handler for it */
+        new_conn->drv = drv;
+        new_conn->client_fd = client_fd;
+        new_conn->server_fd = server_fd;
+        new_conn->type = VP_CONN_SERVER;
+        new_conn->state = VP_STATE_CONNECTED;
+        QLIST_INSERT_HEAD(&drv->conns, new_conn, next);
+        vp_set_fd_handler(server_fd, vp_conn_read, NULL, new_conn);
+
+        break;
+    }
+    case VP_CONTROL_CONNECT_ACK: {
+        int client_fd = msg->args.connect_ack.client_fd;
+        int server_fd = msg->args.connect_ack.server_fd;
+        VPConn *conn;
+
+        TRACE("recieved ack from remote end for client fd %d", client_fd);
+
+        if (server_fd <= 0) {
+            LOG("remote end sent invalid server fd");
+            return -1;
+        }
+
+        conn = get_conn(drv, client_fd, true);
+
+        if (conn == NULL) {
+            LOG("failed to find connection with client_fd %d", client_fd);
+            return -1;
+        }
+
+        conn->server_fd = server_fd;
+        conn->state = VP_STATE_CONNECTED;
+        vp_set_fd_handler(client_fd, vp_conn_read, NULL, conn);
+
+        break;
+    }
+    case VP_CONTROL_CLOSE: {
+        int fd;
+        VPConn *conn;
+
+        TRACE("closing connection on behalf of remote end");
+
+        if (msg->args.close.client_fd >= 0) {
+            fd = msg->args.close.client_fd;
+            TRACE("recieved close msg from remote end for client fd %d", fd);
+            conn = get_conn(drv, fd, true);
+        } else if (msg->args.close.server_fd >= 0) {
+            fd = msg->args.close.server_fd;
+            TRACE("recieved close msg from remote end for server fd %d", fd);
+            conn = get_conn(drv, fd, false);
+        } else {
+            LOG("invalid fd");
+            return -1;
+        }
+
+        if (conn == NULL) {
+            LOG("failed to find conn with specified fd %d", fd);
+            return -1;
+        }
+
+        closesocket(fd);
+        vp_set_fd_handler(fd, NULL, NULL, conn);
+        QLIST_REMOVE(conn, next);
+        qemu_free(conn);
+        break;
+    }
+    }
+    return 0;
+}
+
 /* handle data packets
  *
  * process VPPackets containing data and send them to the corresponding
-- 
1.7.0.4

[Qemu-devel] [RFC][RESEND][PATCH v1 11/15] virtproxy: add vp_handle_packet()

[Michael Roth]

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 virtproxy.c |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/virtproxy.c b/virtproxy.c
index 4f56aba..5ec4e77 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -431,6 +431,29 @@ static int vp_handle_data_packet(void *drv, const VPPacket *pkt)
     return 0;
 }
 
+static inline int vp_handle_packet(VPDriver *drv, const VPPacket *pkt)
+{
+    int ret;
+
+    TRACE("called with drv: %p", drv);
+
+    if (pkt->magic != VP_MAGIC) {
+        LOG("invalid packet magic field");
+        return -1;
+    }
+
+    if (pkt->type == VP_PKT_CONTROL) {
+        ret = vp_handle_control_packet(drv, pkt);
+    } else if (pkt->type == VP_PKT_CLIENT || pkt->type == VP_PKT_SERVER) {
+        ret = vp_handle_data_packet(drv, pkt);
+    } else {
+        LOG("invalid packet type");
+        return -1;
+    }
+
+    return ret;
+}
+
 /* read handler for communication channel
  *
  * de-multiplexes data coming in over the channel. for control messages
-- 
1.7.0.4

[Qemu-devel] [RFC][RESEND][PATCH v1 12/15] virtproxy: interfaces to set/remove VPIForwards

[Michael Roth]

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 virtproxy.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 virtproxy.h |    2 ++
 2 files changed, 61 insertions(+), 0 deletions(-)

diff --git a/virtproxy.c b/virtproxy.c
index 5ec4e77..86a8e5b 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -658,3 +658,62 @@ int vp_set_oforward(VPDriver *drv, int fd, const char *service_id)
 
     return 0;
 }
+
+/* add/modify a service_id -> net/unix socket mapping
+ *
+ * "service_id" is a user-defined id for the service. this is what the
+ * remote end will use to proxy connections to a specific service on
+ * our end.
+ *
+ * if "port" is NULL, "addr" is the address of the net socket the
+ * service is running on. otherwise, addr is the path to the unix socket
+ * the service is running on.
+ *
+ * if "port" AND "addr" are NULL, find and remove the current iforward
+ * for this "service_id" if it exists.
+ *
+ * "ipv6" is a bool denoting whether or not to use ipv6
+ */
+int vp_set_iforward(VPDriver *drv, const char *service_id, const char *addr,
+                    const char *port, bool ipv6)
+{
+    VPIForward *f = get_iforward(drv, service_id);
+
+    if (addr == NULL && port == NULL) {
+        if (f != NULL) {
+            qemu_opts_del(f->socket_opts);
+            QLIST_REMOVE(f, next);
+            qemu_free(f);
+        }
+        return 0;
+    }
+
+    if (f == NULL) {
+        f = qemu_mallocz(sizeof(VPIForward));
+        f->drv = drv;
+        strncpy(f->service_id, service_id, VP_SERVICE_ID_LEN);
+        QLIST_INSERT_HEAD(&drv->iforwards, f, next);
+    } else {
+        qemu_opts_del(f->socket_opts);
+    }
+
+    /* stick socket-related options in a QemuOpts so we can
+     * utilize qemu socket utility functions directly
+     */
+    f->socket_opts = qemu_opts_create(&vp_socket_opts, NULL, 0);
+    if (port == NULL) {
+        /* no port given, assume unix path */
+        qemu_opt_set(f->socket_opts, "path", addr);
+    } else {
+        qemu_opt_set(f->socket_opts, "host", addr);
+        qemu_opt_set(f->socket_opts, "port", port);
+    }
+
+    if (ipv6) {
+        qemu_opt_set(f->socket_opts, "ipv6", "on");
+    } else {
+        qemu_opt_set(f->socket_opts, "ipv4", "on");
+    }
+
+    return 0;
+}
diff --git a/virtproxy.h b/virtproxy.h
index 39d5d40..d2522b3 100644
--- a/virtproxy.h
+++ b/virtproxy.h
@@ -34,5 +34,7 @@ int vp_set_fd_handler(int fd,
 /* virtproxy interface */
 VPDriver *vp_new(int fd, bool listen);
 int vp_set_oforward(VPDriver *drv, int fd, const char *service_id);
+int vp_set_iforward(VPDriver *drv, const char *service_id, const char *addr,
+                    const char *port, bool ipv6);
 
 #endif /* VIRTPROXY_H */
-- 
1.7.0.4

[Qemu-devel] [RFC][RESEND][PATCH v1 13/15] virtproxy: add read handler for proxied connections

[Michael Roth]

reads data from client/server connections as they become readable, then
sends the data over the channel

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 virtproxy.c |   80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 80 insertions(+), 0 deletions(-)

diff --git a/virtproxy.c b/virtproxy.c
index 86a8e5b..f3f7f46 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -200,6 +200,86 @@ static VPIForward *get_iforward(const VPDriver *drv, const char *service_id)
     return NULL;
 }
 
+/* read handler for proxied connections */
+static void vp_conn_read(void *opaque)
+{
+    VPConn *conn = opaque;
+    VPDriver *drv = conn->drv;
+    VPPacket pkt;
+    char buf[VP_CONN_DATA_LEN];
+    int fd, count, ret;
+    bool client;
+
+    TRACE("called with opaque: %p, drv: %p", opaque, drv);
+
+    if (conn->state != VP_STATE_CONNECTED) {
+        LOG("invalid connection state");
+        return;
+    }
+
+    if (conn->type != VP_CONN_CLIENT && conn->type != VP_CONN_SERVER) {
+        LOG("invalid connection type");
+        return;
+    }
+
+    /* TODO: all fields should be explicitly set so we shouldn't
+     * need to memset. this might hurt if we beef up VPPacket size
+     */
+    memset(&pkt, 0, sizeof(VPPacket));
+    pkt.magic = VP_MAGIC;
+
+    if (conn->type == VP_CONN_CLIENT) {
+        client = true;
+        fd = conn->client_fd;
+    } else {
+        client = false;
+        fd = conn->server_fd;
+    }
+
+    count = read(fd, buf, VP_CONN_DATA_LEN);
+    if (count == -1) {
+        LOG("read() failed: %s", strerror(errno));
+        return;
+    } else if (count == 0) {
+        /* connection closed, tell remote end to clean up */
+        TRACE("connection closed");
+        pkt.type = VP_PKT_CONTROL;
+        pkt.payload.msg.type = VP_CONTROL_CLOSE;
+        if (client) {
+            /* we're closing the client, have remote close the server conn */
+            TRACE("closing connection for client fd %d", conn->client_fd);
+            pkt.payload.msg.args.close.client_fd = -1;
+            pkt.payload.msg.args.close.server_fd = conn->server_fd;
+        } else {
+            TRACE("closing connection for server fd %d", conn->server_fd);
+            pkt.payload.msg.args.close.server_fd = -1;
+            pkt.payload.msg.args.close.client_fd = conn->client_fd;;
+        }
+        /* clean up things on our end */
+        closesocket(fd);
+        vp_set_fd_handler(fd, NULL, NULL, NULL);
+        QLIST_REMOVE(conn, next);
+        qemu_free(conn);
+    } else {
+        TRACE("data read");
+        pkt.type = client ? VP_PKT_CLIENT : VP_PKT_SERVER;
+        pkt.payload.proxied.client_fd = conn->client_fd;
+        pkt.payload.proxied.server_fd = conn->server_fd;
+        memcpy(pkt.payload.proxied.data, buf, count);
+        pkt.payload.proxied.bytes = count;
+    }
+
+    ret = vp_send_all(drv->channel_fd, (void *)&pkt, sizeof(VPPacket));
+    if (ret == -1) {
+        LOG("error sending data over channel");
+        return;
+    }
+    if (ret != sizeof(VPPacket)) {
+        TRACE("buffer full?");
+        return;
+    }
+}
+
 /* accept handler for communication channel
  *
  * accept()s connection to communication channel (for sockets), and sets
-- 
1.7.0.4

[Qemu-devel] [RFC][RESEND][PATCH v1 14/15] virtproxy: Makefile/configure changes to build qemu-vp

[Michael Roth]

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 .gitignore |    1 +
 Makefile   |    4 +++-
 configure  |    1 +
 3 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/.gitignore b/.gitignore
index a43e4d1..da307d2 100644
--- a/.gitignore
+++ b/.gitignore
@@ -31,6 +31,7 @@ qemu-img-cmds.texi
 qemu-img-cmds.h
 qemu-io
 qemu-monitor.texi
+qemu-vp
 QMP/qmp-commands.txt
 .gdbinit
 *.a
diff --git a/Makefile b/Makefile
index 252c817..53b58d2 100644
--- a/Makefile
+++ b/Makefile
@@ -127,7 +127,7 @@ version-obj-$(CONFIG_WIN32) += version.o
 ######################################################################
 
 qemu-img.o: qemu-img-cmds.h
-qemu-img.o qemu-tool.o qemu-nbd.o qemu-io.o: $(GENERATED_HEADERS)
+qemu-img.o qemu-tool.o qemu-nbd.o qemu-io.o virtproxy.o: $(GENERATED_HEADERS)
 
 qemu-img$(EXESUF): qemu-img.o qemu-tool.o qemu-error.o $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y)
 
@@ -135,6 +135,8 @@ qemu-nbd$(EXESUF): qemu-nbd.o qemu-tool.o qemu-error.o $(trace-obj-y) $(block-ob
 
 qemu-io$(EXESUF): qemu-io.o cmd.o qemu-tool.o qemu-error.o $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y)
 
+qemu-vp$(EXESUF): qemu-vp.o virtproxy.o qemu-tool.o qemu-error.o qemu-sockets.c $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y)
+
 qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx
 	$(call quiet-command,sh $(SRC_PATH)/hxtool -h < $< > $@,"  GEN   $@")
 
diff --git a/configure b/configure
index a079a49..27f92e0 100755
--- a/configure
+++ b/configure
@@ -2232,6 +2232,7 @@ if test "$softmmu" = yes ; then
   tools="qemu-img\$(EXESUF) qemu-io\$(EXESUF) $tools"
   if [ "$linux" = "yes" -o "$bsd" = "yes" -o "$solaris" = "yes" ] ; then
       tools="qemu-nbd\$(EXESUF) $tools"
+      tools="qemu-vp\$(EXESUF) $tools"
     if [ "$check_utests" = "yes" ]; then
       tools="check-qint check-qstring check-qdict check-qlist $tools"
       tools="check-qfloat check-qjson $tools"
-- 
1.7.0.4

[Qemu-devel] [RFC][RESEND][PATCH v1 15/15] virtproxy: qemu-vp, main logic

[Michael Roth]

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 Makefile  |    2 +-
 qemu-vp.c |  469 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 469 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 53b58d2..2dd64a3 100644
--- a/Makefile
+++ b/Makefile
@@ -135,7 +135,7 @@ qemu-nbd$(EXESUF): qemu-nbd.o qemu-tool.o qemu-error.o $(trace-obj-y) $(block-ob
 
 qemu-io$(EXESUF): qemu-io.o cmd.o qemu-tool.o qemu-error.o $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y)
 
-qemu-vp$(EXESUF): qemu-vp.o virtproxy.o qemu-tool.o qemu-error.o qemu-sockets.c $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y)
+qemu-vp$(EXESUF): qemu-vp.o virtproxy.o qemu-tool.o qemu-error.o qemu-sockets.c $(block-obj-y) $(qobject-obj-y)
 
 qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx
 	$(call quiet-command,sh $(SRC_PATH)/hxtool -h < $< > $@,"  GEN   $@")
diff --git a/qemu-vp.c b/qemu-vp.c
index 5075cdc..0cc0e67 100644
--- a/qemu-vp.c
+++ b/qemu-vp.c
@@ -9,10 +9,54 @@
  * This work is licensed under the terms of the GNU GPL, version 2 or later.
  * See the COPYING file in the top-level directory.
  *
+ * QEMU System Emulator
+ *
+ * Copyright (c) 2003-2008 Fabrice Bellard
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
  */
 
+#include <getopt.h>
+#include <err.h>
+#include "qemu-option.h"
+#include "qemu_socket.h"
 #include "virtproxy.h"
 
+static bool verbose_enabled = 0;
+#define DEBUG_ENABLED
+
+#ifdef DEBUG_ENABLED
+#define DEBUG(msg, ...) do { \
+    fprintf(stderr, "%s:%s():L%d: " msg "\n", \
+            __FILE__, __FUNCTION__, __LINE__, ## __VA_ARGS__); \
+} while(0)
+#else
+#define DEBUG(msg, ...) do {} while (0)
+#endif
+
+#define INFO(msg, ...) do { \
+    if (!verbose_enabled) { \
+        break; \
+    } \
+    warnx(msg, ## __VA_ARGS__); \
+} while(0)
+
 /* mirror qemu I/O-related code for standalone daemon */
 typedef struct IOHandlerRecord {
     int fd;
@@ -98,7 +142,7 @@ static void main_loop_wait(int nonblocking)
     fd_set rfds, wfds, xfds;
     int ret, nfds;
     struct timeval tv;
-    int timeout = 1000;
+    int timeout = 100000;
 
     if (nonblocking) {
         timeout = 0;
@@ -149,3 +193,426 @@ static void main_loop_wait(int nonblocking)
         }
     }
 }
+
+#define VP_ARG_LEN 256
+
+static QemuOptsList vp_opts = {
+    .name = "vpargs",
+    .head = QTAILQ_HEAD_INITIALIZER(vp_opts.head),
+    .desc = {
+        {
+            .name = "service_id",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "channel_method",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "index",
+            .type = QEMU_OPT_NUMBER,
+        },{
+            .name = "path",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "host",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "port",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "ipv4",
+            .type = QEMU_OPT_BOOL,
+        },{
+            .name = "ipv6",
+            .type = QEMU_OPT_BOOL,
+        },
+        { /* end if list */ }
+    },
+};
+
+typedef struct VPData {
+    QemuOpts *opts;
+    void *opaque;
+    QTAILQ_ENTRY(VPData) next;
+} VPData;
+
+static QTAILQ_HEAD(, VPData) iforwards;
+static QTAILQ_HEAD(, VPData) oforwards;
+static QTAILQ_HEAD(, VPData) channels;
+
+static void usage(const char *cmd)
+{
+    printf(
+"Usage: %s -c <channel_opts> [-c ... ] [-i <iforward_opts> ...] "
+"[-o <oforward_opts> ...]\n"
+"QEMU virt-proxy communication channel\n"
+"\n"
+"  -c, --channel    channel options of the form:\n"
+"                   <method>:<addr>:<port>[:channel_id]\n"
+"  -o, --oforward   oforward options of the form:\n"
+"                   <service_id>:<addr>:<port>[:channel_id]\n"
+"  -i, --iforward   iforward options of the form:\n"
+"                   <service_id>:<addr>:<port>[:channel_id]\n"
+"  -v, --verbose    display extra debugging information\n"
+"  -h, --help       display this help and exit\n"
+"\n"
+"  channels are used to establish a data connection between 2 end-points in\n"
+"  the host or the guest (connection method specified by <method>).\n"
+"  oforwards specify a socket to listen for new connections on, outgoing\n"
+"  data from which is tagged with <service_id> before being sent over the\n"
+"  channel. iforwards specify a socket to route incoming data/connections\n"
+"  with a specific <service_id> to. The positional parameters for\n"
+"  channels/iforwards/oforwards are:\n"
+"\n"
+"  <method>:     one of unix-connect, unix-listen, tcp-connect, tcp-listen,\n"
+"                virtserial-open\n"
+"  <addr>:       path of unix socket or virtserial port, or IP of host, to\n"
+"                connect/bind to\n"
+"  <port>:       port to bind/connect to, or '-' if addr is a path\n"
+"  <service_id>: an identifier used to properly route connections to the\n"
+"                corresponding host or guest daemon socket.\n"
+"  <channel_id>: numerical id to identify what channel to use for an iforward\n"
+"                or oforward. (default is 0)\n"
+"\n"
+"Report bugs to <mdroth@linux.vnet.ibm.com>\n"
+    , cmd);
+}
+
+static int vp_parse(QemuOpts *opts, const char *str, bool is_channel)
+{
+    /* TODO: use VP_SERVICE_ID_LEN, bring it into virtproxy.h */
+    char service_id[32];
+    char channel_method[32];
+    char index[10];
+    char *addr;
+    char port[33];
+    int pos, ret;
+
+    if (is_channel == false) {
+        /* parse service id */
+        ret = sscanf(str,"%32[^:]:%n",service_id,&pos);
+        if (ret != 1) {
+            warn("error parsing service id");
+            return -1;
+        }
+        qemu_opt_set(opts, "service_id", service_id);
+    } else {
+        /* parse connection type */
+        ret = sscanf(str,"%32[^:]:%n",channel_method,&pos);
+        if (ret != 1) {
+            warn("error parsing channel method");
+            return -1;
+        }
+        qemu_opt_set(opts, "channel_method", channel_method);
+    }
+    str += pos;
+    pos = 0;
+
+    /* parse path/addr and port */
+    if (str[0] == '[') {
+        /* ipv6 formatted */
+        ret = sscanf(str,"[%a[^]:]]:%32[^:]%n",&addr,port,&pos);
+        qemu_opt_set(opts, "ipv6", "on");
+    } else {
+        ret = sscanf(str,"%a[^:]:%32[^:]%n",&addr,port,&pos);
+        qemu_opt_set(opts, "ipv4", "on");
+    }
+
+    if (ret != 2) {
+        warnx("error parsing path/addr/port");
+        return -1;
+    } else if (port[0] == '-') {
+        /* no port given, assume unix path */
+        qemu_opt_set(opts, "path", addr);
+    } else {
+        qemu_opt_set(opts, "host", addr);
+        qemu_opt_set(opts, "port", port);
+        qemu_free(addr);
+    }
+    str += pos;
+    pos = 0;
+
+    if (str[0] == ':') {
+        /* parse optional index parameter */
+        ret = sscanf(str,":%10[^:]%n",index,&pos);
+    } else {
+        qemu_opt_set(opts, "index", "0");
+        return 0;
+    }
+
+    if (ret != 1) {
+        warnx("error parsing index");
+        return -1;
+    } else {
+        qemu_opt_set(opts, "index", index);
+    }
+    str += pos;
+    pos = 0;
+
+    return 0;
+}
+
+static VPDriver *get_channel_drv(int index) {
+    VPData *data;
+    VPDriver *drv;
+    int cindex;
+
+    QTAILQ_FOREACH(data, &channels, next) {
+        cindex = qemu_opt_get_number(data->opts, "index", 0);
+        if (cindex == index) {
+            drv = data->opaque;
+            return drv;
+        }
+    }
+
+    return NULL;
+}
+
+static int init_channels(void) {
+    VPDriver *drv;
+    VPData *channel_data;
+    const char *channel_method, *path;
+    int fd, ret;
+    bool listen;
+
+    if (QTAILQ_EMPTY(&channels)) {
+        warnx("no channel specified");
+        return -1;
+    }
+
+    channel_data = QTAILQ_FIRST(&channels);
+
+    /* TODO: add this support, optional idx param for -i/-o/-c
+     * args should suffice
+     */
+    if (QTAILQ_NEXT(channel_data, next) != NULL) {
+        warnx("multiple channels not currently supported, defaulting to first");
+    }
+
+    INFO("initializing channel...");
+    if (verbose_enabled) {
+        qemu_opts_print(channel_data->opts, NULL);
+    }
+
+    channel_method = qemu_opt_get(channel_data->opts, "channel_method");
+
+    if (strcmp(channel_method, "tcp-listen") == 0) {
+        fd = inet_listen_opts(channel_data->opts, 0);
+        listen = true;
+    } else if (strcmp(channel_method, "tcp-connect") == 0) {
+        fd = inet_connect_opts(channel_data->opts);
+        listen = false;
+    } else if (strcmp(channel_method, "unix-listen") == 0) {
+        fd = unix_listen_opts(channel_data->opts);
+        listen = true;
+    } else if (strcmp(channel_method, "unix-connect") == 0) {
+        fd = unix_connect_opts(channel_data->opts);
+        listen = false;
+    } else if (strcmp(channel_method, "virtserial-open") == 0) {
+        path = qemu_opt_get(channel_data->opts, "path");
+        fd = qemu_open(path, O_RDWR);
+        ret = fcntl(fd, F_GETFL);
+        ret = fcntl(fd, F_SETFL, ret | O_ASYNC);
+        if (ret < 0) {
+            warn("error setting flags for fd");
+            return -1;
+        }
+        listen = false;
+    } else {
+        warnx("invalid channel type: %s", channel_method);
+        return -1;
+    }
+
+    if (fd == -1) {
+        warn("error opening connection");
+        return -1;
+    }
+
+    drv = vp_new(fd, listen);
+    channel_data->opaque = drv;
+
+    return 0;
+}
+
+static int init_oforwards(void) {
+    VPDriver *drv;
+    VPData *oforward_data;
+    int index, ret, fd;
+    const char *service_id;
+
+    QTAILQ_FOREACH(oforward_data, &oforwards, next) {
+        INFO("initializing oforward...");
+        if (verbose_enabled) {
+            qemu_opts_print(oforward_data->opts, NULL);
+        }
+
+        index = qemu_opt_get_number(oforward_data->opts, "index", 0);
+        drv = get_channel_drv(index);
+        if (drv == NULL) {
+            warnx("unable to find channel with index: %d", index);
+            return -1;
+        }
+
+        if (qemu_opt_get(oforward_data->opts, "host") != NULL) {
+            fd = inet_listen_opts(oforward_data->opts, 0);
+        } else if (qemu_opt_get(oforward_data->opts, "path") != NULL) {
+            fd = unix_listen_opts(oforward_data->opts);
+        } else {
+            warnx("unable to find listening socket host/addr info");
+            return -1;
+        }
+
+        if (fd == -1) {
+            warnx("failed to create FD");
+            return -1;
+        }
+
+        service_id = qemu_opt_get(oforward_data->opts, "service_id");
+
+        if (service_id == NULL) {
+            warnx("no service_id specified");
+            return -1;
+        }
+
+        ret = vp_set_oforward(drv, fd, service_id);
+    }
+
+    return 0;
+}
+
+static int init_iforwards(void) {
+    VPDriver *drv;
+    VPData *iforward_data;
+    int index, ret;
+    const char *service_id, *addr, *port;
+    bool ipv6;
+
+    QTAILQ_FOREACH(iforward_data, &iforwards, next) {
+        INFO("initializing iforward...");
+        if (verbose_enabled) {
+            qemu_opts_print(iforward_data->opts, NULL);
+        }
+
+        index = qemu_opt_get_number(iforward_data->opts, "index", 0);
+        drv = get_channel_drv(index);
+        if (drv == NULL) {
+            warnx("unable to find channel with index: %d", index);
+            return -1;
+        }
+
+        service_id = qemu_opt_get(iforward_data->opts, "service_id");
+        if (service_id == NULL) {
+            warnx("no service_id specified");
+            return -1;
+        }
+
+        addr = qemu_opt_get(iforward_data->opts, "path");
+        port = NULL;
+
+        if (addr == NULL) {
+            /* map service to a network socket instead */
+            addr = qemu_opt_get(iforward_data->opts, "host");
+            port = qemu_opt_get(iforward_data->opts, "port");
+        }
+
+        ipv6 = qemu_opt_get_bool(iforward_data->opts, "ipv6", 0) ?
+               true : false;
+
+        ret = vp_set_iforward(drv, service_id, addr, port, ipv6);
+        if (ret != 0) {
+            warnx("error adding iforward");
+            return -1;
+        }
+    }
+
+    return 0;
+}
+
+int main(int argc, char **argv)
+{
+    const char *sopt = "hVvi:o:c:";
+    struct option lopt[] = {
+        { "help", 0, NULL, 'h' },
+        { "version", 0, NULL, 'V' },
+        { "verbose", 0, NULL, 'v' },
+        { "iforward", 0, NULL, 'i' },
+        { "oforward", 0, NULL, 'o' },
+        { "channel", 0, NULL, 'c' },
+        { NULL, 0, NULL, 0 }
+    };
+    int opt_ind = 0, ch, ret;
+    QTAILQ_INIT(&iforwards);
+    QTAILQ_INIT(&oforwards);
+    QTAILQ_INIT(&channels);
+
+    while ((ch = getopt_long(argc, argv, sopt, lopt, &opt_ind)) != -1) {
+        QemuOpts *opts;
+        VPData *data;
+        switch (ch) {
+        case 'i':
+            opts = qemu_opts_create(&vp_opts, NULL, 0);
+            ret = vp_parse(opts, optarg, 0);
+            if (ret) {
+                errx(EXIT_FAILURE, "error parsing arg: %s", optarg);
+            }
+            data = qemu_mallocz(sizeof(VPData));
+            data->opts = opts;
+            QTAILQ_INSERT_TAIL(&iforwards, data, next);
+            break;
+        case 'o':
+            opts = qemu_opts_create(&vp_opts, NULL, 0);
+            ret = vp_parse(opts, optarg, 0);
+            if (ret) {
+                errx(EXIT_FAILURE, "error parsing arg: %s", optarg);
+            }
+            data = qemu_mallocz(sizeof(VPData));
+            data->opts = opts;
+            QTAILQ_INSERT_TAIL(&oforwards, data, next);
+            break;
+        case 'c':
+            opts = qemu_opts_create(&vp_opts, NULL, 0);
+            ret = vp_parse(opts, optarg, 1);
+            if (ret) {
+                errx(EXIT_FAILURE, "error parsing arg: %s", optarg);
+            }
+            data = qemu_mallocz(sizeof(VPData));
+            data->opts = opts;
+            QTAILQ_INSERT_TAIL(&channels, data, next);
+            break;
+        case 'v':
+            verbose_enabled = 1;
+            break;
+        case 'h':
+            usage(argv[0]);
+            return 0;
+        case '?':
+            errx(EXIT_FAILURE, "Try '%s --help' for more information.",
+                 argv[0]);
+        }
+    }
+
+    ret = init_channels();
+    if (ret) {
+        errx(EXIT_FAILURE, "error initializing communication channel");
+    }
+
+    ret = init_oforwards();
+    if (ret) {
+        errx(EXIT_FAILURE,
+             "error initializing forwarders for outgoing connections");
+    }
+
+    ret = init_iforwards();
+    if (ret) {
+        errx(EXIT_FAILURE,
+             "error initializing service mappings for incoming connections");
+    }
+
+    /* main i/o loop */
+    for (;;) {
+        DEBUG("entering main_loop_wait()");
+        main_loop_wait(0);
+        DEBUG("left main_loop_wait()");
+    }
+
+    return 0;
+}
-- 
1.7.0.4

[Qemu-devel] Re: [RFC][RESEND][PATCH v1 01/15] virtproxy: base data structures and constants

[Adam Litke]

On Wed, 2010-11-03 at 10:27 -0500, Michael Roth wrote:
> +static QemuOptsList vp_socket_opts = {
> +    .name = "vp_socket_opts",
> +    .head = QTAILQ_HEAD_INITIALIZER(vp_socket_opts.head),
> +    .desc = {
> +        {
> +            .name = "path",
> +            .type = QEMU_OPT_STRING,
> +        },{
> +            .name = "host",
> +            .type = QEMU_OPT_STRING,
> +        },{
> +            .name = "port",
> +            .type = QEMU_OPT_STRING,
> +        },{
> +            .name = "ipv4",
> +            .type = QEMU_OPT_BOOL,
> +        },{
> +            .name = "ipv6",
> +            .type = QEMU_OPT_BOOL,
> +        },
> +        { /* end if list */ }

end of list

> +/* wrappers for s/vp/qemu/ functions we need */
> +int vp_send_all(int fd, const void *buf, int len1);
> +int vp_set_fd_handler2(int fd,
> +                         IOCanReadHandler *fd_read_poll,
> +                         IOHandler *fd_read,
> +                         IOHandler *fd_write,
> +                         void *opaque);
> +int vp_set_fd_handler(int fd,
> +                        IOHandler *fd_read,
> +                        IOHandler *fd_write,
> +                        void *opaque);

Where are the corresponding definitions for these functions?  The
declarations should appear in that patch.

-- 
Thanks,
Adam

[Qemu-devel] Re: [RFC][RESEND][PATCH v1 02/15] virtproxy: qemu-vp, standalone daemon skeleton

[Adam Litke]

On Wed, 2010-11-03 at 10:27 -0500, Michael Roth wrote:
> +/* mirror qemu I/O-related code for standalone daemon */
> +typedef struct IOHandlerRecord {
> +    int fd;
> +    IOCanReadHandler *fd_read_poll;
> +    IOHandler *fd_read;
> +    IOHandler *fd_write;
> +    int deleted;
> +    void *opaque;
> +    /* temporary data */
> +    struct pollfd *ufd;
> +    QLIST_ENTRY(IOHandlerRecord) next;
> +} IOHandlerRecord;

As you say, this is exactly the same structure as defined in vl.c.  If
you copy and paste code for a new feature you should be looking for a
way to share it instead.  Why not create a separate patch that defines
this structure in vl.h which can then be included by vl.c and virtproxy
code.

> +static QLIST_HEAD(, IOHandlerRecord) io_handlers =
> +    QLIST_HEAD_INITIALIZER(io_handlers);
> +
> +int vp_set_fd_handler2(int fd,
> +                         IOCanReadHandler *fd_read_poll,
> +                         IOHandler *fd_read,
> +                         IOHandler *fd_write,
> +                         void *opaque)
> +{
> +    IOHandlerRecord *ioh;
> +
> +    if (!fd_read && !fd_write) {
> +        QLIST_FOREACH(ioh, &io_handlers, next) {
> +            if (ioh->fd == fd) {
> +                ioh->deleted = 1;
> +                break;
> +            }
> +        }
> +    } else {
> +        QLIST_FOREACH(ioh, &io_handlers, next) {
> +            if (ioh->fd == fd)
> +                goto found;
> +        }
> +        ioh = qemu_mallocz(sizeof(IOHandlerRecord));
> +        QLIST_INSERT_HEAD(&io_handlers, ioh, next);
> +    found:
> +        ioh->fd = fd;
> +        ioh->fd_read_poll = fd_read_poll;
> +        ioh->fd_read = fd_read;
> +        ioh->fd_write = fd_write;
> +        ioh->opaque = opaque;
> +        ioh->deleted = 0;
> +    }
> +    return 0;
> +}

Copied from vl.c -- export it instead.

> +int vp_set_fd_handler(int fd,
> +                        IOHandler *fd_read,
> +                        IOHandler *fd_write,
> +                        void *opaque)
> +{
> +    return vp_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
> +}
> +
> +int vp_send_all(int fd, const void *buf, int len1)
> +{
> +    int ret, len;
> +
> +    len = len1;
> +    while (len > 0) {
> +        ret = write(fd, buf, len);
> +        if (ret < 0) {
> +            if (errno != EINTR && errno != EAGAIN) {
> +                warn("write() failed");
> +                return -1;
> +            }
> +        } else if (ret == 0) {
> +            break;
> +        } else {
> +            buf += ret;
> +            len -= ret;
> +        }
> +    }
> +    return len1 - len;
> +}

Copied from qemu-char.c -- Share please.

> +static void main_loop_wait(int nonblocking)
> +{
> +    IOHandlerRecord *ioh;
> +    fd_set rfds, wfds, xfds;
> +    int ret, nfds;
> +    struct timeval tv;
> +    int timeout = 1000;
> +
> +    if (nonblocking) {
> +        timeout = 0;
> +    }
> +
> +    /* poll any events */
> +    nfds = -1;
> +    FD_ZERO(&rfds);
> +    FD_ZERO(&wfds);
> +    FD_ZERO(&xfds);
> +    QLIST_FOREACH(ioh, &io_handlers, next) {
> +        if (ioh->deleted)
> +            continue;
> +        if (ioh->fd_read &&
> +            (!ioh->fd_read_poll ||
> +             ioh->fd_read_poll(ioh->opaque) != 0)) {
> +            FD_SET(ioh->fd, &rfds);
> +            if (ioh->fd > nfds)
> +                nfds = ioh->fd;
> +        }
> +        if (ioh->fd_write) {
> +            FD_SET(ioh->fd, &wfds);
> +            if (ioh->fd > nfds)
> +                nfds = ioh->fd;
> +        }
> +    }
> +
> +    tv.tv_sec = timeout / 1000;
> +    tv.tv_usec = (timeout % 1000) * 1000;
> +
> +    ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
> +
> +    if (ret > 0) {
> +        IOHandlerRecord *pioh;
> +
> +        QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) {
> +            if (ioh->deleted) {
> +                QLIST_REMOVE(ioh, next);
> +                qemu_free(ioh);
> +                continue;
> +            }
> +            if (ioh->fd_read && FD_ISSET(ioh->fd, &rfds)) {
> +                ioh->fd_read(ioh->opaque);
> +            }
> +            if (ioh->fd_write && FD_ISSET(ioh->fd, &wfds)) {
> +                ioh->fd_write(ioh->opaque);
> +            }
> +        }
> +    }
> +

This resembles vl.c's main_loop_wait() but I can see why you might want
your own.  There is opportunity for sharing the select logic and ioh
callbacks but I think that could be addressed later.

-- 
Thanks,
Adam

[Qemu-devel] Re: [RFC][RESEND][PATCH v1 03/15] virtproxy: add debug functions for virtproxy core

[Adam Litke]

Alas, this code exists in several other places too... I guess you can't
be responsible for cleaning up all of qemu :)

On Wed, 2010-11-03 at 10:27 -0500, Michael Roth wrote:
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  virtproxy.c |   17 +++++++++++++++++
>  1 files changed, 17 insertions(+), 0 deletions(-)
> 
> diff --git a/virtproxy.c b/virtproxy.c
> index f30b859..2f8996c 100644
> --- a/virtproxy.c
> +++ b/virtproxy.c
> @@ -13,6 +13,23 @@
> 
>  #include "virtproxy.h"
> 
> +#define DEBUG_VP
> +
> +#ifdef DEBUG_VP
> +#define TRACE(msg, ...) do { \
> +    fprintf(stderr, "%s:%s():L%d: " msg "\n", \
> +            __FILE__, __FUNCTION__, __LINE__, ## __VA_ARGS__); \
> +} while(0)
> +#else
> +#define TRACE(msg, ...) \
> +    do { } while (0)
> +#endif
> +
> +#define LOG(msg, ...) do { \
> +    fprintf(stderr, "%s:%s(): " msg "\n", \
> +            __FILE__, __FUNCTION__, ## __VA_ARGS__); \
> +} while(0)

-- 
Thanks,
Adam

[Qemu-devel] Re: [RFC][RESEND][PATCH v1 04/15] virtproxy: list look-up functions conns/oforwards/iforwards

[Adam Litke]

You should describe your changes a little bit more on the top here.
Looks good otherwise.

On Wed, 2010-11-03 at 10:27 -0500, Michael Roth wrote:
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  virtproxy.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 44 insertions(+), 0 deletions(-)
> 
> diff --git a/virtproxy.c b/virtproxy.c
> index 2f8996c..fa17722 100644
> --- a/virtproxy.c
> +++ b/virtproxy.c
> @@ -149,3 +149,47 @@ static QemuOptsList vp_socket_opts = {
>          { /* end if list */ }
>      },
>  };
> +
> +/* get VPConn by fd, "client" denotes whether to look for client or server */
> +static VPConn *get_conn(const VPDriver *drv, int fd, bool client)
> +{
> +    VPConn *c = NULL;
> +    int cur_fd;
> +
> +    QLIST_FOREACH(c, &drv->conns, next) {
> +        cur_fd = client ? c->client_fd : c->server_fd;
> +        if (cur_fd == fd) {
> +            return c;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +/* get VPOForward by service_id */
> +static VPOForward *get_oforward(const VPDriver *drv, const char *service_id)
> +{
> +    VPOForward *f = NULL;
> +
> +    QLIST_FOREACH(f, &drv->oforwards, next) {
> +        if (strncmp(f->service_id, service_id, VP_SERVICE_ID_LEN) == 0) {
> +            return f;
> +        }
> +    }
> +
> +    return NULL;
> +}
> +
> +/* get VPIForward by service_id */
> +static VPIForward *get_iforward(const VPDriver *drv, const char *service_id)
> +{
> +    VPIForward *f = NULL;
> +
> +    QLIST_FOREACH(f, &drv->iforwards, next) {
> +        if (strncmp(f->service_id, service_id, VP_SERVICE_ID_LEN) == 0) {
> +            return f;
> +        }
> +    }
> +
> +    return NULL;
> +}

-- 
Thanks,
Adam

[Qemu-devel] Re: [RFC][RESEND][PATCH v1 05/15] virtproxy: add accept handler for communication channel

[Adam Litke]

On Wed, 2010-11-03 at 10:28 -0500, Michael Roth wrote:
> This accept()'s connections to the socket we told virt-proxy to listen
> for the channel connection on and sets the appropriate read handler for
> the resulting FD.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  virtproxy.c |   37 +++++++++++++++++++++++++++++++++++++
>  1 files changed, 37 insertions(+), 0 deletions(-)
> 
> diff --git a/virtproxy.c b/virtproxy.c
> index fa17722..20532c2 100644
> --- a/virtproxy.c
> +++ b/virtproxy.c
> @@ -166,6 +166,8 @@ static VPConn *get_conn(const VPDriver *drv, int fd, bool client)
>      return NULL;
>  }
> 
> +static void vp_channel_accept(void *opaque);
> +

Why the forward declaration?  Can you move the function to a different
place in the file to avoid this?


> +/* accept handler for communication channel
> + *
> + * accept()s connection to communication channel (for sockets), and sets
> + * up the read handler for resulting FD.
> + */
> +static void vp_channel_accept(void *opaque)
> +{
> +    VPDriver *drv = opaque;
> +    struct sockaddr_in saddr;
> +    struct sockaddr *addr;
> +    socklen_t len;
> +    int fd;
> +
> +    TRACE("called with opaque: %p", drv);
> +
> +    for(;;) {
> +        len = sizeof(saddr);
> +        addr = (struct sockaddr *)&saddr;
> +        fd = qemu_accept(drv->listen_fd, addr, &len);
> +
> +        if (fd < 0 && errno != EINTR) {
> +            TRACE("accept() failed");
> +            return;
> +        } else if (fd >= 0) {
> +            TRACE("accepted connection");
> +            break;
> +        }
> +    }
> +
> +    drv->channel_fd = fd;
> +    vp_set_fd_handler(drv->channel_fd, vp_channel_read, NULL, drv);
> +    /* dont accept anymore connections until channel_fd is closed */
> +    vp_set_fd_handler(drv->listen_fd, NULL, NULL, NULL);
> +}

-- 
Thanks,
Adam

[Qemu-devel] Re: [RFC][RESEND][PATCH v1 06/15] virtproxy: add read handler for communication channel

[Adam Litke]

On Wed, 2010-11-03 at 10:28 -0500, Michael Roth wrote:
> Handle data coming in over the channel as VPPackets: Process control
> messages and forward data from remote client/server connections to the
> appropriate server/client FD on our end.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  virtproxy.c |   83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 83 insertions(+), 0 deletions(-)
> 
> diff --git a/virtproxy.c b/virtproxy.c
> index 20532c2..c9c3022 100644
> --- a/virtproxy.c
> +++ b/virtproxy.c
> @@ -33,6 +33,7 @@
>  #define VP_SERVICE_ID_LEN 32    /* max length of service id string */
>  #define VP_PKT_DATA_LEN 1024    /* max proxied bytes per VPPacket */
>  #define VP_CONN_DATA_LEN 1024   /* max bytes conns can send at a time */
> +#define VP_CHAN_DATA_LEN 4096   /* max bytes channel can send at a time */
>  #define VP_MAGIC 0x1F374059
> 
>  /* listening fd, one for each service we're forwarding to remote end */
> @@ -150,6 +151,8 @@ static QemuOptsList vp_socket_opts = {
>      },
>  };
> 
> +static void vp_channel_read(void *opaque);

Try to get rid of these forward declarations.  If you really need them,
consider adding a virtproxy-internal.h.


> @@ -230,3 +233,83 @@ static void vp_channel_accept(void *opaque)
>      /* dont accept anymore connections until channel_fd is closed */
>      vp_set_fd_handler(drv->listen_fd, NULL, NULL, NULL);
>  }
> +
> +/* read handler for communication channel
> + *
> + * de-multiplexes data coming in over the channel. for control messages
> + * we process them here, for data destined for a service or client we
> + * send it to the appropriate FD.
> + */
> +static void vp_channel_read(void *opaque)
> +{
> +    VPDriver *drv = opaque;
> +    VPPacket pkt;
> +    int count, ret, buf_offset;
> +    char buf[VP_CHAN_DATA_LEN];
> +    char *pkt_ptr, *buf_ptr;
> +
> +    TRACE("called with opaque: %p", drv);
> +
> +    count = read(drv->channel_fd, buf, sizeof(buf));
> +
> +    if (count == -1) {
> +        LOG("read() failed: %s", strerror(errno));
> +        return;
> +    } else if (count == 0) {
> +        /* TODO: channel closed, this probably shouldn't happen for guest-side
> +         * serial/virtio-serial connections, but need to confirm and consider
> +         * what should happen in this case. as it stands this virtproxy instance
> +         * is basically defunct at this point, same goes for "client" instances
> +         * of virtproxy where the remote end has hung-up.
> +         */
> +        LOG("channel connection closed");
> +        vp_set_fd_handler(drv->channel_fd, NULL, NULL, drv);
> +        drv->channel_fd = -1;
> +        if (drv->listen_fd) {
> +            vp_set_fd_handler(drv->listen_fd, vp_channel_accept, NULL, drv);
> +        }
> +        /* TODO: should close/remove/delete all existing VPConns here */

Looks like you have a little work TODO here still.  Perhaps you could
add some reset/init functions that would make handling this easier.  The
ability to reset state at both the channel and VPConn levels should give
you the ability to handle errors more gracefully in other places where
you currently just log them.

> +    }
> +
> +    if (drv->buflen + count >= sizeof(VPPacket)) {
> +        TRACE("initial packet, drv->buflen: %d", drv->buflen);
> +        pkt_ptr = (char *)&pkt;
> +        memcpy(pkt_ptr, drv->buf, drv->buflen);

Can drv->buflen ever be > sizeof(VPPacket) ?  You might consider adding
an assert(drv->buflen < sizeof(VPPacket)) unless it's mathematically
impossible.

> +        pkt_ptr += drv->buflen;
> +        memcpy(pkt_ptr, buf, sizeof(VPPacket) - drv->buflen);
> +        /* handle first packet */
> +        ret = vp_handle_packet(drv, &pkt);
> +        if (ret != 0) {
> +            LOG("error handling packet");
> +        }
> +        /* handle the rest of the buffer */
> +        buf_offset = sizeof(VPPacket) - drv->buflen;
> +        drv->buflen = 0;
> +        buf_ptr = buf + buf_offset;
> +        count -= buf_offset;
> +        while (count > 0) {
> +            if (count >= sizeof(VPPacket)) {
> +                /* handle full packet */
> +                TRACE("additional packet, drv->buflen: %d", drv->buflen);
> +                memcpy((void *)&pkt, buf_ptr, sizeof(VPPacket));
> +                ret = vp_handle_packet(drv, &pkt);
> +                if (ret != 0) {
> +                    LOG("error handling packet");
> +                }
> +                count -= sizeof(VPPacket);
> +                buf_ptr += sizeof(VPPacket);
> +            } else {
> +                /* buffer the remainder */
> +                TRACE("buffering packet");
> +                memcpy(drv->buf, buf_ptr, count);
> +                drv->buflen = count;
> +                break;
> +            }
> +        }
> +    } else {
> +        /* haven't got a full VPPacket yet, buffer for later */
> +        buf_ptr = drv->buf + drv->buflen;
> +        memcpy(buf_ptr, buf, count);
> +        drv->buflen += count;

With this algorithm you can hold some data hostage indefinitely.  You
either need to send out smaller packets of whatever data you receive or
maybe have a timer that periodically fires to flush drv->buf.  Any
thoughts on what you plan to do here?

-- 
Thanks,
Adam

[Qemu-devel] Re: [RFC][RESEND][PATCH v1 00/15] virtproxy: host/guest communication layer

[Adam Litke]

You've got a lot of "objects" interacting with one another in virtproxy.
I think it would help other reviewers if you could describe the
relationships between the entities such as: VPDriver, VPConn, VPChannel,
VPIForward, VPOForward, etc.  This patch series is really wiring a lot
of things together.  Your documentation could end up as a nice ASCII art
diagram and associated commentary at the top of virtproxy.c.

-- 
Thanks,
Adam

[Qemu-devel] Re: [RFC][RESEND][PATCH v1 07/15] virtproxy: add vp_new() VPDriver constructor

[Adam Litke]

Be more descriptive here please.

On Wed, 2010-11-03 at 10:28 -0500, Michael Roth wrote:
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  virtproxy.c |   23 +++++++++++++++++++++++
>  virtproxy.h |    3 +++
>  2 files changed, 26 insertions(+), 0 deletions(-)
> 
> diff --git a/virtproxy.c b/virtproxy.c
> index c9c3022..cc0ac9a 100644
> --- a/virtproxy.c
> +++ b/virtproxy.c
> @@ -313,3 +313,26 @@ static void vp_channel_read(void *opaque)
>          drv->buflen += count;
>      }
>  }
> +
> +/* create/init VPDriver object */
> +VPDriver *vp_new(int fd, bool listen)
> +{
> +    VPDriver *drv = NULL;
> +
> +    drv = qemu_mallocz(sizeof(VPDriver));
> +    drv->listen_fd = -1;
> +    drv->channel_fd = -1;
> +    QLIST_INIT(&drv->oforwards);
> +    QLIST_INIT(&drv->conns);
> +
> +    if (listen) {
> +        /* provided FD is to be listened on for channel connection */
> +        drv->listen_fd = fd;
> +        vp_set_fd_handler(drv->listen_fd, vp_channel_accept, NULL, drv);
> +    } else {
> +        drv->channel_fd = fd;
> +        vp_set_fd_handler(drv->channel_fd, vp_channel_read, NULL, drv);
> +    }
> +
> +    return drv;
> +}
> diff --git a/virtproxy.h b/virtproxy.h
> index 0203421..3df1691 100644
> --- a/virtproxy.h
> +++ b/virtproxy.h
> @@ -31,4 +31,7 @@ int vp_set_fd_handler(int fd,
>                          IOHandler *fd_write,
>                          void *opaque);
> 
> +/* virtproxy interface */
> +VPDriver *vp_new(int fd, bool listen);
> +
>  #endif /* VIRTPROXY_H */

-- 
Thanks,
Adam

[Qemu-devel] Re: [RFC][RESEND][PATCH v1 09/15] virtproxy: add handler for data packets

[Adam Litke]

On Wed, 2010-11-03 at 10:28 -0500, Michael Roth wrote:
> Process VPPackets coming in from channel and send them to the
> appropriate server/client connections.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  virtproxy.c |   42 ++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 42 insertions(+), 0 deletions(-)
> 
> diff --git a/virtproxy.c b/virtproxy.c
> index 6c3611b..57ab2b0 100644
> --- a/virtproxy.c
> +++ b/virtproxy.c
> @@ -235,6 +235,48 @@ static void vp_channel_accept(void *opaque)
>      vp_set_fd_handler(drv->listen_fd, NULL, NULL, NULL);
>  }
> 
> +/* handle data packets
> + *
> + * process VPPackets containing data and send them to the corresponding
> + * FDs
> + */
> +static int vp_handle_data_packet(void *drv, const VPPacket *pkt)
> +{
> +    int fd, ret;
> +
> +    TRACE("called with drv: %p", drv);
> +
> +    if (pkt->type == VP_PKT_CLIENT) {
> +        TRACE("recieved client packet, client fd: %d, server fd: %d",
> +              pkt->payload.proxied.client_fd, pkt->payload.proxied.server_fd);
> +        fd = pkt->payload.proxied.server_fd;
> +    } else if (pkt->type == VP_PKT_SERVER) {
> +        TRACE("recieved server packet, client fd: %d, server fd: %d",
> +              pkt->payload.proxied.client_fd, pkt->payload.proxied.server_fd);
> +        fd = pkt->payload.proxied.client_fd;
> +    } else {
> +        TRACE("unknown packet type");
> +        return -1;
> +    }
> +
> +    /* TODO: proxied in non-blocking mode can causes us to spin here
> +     * for slow servers/clients. need to use write()'s and maintain
> +     * a per-conn write queue that we clear out before sending any
> +     * more data to the fd
> +     */

Hmm, so a guest can cause a denial of service in the host qemu process?
Are you working on adding the write queues?

> +    ret = vp_send_all(fd, (void *)pkt->payload.proxied.data,
> +            pkt->payload.proxied.bytes);
> +    if (ret == -1) {
> +        LOG("error sending data over channel");
> +        return -1;
> +    } else if (ret != pkt->payload.proxied.bytes) {
> +        TRACE("buffer full?");

This can happen?  Does this bring the whole connection down?  The whole
virtproxy instance?

-- 
Thanks,
Adam

[Qemu-devel] Re: [RFC][RESEND][PATCH v1 12/15] virtproxy: interfaces to set/remove VPIForwards

[Adam Litke]

Description please.

On Wed, 2010-11-03 at 10:28 -0500, Michael Roth wrote:
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  virtproxy.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  virtproxy.h |    2 ++
>  2 files changed, 61 insertions(+), 0 deletions(-)
> 
> diff --git a/virtproxy.c b/virtproxy.c
> index 5ec4e77..86a8e5b 100644
> --- a/virtproxy.c
> +++ b/virtproxy.c
> @@ -658,3 +658,62 @@ int vp_set_oforward(VPDriver *drv, int fd, const char *service_id)
> 
>      return 0;
>  }
> +
> +/* add/modify a service_id -> net/unix socket mapping
> + *
> + * "service_id" is a user-defined id for the service. this is what the
> + * remote end will use to proxy connections to a specific service on
> + * our end.
> + *
> + * if "port" is NULL, "addr" is the address of the net socket the
> + * service is running on. otherwise, addr is the path to the unix socket
> + * the service is running on.
> + *
> + * if "port" AND "addr" are NULL, find and remove the current iforward
> + * for this "service_id" if it exists.
> + *
> + * "ipv6" is a bool denoting whether or not to use ipv6
> + */
> +int vp_set_iforward(VPDriver *drv, const char *service_id, const char *addr,
> +                    const char *port, bool ipv6)
> +{
> +    VPIForward *f = get_iforward(drv, service_id);
> +
> +    if (addr == NULL && port == NULL) {
> +        if (f != NULL) {
> +            qemu_opts_del(f->socket_opts);
> +            QLIST_REMOVE(f, next);
> +            qemu_free(f);
> +        }
> +        return 0;
> +    }
> +
> +    if (f == NULL) {
> +        f = qemu_mallocz(sizeof(VPIForward));
> +        f->drv = drv;
> +        strncpy(f->service_id, service_id, VP_SERVICE_ID_LEN);
> +        QLIST_INSERT_HEAD(&drv->iforwards, f, next);
> +    } else {
> +        qemu_opts_del(f->socket_opts);
> +    }
> +
> +    /* stick socket-related options in a QemuOpts so we can
> +     * utilize qemu socket utility functions directly
> +     */
> +    f->socket_opts = qemu_opts_create(&vp_socket_opts, NULL, 0);
> +    if (port == NULL) {
> +        /* no port given, assume unix path */
> +        qemu_opt_set(f->socket_opts, "path", addr);
> +    } else {
> +        qemu_opt_set(f->socket_opts, "host", addr);
> +        qemu_opt_set(f->socket_opts, "port", port);
> +    }
> +
> +    if (ipv6) {
> +        qemu_opt_set(f->socket_opts, "ipv6", "on");
> +    } else {
> +        qemu_opt_set(f->socket_opts, "ipv4", "on");
> +    }
> +
> +    return 0;
> +}
> diff --git a/virtproxy.h b/virtproxy.h
> index 39d5d40..d2522b3 100644
> --- a/virtproxy.h
> +++ b/virtproxy.h
> @@ -34,5 +34,7 @@ int vp_set_fd_handler(int fd,
>  /* virtproxy interface */
>  VPDriver *vp_new(int fd, bool listen);
>  int vp_set_oforward(VPDriver *drv, int fd, const char *service_id);
> +int vp_set_iforward(VPDriver *drv, const char *service_id, const char *addr,
> +                    const char *port, bool ipv6);
> 
>  #endif /* VIRTPROXY_H */

-- 
Thanks,
Adam

[Qemu-devel] Re: [RFC][RESEND][PATCH v1 11/15] virtproxy: add vp_handle_packet()

[Adam Litke]

Description please.

On Wed, 2010-11-03 at 10:28 -0500, Michael Roth wrote:
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  virtproxy.c |   23 +++++++++++++++++++++++
>  1 files changed, 23 insertions(+), 0 deletions(-)
> 
> diff --git a/virtproxy.c b/virtproxy.c
> index 4f56aba..5ec4e77 100644
> --- a/virtproxy.c
> +++ b/virtproxy.c
> @@ -431,6 +431,29 @@ static int vp_handle_data_packet(void *drv, const VPPacket *pkt)
>      return 0;
>  }
> 
> +static inline int vp_handle_packet(VPDriver *drv, const VPPacket *pkt)
> +{
> +    int ret;
> +
> +    TRACE("called with drv: %p", drv);
> +
> +    if (pkt->magic != VP_MAGIC) {
> +        LOG("invalid packet magic field");
> +        return -1;
> +    }
> +
> +    if (pkt->type == VP_PKT_CONTROL) {
> +        ret = vp_handle_control_packet(drv, pkt);
> +    } else if (pkt->type == VP_PKT_CLIENT || pkt->type == VP_PKT_SERVER) {
> +        ret = vp_handle_data_packet(drv, pkt);
> +    } else {
> +        LOG("invalid packet type");
> +        return -1;
> +    }
> +
> +    return ret;
> +}
> +
>  /* read handler for communication channel
>   *
>   * de-multiplexes data coming in over the channel. for control messages

-- 
Thanks,
Adam

[Qemu-devel] Re: [RFC][RESEND][PATCH v1 13/15] virtproxy: add read handler for proxied connections

[Adam Litke]

On Wed, 2010-11-03 at 10:28 -0500, Michael Roth wrote:
> reads data from client/server connections as they become readable, then
> sends the data over the channel
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  virtproxy.c |   80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 80 insertions(+), 0 deletions(-)
> 
> diff --git a/virtproxy.c b/virtproxy.c
> index 86a8e5b..f3f7f46 100644
> --- a/virtproxy.c
> +++ b/virtproxy.c
> @@ -200,6 +200,86 @@ static VPIForward *get_iforward(const VPDriver *drv, const char *service_id)
>      return NULL;
>  }
> 
> +/* read handler for proxied connections */
> +static void vp_conn_read(void *opaque)
> +{
> +    VPConn *conn = opaque;
> +    VPDriver *drv = conn->drv;
> +    VPPacket pkt;
> +    char buf[VP_CONN_DATA_LEN];
> +    int fd, count, ret;
> +    bool client;
> +
> +    TRACE("called with opaque: %p, drv: %p", opaque, drv);
> +
> +    if (conn->state != VP_STATE_CONNECTED) {
> +        LOG("invalid connection state");
> +        return;
> +    }
> +
> +    if (conn->type != VP_CONN_CLIENT && conn->type != VP_CONN_SERVER) {
> +        LOG("invalid connection type");
> +        return;
> +    }

When either of these happen (or some of the other errors below), would
it make sense to close and clean up this connection since you're now in
an invalid state?

> +    /* TODO: all fields should be explicitly set so we shouldn't
> +     * need to memset. this might hurt if we beef up VPPacket size
> +     */
> +    memset(&pkt, 0, sizeof(VPPacket));
> +    pkt.magic = VP_MAGIC;
> +
> +    if (conn->type == VP_CONN_CLIENT) {
> +        client = true;
> +        fd = conn->client_fd;
> +    } else {
> +        client = false;
> +        fd = conn->server_fd;
> +    }
> +
> +    count = read(fd, buf, VP_CONN_DATA_LEN);
> +    if (count == -1) {
> +        LOG("read() failed: %s", strerror(errno));
> +        return;
> +    } else if (count == 0) {
> +        /* connection closed, tell remote end to clean up */
> +        TRACE("connection closed");
> +        pkt.type = VP_PKT_CONTROL;
> +        pkt.payload.msg.type = VP_CONTROL_CLOSE;
> +        if (client) {
> +            /* we're closing the client, have remote close the server conn */
> +            TRACE("closing connection for client fd %d", conn->client_fd);
> +            pkt.payload.msg.args.close.client_fd = -1;
> +            pkt.payload.msg.args.close.server_fd = conn->server_fd;
> +        } else {
> +            TRACE("closing connection for server fd %d", conn->server_fd);
> +            pkt.payload.msg.args.close.server_fd = -1;
> +            pkt.payload.msg.args.close.client_fd = conn->client_fd;;
> +        }
> +        /* clean up things on our end */
> +        closesocket(fd);
> +        vp_set_fd_handler(fd, NULL, NULL, NULL);
> +        QLIST_REMOVE(conn, next);
> +        qemu_free(conn);
> +    } else {
> +        TRACE("data read");
> +        pkt.type = client ? VP_PKT_CLIENT : VP_PKT_SERVER;
> +        pkt.payload.proxied.client_fd = conn->client_fd;
> +        pkt.payload.proxied.server_fd = conn->server_fd;
> +        memcpy(pkt.payload.proxied.data, buf, count);
> +        pkt.payload.proxied.bytes = count;
> +    }
> +
> +    ret = vp_send_all(drv->channel_fd, (void *)&pkt, sizeof(VPPacket));
> +    if (ret == -1) {
> +        LOG("error sending data over channel");
> +        return;
> +    }
> +    if (ret != sizeof(VPPacket)) {
> +        TRACE("buffer full?");
> +        return;
> +    }
> +}
> +
>  /* accept handler for communication channel
>   *
>   * accept()s connection to communication channel (for sockets), and sets

-- 
Thanks,
Adam

[Qemu-devel] Re: [RFC][RESEND][PATCH v1 09/15] virtproxy: add handler for data packets

[Adam Litke]

On Wed, 2010-11-03 at 10:28 -0500, Michael Roth wrote: 
> Process VPPackets coming in from channel and send them to the
> appropriate server/client connections.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  virtproxy.c |   42 ++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 42 insertions(+), 0 deletions(-)
> 
> diff --git a/virtproxy.c b/virtproxy.c
> index 6c3611b..57ab2b0 100644
> --- a/virtproxy.c
> +++ b/virtproxy.c
> @@ -235,6 +235,48 @@ static void vp_channel_accept(void *opaque)
>      vp_set_fd_handler(drv->listen_fd, NULL, NULL, NULL);
>  }
> 
> +/* handle data packets
> + *
> + * process VPPackets containing data and send them to the corresponding
> + * FDs
> + */
> +static int vp_handle_data_packet(void *drv, const VPPacket *pkt)
> +{
> +    int fd, ret;
> +
> +    TRACE("called with drv: %p", drv);
> +
> +    if (pkt->type == VP_PKT_CLIENT) {
> +        TRACE("recieved client packet, client fd: %d, server fd: %d",
> +              pkt->payload.proxied.client_fd, pkt->payload.proxied.server_fd);
> +        fd = pkt->payload.proxied.server_fd;
> +    } else if (pkt->type == VP_PKT_SERVER) {
> +        TRACE("recieved server packet, client fd: %d, server fd: %d",
> +              pkt->payload.proxied.client_fd, pkt->payload.proxied.server_fd);
> +        fd = pkt->payload.proxied.client_fd;
> +    } else {
> +        TRACE("unknown packet type");
> +        return -1;
> +    }
> +
> +    /* TODO: proxied in non-blocking mode can causes us to spin here
> +     * for slow servers/clients. need to use write()'s and maintain
> +     * a per-conn write queue that we clear out before sending any
> +     * more data to the fd
> +     */

Hmm, so a guest can cause a denial of service in the host qemu process?
Are you working on adding the write queues?

> +    ret = vp_send_all(fd, (void *)pkt->payload.proxied.data,
> +            pkt->payload.proxied.bytes);
> +    if (ret == -1) {
> +        LOG("error sending data over channel");
> +        return -1;
> +    } else if (ret != pkt->payload.proxied.bytes) {
> +        TRACE("buffer full?");

This can happen?  

> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
>  /* read handler for communication channel
>   *
>   * de-multiplexes data coming in over the channel. for control messages

-- 
Thanks,
Adam

[Qemu-devel] Re: [RFC][RESEND][PATCH v1 02/15] virtproxy: qemu-vp, standalone daemon skeleton

[Michael Roth]

On 11/03/2010 05:47 PM, Adam Litke wrote:
> On Wed, 2010-11-03 at 10:27 -0500, Michael Roth wrote:
>> +/* mirror qemu I/O-related code for standalone daemon */
>> +typedef struct IOHandlerRecord {
>> +    int fd;
>> +    IOCanReadHandler *fd_read_poll;
>> +    IOHandler *fd_read;
>> +    IOHandler *fd_write;
>> +    int deleted;
>> +    void *opaque;
>> +    /* temporary data */
>> +    struct pollfd *ufd;
>> +    QLIST_ENTRY(IOHandlerRecord) next;
>> +} IOHandlerRecord;
>
> As you say, this is exactly the same structure as defined in vl.c.  If
> you copy and paste code for a new feature you should be looking for a
> way to share it instead.  Why not create a separate patch that defines
> this structure in vl.h which can then be included by vl.c and virtproxy
> code.
>
>> +static QLIST_HEAD(, IOHandlerRecord) io_handlers =
>> +    QLIST_HEAD_INITIALIZER(io_handlers);
>> +
>> +int vp_set_fd_handler2(int fd,
>> +                         IOCanReadHandler *fd_read_poll,
>> +                         IOHandler *fd_read,
>> +                         IOHandler *fd_write,
>> +                         void *opaque)
>> +{
>> +    IOHandlerRecord *ioh;
>> +
>> +    if (!fd_read&&  !fd_write) {
>> +        QLIST_FOREACH(ioh,&io_handlers, next) {
>> +            if (ioh->fd == fd) {
>> +                ioh->deleted = 1;
>> +                break;
>> +            }
>> +        }
>> +    } else {
>> +        QLIST_FOREACH(ioh,&io_handlers, next) {
>> +            if (ioh->fd == fd)
>> +                goto found;
>> +        }
>> +        ioh = qemu_mallocz(sizeof(IOHandlerRecord));
>> +        QLIST_INSERT_HEAD(&io_handlers, ioh, next);
>> +    found:
>> +        ioh->fd = fd;
>> +        ioh->fd_read_poll = fd_read_poll;
>> +        ioh->fd_read = fd_read;
>> +        ioh->fd_write = fd_write;
>> +        ioh->opaque = opaque;
>> +        ioh->deleted = 0;
>> +    }
>> +    return 0;
>> +}
>
> Copied from vl.c -- export it instead.
>
>> +int vp_set_fd_handler(int fd,
>> +                        IOHandler *fd_read,
>> +                        IOHandler *fd_write,
>> +                        void *opaque)
>> +{
>> +    return vp_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque);
>> +}
>> +
>> +int vp_send_all(int fd, const void *buf, int len1)
>> +{
>> +    int ret, len;
>> +
>> +    len = len1;
>> +    while (len>  0) {
>> +        ret = write(fd, buf, len);
>> +        if (ret<  0) {
>> +            if (errno != EINTR&&  errno != EAGAIN) {
>> +                warn("write() failed");
>> +                return -1;
>> +            }
>> +        } else if (ret == 0) {
>> +            break;
>> +        } else {
>> +            buf += ret;
>> +            len -= ret;
>> +        }
>> +    }
>> +    return len1 - len;
>> +}
>
> Copied from qemu-char.c -- Share please.
>
>> +static void main_loop_wait(int nonblocking)
>> +{
>> +    IOHandlerRecord *ioh;
>> +    fd_set rfds, wfds, xfds;
>> +    int ret, nfds;
>> +    struct timeval tv;
>> +    int timeout = 1000;
>> +
>> +    if (nonblocking) {
>> +        timeout = 0;
>> +    }
>> +
>> +    /* poll any events */
>> +    nfds = -1;
>> +    FD_ZERO(&rfds);
>> +    FD_ZERO(&wfds);
>> +    FD_ZERO(&xfds);
>> +    QLIST_FOREACH(ioh,&io_handlers, next) {
>> +        if (ioh->deleted)
>> +            continue;
>> +        if (ioh->fd_read&&
>> +            (!ioh->fd_read_poll ||
>> +             ioh->fd_read_poll(ioh->opaque) != 0)) {
>> +            FD_SET(ioh->fd,&rfds);
>> +            if (ioh->fd>  nfds)
>> +                nfds = ioh->fd;
>> +        }
>> +        if (ioh->fd_write) {
>> +            FD_SET(ioh->fd,&wfds);
>> +            if (ioh->fd>  nfds)
>> +                nfds = ioh->fd;
>> +        }
>> +    }
>> +
>> +    tv.tv_sec = timeout / 1000;
>> +    tv.tv_usec = (timeout % 1000) * 1000;
>> +
>> +    ret = select(nfds + 1,&rfds,&wfds,&xfds,&tv);
>> +
>> +    if (ret>  0) {
>> +        IOHandlerRecord *pioh;
>> +
>> +        QLIST_FOREACH_SAFE(ioh,&io_handlers, next, pioh) {
>> +            if (ioh->deleted) {
>> +                QLIST_REMOVE(ioh, next);
>> +                qemu_free(ioh);
>> +                continue;
>> +            }
>> +            if (ioh->fd_read&&  FD_ISSET(ioh->fd,&rfds)) {
>> +                ioh->fd_read(ioh->opaque);
>> +            }
>> +            if (ioh->fd_write&&  FD_ISSET(ioh->fd,&wfds)) {
>> +                ioh->fd_write(ioh->opaque);
>> +            }
>> +        }
>> +    }
>> +
>
> This resembles vl.c's main_loop_wait() but I can see why you might want
> your own.  There is opportunity for sharing the select logic and ioh
> callbacks but I think that could be addressed later.
>

Yup these are all basically copy/pastes from vl.c. It feels a bit dirty, 
but I modeled things after the other qemu tools like qemu-nbd/qemu-io, 
which don't link against vl.c (and adding a target for tools to do so 
looks like it'd be a bit hairy since vl.c touches basically everything).

It might still make sense to share things like structs...but the ones 
I'm stealing here are specific to reproducing the main_loop_wait logic. 
So I guess the real question is whether main_loop_wait and friends make 
sense to expose as a utility function of some sort, and virtproxy seems 
to be the only use case so far.

[Qemu-devel] Re: [RFC][RESEND][PATCH v1 05/15] virtproxy: add accept handler for communication channel

[Michael Roth]

On 11/03/2010 06:02 PM, Adam Litke wrote:
> On Wed, 2010-11-03 at 10:28 -0500, Michael Roth wrote:
>> This accept()'s connections to the socket we told virt-proxy to listen
>> for the channel connection on and sets the appropriate read handler for
>> the resulting FD.
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>> ---
>>   virtproxy.c |   37 +++++++++++++++++++++++++++++++++++++
>>   1 files changed, 37 insertions(+), 0 deletions(-)
>>
>> diff --git a/virtproxy.c b/virtproxy.c
>> index fa17722..20532c2 100644
>> --- a/virtproxy.c
>> +++ b/virtproxy.c
>> @@ -166,6 +166,8 @@ static VPConn *get_conn(const VPDriver *drv, int fd, bool client)
>>       return NULL;
>>   }
>>
>> +static void vp_channel_accept(void *opaque);
>> +
>
> Why the forward declaration?  Can you move the function to a different
> place in the file to avoid this?
>
>

For this particular one vp_channel_accept and vp_channel_read reference 
each other at least this one needed a forward declaration. I'll see what 
I can do about these...I'm hesitant to add yet another header file but 
if that's preferred way I can move all the declarations into a 
virtproxy-internal.h file as you suggested.

[Qemu-devel] Re: [RFC][RESEND][PATCH v1 06/15] virtproxy: add read handler for communication channel

[Michael Roth]

On 11/03/2010 06:38 PM, Adam Litke wrote:
> On Wed, 2010-11-03 at 10:28 -0500, Michael Roth wrote:
>> Handle data coming in over the channel as VPPackets: Process control
>> messages and forward data from remote client/server connections to the
>> appropriate server/client FD on our end.
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>> ---
>>   virtproxy.c |   83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 83 insertions(+), 0 deletions(-)
>>
>> diff --git a/virtproxy.c b/virtproxy.c
>> index 20532c2..c9c3022 100644
>> --- a/virtproxy.c
>> +++ b/virtproxy.c
>> @@ -33,6 +33,7 @@
>>   #define VP_SERVICE_ID_LEN 32    /* max length of service id string */
>>   #define VP_PKT_DATA_LEN 1024    /* max proxied bytes per VPPacket */
>>   #define VP_CONN_DATA_LEN 1024   /* max bytes conns can send at a time */
>> +#define VP_CHAN_DATA_LEN 4096   /* max bytes channel can send at a time */
>>   #define VP_MAGIC 0x1F374059
>>
>>   /* listening fd, one for each service we're forwarding to remote end */
>> @@ -150,6 +151,8 @@ static QemuOptsList vp_socket_opts = {
>>       },
>>   };
>>
>> +static void vp_channel_read(void *opaque);
>
> Try to get rid of these forward declarations.  If you really need them,
> consider adding a virtproxy-internal.h.
>
>
>> @@ -230,3 +233,83 @@ static void vp_channel_accept(void *opaque)
>>       /* dont accept anymore connections until channel_fd is closed */
>>       vp_set_fd_handler(drv->listen_fd, NULL, NULL, NULL);
>>   }
>> +
>> +/* read handler for communication channel
>> + *
>> + * de-multiplexes data coming in over the channel. for control messages
>> + * we process them here, for data destined for a service or client we
>> + * send it to the appropriate FD.
>> + */
>> +static void vp_channel_read(void *opaque)
>> +{
>> +    VPDriver *drv = opaque;
>> +    VPPacket pkt;
>> +    int count, ret, buf_offset;
>> +    char buf[VP_CHAN_DATA_LEN];
>> +    char *pkt_ptr, *buf_ptr;
>> +
>> +    TRACE("called with opaque: %p", drv);
>> +
>> +    count = read(drv->channel_fd, buf, sizeof(buf));
>> +
>> +    if (count == -1) {
>> +        LOG("read() failed: %s", strerror(errno));
>> +        return;
>> +    } else if (count == 0) {
>> +        /* TODO: channel closed, this probably shouldn't happen for guest-side
>> +         * serial/virtio-serial connections, but need to confirm and consider
>> +         * what should happen in this case. as it stands this virtproxy instance
>> +         * is basically defunct at this point, same goes for "client" instances
>> +         * of virtproxy where the remote end has hung-up.
>> +         */
>> +        LOG("channel connection closed");
>> +        vp_set_fd_handler(drv->channel_fd, NULL, NULL, drv);
>> +        drv->channel_fd = -1;
>> +        if (drv->listen_fd) {
>> +            vp_set_fd_handler(drv->listen_fd, vp_channel_accept, NULL, drv);
>> +        }
>> +        /* TODO: should close/remove/delete all existing VPConns here */
>
> Looks like you have a little work TODO here still.  Perhaps you could
> add some reset/init functions that would make handling this easier.  The
> ability to reset state at both the channel and VPConn levels should give
> you the ability to handle errors more gracefully in other places where
> you currently just log them.
>

Yah, definitely. Planning on going back through these cases soon and 
handling cleanup more properly.

>> +    }
>> +
>> +    if (drv->buflen + count>= sizeof(VPPacket)) {
>> +        TRACE("initial packet, drv->buflen: %d", drv->buflen);
>> +        pkt_ptr = (char *)&pkt;
>> +        memcpy(pkt_ptr, drv->buf, drv->buflen);
>
> Can drv->buflen ever be>  sizeof(VPPacket) ?  You might consider adding
> an assert(drv->buflen<  sizeof(VPPacket)) unless it's mathematically
> impossible.
>

It shouldn't be possible. There are 2 situations where we set 
drv->buflen, and those situations are basically (in vp_channel_read):

1)

while (count > 0) {
     if (count >= sizeof(VPPacket)) {
         //handle packet
         count -= sizeof(VPPacket);
     } else {
         //buffer leftovers
         drv->buflen = count; // must be < sizeof(VPPacket) to get here
     }
}

2)

if (drv->buflen + count >= sizeof(VPPacket)) {
     //handle buffered/read packet data
} else {
     //buffer leftovers
     drv->buflen += count; // must be < sizeof(VPPacket)
}

>> +        pkt_ptr += drv->buflen;
>> +        memcpy(pkt_ptr, buf, sizeof(VPPacket) - drv->buflen);
>> +        /* handle first packet */
>> +        ret = vp_handle_packet(drv,&pkt);
>> +        if (ret != 0) {
>> +            LOG("error handling packet");
>> +        }
>> +        /* handle the rest of the buffer */
>> +        buf_offset = sizeof(VPPacket) - drv->buflen;
>> +        drv->buflen = 0;
>> +        buf_ptr = buf + buf_offset;
>> +        count -= buf_offset;
>> +        while (count>  0) {
>> +            if (count>= sizeof(VPPacket)) {
>> +                /* handle full packet */
>> +                TRACE("additional packet, drv->buflen: %d", drv->buflen);
>> +                memcpy((void *)&pkt, buf_ptr, sizeof(VPPacket));
>> +                ret = vp_handle_packet(drv,&pkt);
>> +                if (ret != 0) {
>> +                    LOG("error handling packet");
>> +                }
>> +                count -= sizeof(VPPacket);
>> +                buf_ptr += sizeof(VPPacket);
>> +            } else {
>> +                /* buffer the remainder */
>> +                TRACE("buffering packet");
>> +                memcpy(drv->buf, buf_ptr, count);
>> +                drv->buflen = count;
>> +                break;
>> +            }
>> +        }
>> +    } else {
>> +        /* haven't got a full VPPacket yet, buffer for later */
>> +        buf_ptr = drv->buf + drv->buflen;
>> +        memcpy(buf_ptr, buf, count);
>> +        drv->buflen += count;
>
> With this algorithm you can hold some data hostage indefinitely.  You
> either need to send out smaller packets of whatever data you receive or
> maybe have a timer that periodically fires to flush drv->buf.  Any
> thoughts on what you plan to do here?
>

All data is written to the channel in set-size packets 
(sizeof(VPPacket)), so if we only read a partial packet it's presumably 
because the other is either still sending, or it's sitting in a buffer 
in the underlying device (isa-serial/virtio-serial/etc). So assuming 
things eventually get flushed we *shouldn't* have an issue with this.

...that said...I'm currently debugging a issue over virtio-serial where 
data doesn't seem like it's getting flushed automatically so I'll 
definitely be looking for problem areas like this in the code :)

Might there be some subtlety about how virtio handles kicks/flushes that 
I might be missing? An example is I forward an ssh session over the 
channel to the guest's ssh server, do reads (top -d .001 for instance), 
let it run for a while, then start typing. What'll happen is a character 
echo won't get written to my terminal till some number of additional 
characters are typed. i.e.

my input:    echo "hello there this is a test"<enter>
my terminal: echo "he
my input:    echo "more"<enter>
my terminal: echo "hello there th

It doesn't behave like this initially though, and I haven't seen this 
with net or isa-serial.

Thought I'd ask...I'll keep looking in the meantime.

[Qemu-devel] Re: [RFC][RESEND][PATCH v1 09/15] virtproxy: add handler for data packets

[Michael Roth]

On 11/03/2010 07:46 PM, Adam Litke wrote:
> On Wed, 2010-11-03 at 10:28 -0500, Michael Roth wrote:
>> Process VPPackets coming in from channel and send them to the
>> appropriate server/client connections.
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>> ---
>>   virtproxy.c |   42 ++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 42 insertions(+), 0 deletions(-)
>>
>> diff --git a/virtproxy.c b/virtproxy.c
>> index 6c3611b..57ab2b0 100644
>> --- a/virtproxy.c
>> +++ b/virtproxy.c
>> @@ -235,6 +235,48 @@ static void vp_channel_accept(void *opaque)
>>       vp_set_fd_handler(drv->listen_fd, NULL, NULL, NULL);
>>   }
>>
>> +/* handle data packets
>> + *
>> + * process VPPackets containing data and send them to the corresponding
>> + * FDs
>> + */
>> +static int vp_handle_data_packet(void *drv, const VPPacket *pkt)
>> +{
>> +    int fd, ret;
>> +
>> +    TRACE("called with drv: %p", drv);
>> +
>> +    if (pkt->type == VP_PKT_CLIENT) {
>> +        TRACE("recieved client packet, client fd: %d, server fd: %d",
>> +              pkt->payload.proxied.client_fd, pkt->payload.proxied.server_fd);
>> +        fd = pkt->payload.proxied.server_fd;
>> +    } else if (pkt->type == VP_PKT_SERVER) {
>> +        TRACE("recieved server packet, client fd: %d, server fd: %d",
>> +              pkt->payload.proxied.client_fd, pkt->payload.proxied.server_fd);
>> +        fd = pkt->payload.proxied.client_fd;
>> +    } else {
>> +        TRACE("unknown packet type");
>> +        return -1;
>> +    }
>> +
>> +    /* TODO: proxied in non-blocking mode can causes us to spin here
>> +     * for slow servers/clients. need to use write()'s and maintain
>> +     * a per-conn write queue that we clear out before sending any
>> +     * more data to the fd
>> +     */
>
> Hmm, so a guest can cause a denial of service in the host qemu process?
> Are you working on adding the write queues?
>

Not a guest...though they could DoS the agent in this manner. But there 
seems to be an analogous situation in qemu currently, where a malicious 
client connects to, say, a socket setup to listen for telnet connections 
to the qemu monitor, sends a bunch of info commands, and doesn't read 
anything coming back to it. That might eventually cause the qemu process 
to hang when a monitor_flush->qemu_chr_write->send_all(client_fd) 
happens. A malicious client connecting to a forwarding port/socket on 
the host side could cause a similiar situation.

So if it's a problem, it seems like a problem that should be addressed 
more generally. A general, asynchronous implementation of send_all for 
instance.

>> +    ret = vp_send_all(fd, (void *)pkt->payload.proxied.data,
>> +            pkt->payload.proxied.bytes);
>> +    if (ret == -1) {
>> +        LOG("error sending data over channel");
>> +        return -1;
>> +    } else if (ret != pkt->payload.proxied.bytes) {
>> +        TRACE("buffer full?");
>
> This can happen?  Does this bring the whole connection down?  The whole
> virtproxy instance?
>

It can if write() returns 0, so only if we do a non-blocking write() 
inside vp_send_all/send_all. It's basically a TODO...since right now the 
writes are blocking ones, so it shouldn't happen. If we used 
non-blocking writes though we'd need to call vp_send_all(fd, (void 
*)pkt->payload.proxied.data + ret, pkt->payload.proxied.bytes - ret) in 
a loop, it wouldn't be an error case. I'll clean this up a bit.

[Qemu-devel] Re: [RFC][RESEND][PATCH v1 13/15] virtproxy: add read handler for proxied connections

[Michael Roth]

On 11/03/2010 08:21 PM, Adam Litke wrote:
> On Wed, 2010-11-03 at 10:28 -0500, Michael Roth wrote:
>> reads data from client/server connections as they become readable, then
>> sends the data over the channel
>>
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>> ---
>>   virtproxy.c |   80 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 files changed, 80 insertions(+), 0 deletions(-)
>>
>> diff --git a/virtproxy.c b/virtproxy.c
>> index 86a8e5b..f3f7f46 100644
>> --- a/virtproxy.c
>> +++ b/virtproxy.c
>> @@ -200,6 +200,86 @@ static VPIForward *get_iforward(const VPDriver *drv, const char *service_id)
>>       return NULL;
>>   }
>>
>> +/* read handler for proxied connections */
>> +static void vp_conn_read(void *opaque)
>> +{
>> +    VPConn *conn = opaque;
>> +    VPDriver *drv = conn->drv;
>> +    VPPacket pkt;
>> +    char buf[VP_CONN_DATA_LEN];
>> +    int fd, count, ret;
>> +    bool client;
>> +
>> +    TRACE("called with opaque: %p, drv: %p", opaque, drv);
>> +
>> +    if (conn->state != VP_STATE_CONNECTED) {
>> +        LOG("invalid connection state");
>> +        return;
>> +    }
>> +
>> +    if (conn->type != VP_CONN_CLIENT&&  conn->type != VP_CONN_SERVER) {
>> +        LOG("invalid connection type");
>> +        return;
>> +    }
>
> When either of these happen (or some of the other errors below), would
> it make sense to close and clean up this connection since you're now in
> an invalid state?
>

Yah, should definitely clean it up if that happens.

[Qemu-devel] Re: [RFC][RESEND][PATCH v1 00/15] virtproxy: host/guest communication layer

[Michael Roth]

On 11/03/2010 06:44 PM, Adam Litke wrote:
> You've got a lot of "objects" interacting with one another in virtproxy.
> I think it would help other reviewers if you could describe the
> relationships between the entities such as: VPDriver, VPConn, VPChannel,
> VPIForward, VPOForward, etc.  This patch series is really wiring a lot
> of things together.  Your documentation could end up as a nice ASCII art
> diagram and associated commentary at the top of virtproxy.c.
>

Hmm, yah, at the very least I should document these a bit better 
somewhere. virtproxy.c or maybe even docs/virtproxy.txt.

For the benefit of anyone reviewing these patches though:

VPDriver: encompasses the virtproxy state, which manages connections 
coming in/out of a data channel between the host and the guest, such as 
a virtio-serial or isa-serial port. Additional virtproxy/VPDriver 
instances can be set up to manage additional data channels. In the 
integrated version of these patches this channel will be accessed via an 
encapsulated virtproxy character device in the host, or an fd in the guest.

VPConn: encapsulates a bit of state, and client fds connected to 
forwarding sockets/ports that virtproxy set up or server fds that 
virtproxy established a connection to and is forwarding connections to.

VPOForward: encapsulates a listening forwarding port/socket that was 
passed to it, and an associated service_id. connections to the 
port/socket cause a VPConn to be set up, and data is proxied to the 
service on the other end of the channel that this service_id maps to.

VPIForward: maps a service_id to a local socket/port.

[Qemu-devel] Re: [RFC][RESEND][PATCH v1 02/15] virtproxy: qemu-vp, standalone daemon skeleton

[Adam Litke]

On Thu, 2010-11-04 at 08:57 -0500, Michael Roth wrote:
> > This resembles vl.c's main_loop_wait() but I can see why you might want
> > your own.  There is opportunity for sharing the select logic and ioh
> > callbacks but I think that could be addressed later.
> >
> 
> Yup these are all basically copy/pastes from vl.c. It feels a bit dirty, 
> but I modeled things after the other qemu tools like qemu-nbd/qemu-io, 
> which don't link against vl.c (and adding a target for tools to do so 
> looks like it'd be a bit hairy since vl.c touches basically everything).

> It might still make sense to share things like structs...but the ones 
> I'm stealing here are specific to reproducing the main_loop_wait logic. 
> So I guess the real question is whether main_loop_wait and friends make 
> sense to expose as a utility function of some sort, and virtproxy seems 
> to be the only use case so far.

You make a fair point about following precedent, but the thought of
dual-maintaining code into the future is not that appealing.  I guess we
could benefit from other voices on this topic.

-- 
Thanks,
Adam

Re: [Qemu-devel] Re: [RFC][RESEND][PATCH v1 02/15] virtproxy: qemu-vp, standalone daemon skeleton

[Amit Shah]

On (Fri) Nov 05 2010 [08:32:30], Adam Litke wrote:
> On Thu, 2010-11-04 at 08:57 -0500, Michael Roth wrote:
> > > This resembles vl.c's main_loop_wait() but I can see why you might want
> > > your own.  There is opportunity for sharing the select logic and ioh
> > > callbacks but I think that could be addressed later.
> > >
> > 
> > Yup these are all basically copy/pastes from vl.c. It feels a bit dirty, 
> > but I modeled things after the other qemu tools like qemu-nbd/qemu-io, 
> > which don't link against vl.c (and adding a target for tools to do so 
> > looks like it'd be a bit hairy since vl.c touches basically everything).
> 
> > It might still make sense to share things like structs...but the ones 
> > I'm stealing here are specific to reproducing the main_loop_wait logic. 
> > So I guess the real question is whether main_loop_wait and friends make 
> > sense to expose as a utility function of some sort, and virtproxy seems 
> > to be the only use case so far.
> 
> You make a fair point about following precedent, but the thought of
> dual-maintaining code into the future is not that appealing.  I guess we
> could benefit from other voices on this topic.

I agree we should share the code -- I have some patches for qemu
chardevs to behave reasonably when buffers are full (so we don't see
guest hangs).  You'll benefit as soon as that work enters git.

		Amit

Re: [Qemu-devel] Re: [RFC][RESEND][PATCH v1 02/15] virtproxy: qemu-vp, standalone daemon skeleton

[Michael Roth]

On 11/09/2010 04:45 AM, Amit Shah wrote:
> On (Fri) Nov 05 2010 [08:32:30], Adam Litke wrote:
>> On Thu, 2010-11-04 at 08:57 -0500, Michael Roth wrote:
>>>> This resembles vl.c's main_loop_wait() but I can see why you might want
>>>> your own.  There is opportunity for sharing the select logic and ioh
>>>> callbacks but I think that could be addressed later.
>>>>
>>>
>>> Yup these are all basically copy/pastes from vl.c. It feels a bit dirty,
>>> but I modeled things after the other qemu tools like qemu-nbd/qemu-io,
>>> which don't link against vl.c (and adding a target for tools to do so
>>> looks like it'd be a bit hairy since vl.c touches basically everything).
>>
>>> It might still make sense to share things like structs...but the ones
>>> I'm stealing here are specific to reproducing the main_loop_wait logic.
>>> So I guess the real question is whether main_loop_wait and friends make
>>> sense to expose as a utility function of some sort, and virtproxy seems
>>> to be the only use case so far.
>>
>> You make a fair point about following precedent, but the thought of
>> dual-maintaining code into the future is not that appealing.  I guess we
>> could benefit from other voices on this topic.
>
> I agree we should share the code -- I have some patches for qemu
> chardevs to behave reasonably when buffers are full (so we don't see
> guest hangs).  You'll benefit as soon as that work enters git.
>
> 		Amit
>

Thanks. I've been giving this a good look, but I'm still not sure I 
agree on how much code/future work we'd really be saving.

To be clear I basically re-implement the following:

qemu-char.c:send_all()
vl.c:qemu_set_fd_handler2()

qemu-vp.c has a main_loop_wait() that closely mirrors the one in vl.c, 
but I don't think that could be shared, or broken out into shareable 
bits in any meaningful way.

So the only thing in qemu-char.c I'm actually using/re-implementing is 
qemu-char.c:send_all(), and I think it'd make more sense to move this 
function out of qemu-char.c rather than fixing up the qemu tool targets 
(qemu-nbd/qemu-img/etc) to link against qemu-char.c, none of which 
currently seem to (nor does qemu-vp). Maybe qemu-sockets.c?

vl.c:qemu_set_fd_handler*() is a bit more involved, it touches state 
information specific to vl.c (vl.c:io_handlers list mainly, which is 
static, and vl.c:main_loop_wait loops through and modifies it), and 
currently it's noop'd in qemu-tool.c for standalone binaries.

Am I right in thinking the most logical approach is to move it out of 
qemu-tool.c/vl.c and into some common obj, along with vl.c:io_handlers 
and friends?

The major downside there is that to implement the i/o loop in qemu-vp.c, 
or any tool implementing an i/o loop modeled after main_loop_wait() and 
using qemu_set_fd_handler(), I'd need to be able to access/modify the 
io_handlers list, which means it'd need to be global rather than static 
as it is in vl.c.

I'm not sure how well that'd go over since it effectively allows us to 
bypass the qemu_set_fd_handler* interfaces and muck with it directly. 
And I'm not sure there's a reasonable way to share this code without 
making that tradeoff, or modifying these interfaces...which would touch 
quite a bit of code. Or maybe this isn't that big a concern, in which 
case I'd be willing to take a whack putting together a patch.

-Mike