[Qemu-devel] [RFC][PATCH v3 00/21] virtproxy: host/guest communication layer

[Qemu-devel] [RFC][PATCH v3 00/21] virtproxy: host/guest communication layer

[Michael Roth]

OVERVIEW:

Virtproxy proxies and multiplexes network/unix 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 is especially useful in environments where there is no network connectivity between the host and the guest, allowing for standard services to be utilized without modification or knowledge about the nature of the data connection.

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.

It should be noted that the proposed guest agent, virtagent, is not fundamentally dependent on this code, but by leveraging it we provide both the benefits of a closely-integrated QEMU guest agent as well as the flexibility to utilize other services in network-limited environments with this single guest daemon.

CHANGES IN V3:
 - virtproxy chardev options added to support specification of forwarding ports/sockets (examples below). code base is testable again now.
 - various cleanups

CHANGES IN V2:
 - host daemon deprecated, virtproxy now integrated into qemu via a virtproxy chardev. The command-line options have not yet been converted over so for now the virtproxy layer is no longer testable directly. This will be addressed soon. Virtagent will hook into this chardev via a boolean chardev option. That patchset will follow this one shortly.
 - deadlocking issue resolved

BUILD/USAGE INFO:
  # build guest daemon
  make qemu-vp
  ./qemu-vp -h

EXAMPLE USAGE:

note: oforward/iforward chardev options have not yet been converted over from original standalone host daemon implementation so this won't work till then. The examples however have been updated for reference.

 - 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 virtproxy,id=test0, \
             oforward=http:127.0.0.1:9080,oforward=ssh:127.0.0.1:22 \
    -device virtconsole,chardev=test0,name=test0 \
    ...
    # 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

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. Unix sockets can be specified by providing a pathname in place of the hostname/ip, and a '-' in place of the port number.

KNOWN ISSUES:

 - 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. UPDATE: this issue seems to be fixed here:

    http://git.kernel.org/?p=linux/kernel/git/rusty/linux-2.6-for-linus.git;a=commit;h=6df7aadcd9290807c464675098b5dd2dc9da5075

TODO:

 - 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 +-
 Makefile.target     |    2 +-
 configure           |    1 +
 qemu-char.c         |  130 +++++++
 qemu-config.c       |    6 +
 qemu-vp.c           |  517 ++++++++++++++++++++++++++++
 roms/seabios        |    2 +-
 virtproxy-builtin.c |   38 ++
 virtproxy.c         |  950 +++++++++++++++++++++++++++++++++++++++++++++++++++
 virtproxy.h         |   49 +++
 11 files changed, 1697 insertions(+), 3 deletions(-)

[Qemu-devel] [RFC][PATCH v3 01/21] virtproxy: base data structures and constants

[Michael Roth]

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 virtproxy.c |  136 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 virtproxy.h |   34 +++++++++++++++
 2 files changed, 170 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..8f18d83
--- /dev/null
+++ b/virtproxy.c
@@ -0,0 +1,136 @@
+/*
+ * 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 {
+    enum vp_context ctx;
+    int channel_fd;
+    int listen_fd;
+    CharDriverState *chr;
+    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

Re: [Qemu-devel] [RFC][PATCH v3 01/21] virtproxy: base data structures and constants

[Jes Sorensen]

On 11/16/10 02:15, Michael Roth wrote:
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  virtproxy.c |  136 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  virtproxy.h |   34 +++++++++++++++
>  2 files changed, 170 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..8f18d83
> --- /dev/null
> +++ b/virtproxy.c
> @@ -0,0 +1,136 @@
> +/*
> + * 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;

I am really not a fan of the typedefmeharder approach you are taking in
here, but others may disagree with me.

Cheers,
Jes

Re: [Qemu-devel] [RFC][PATCH v3 01/21] virtproxy: base data structures and constants

[Michael Roth]

On 11/18/2010 05:06 AM, Jes Sorensen wrote:
> On 11/16/10 02:15, Michael Roth wrote:
>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
>> ---
>>   virtproxy.c |  136 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   virtproxy.h |   34 +++++++++++++++
>>   2 files changed, 170 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..8f18d83
>> --- /dev/null
>> +++ b/virtproxy.c
>> @@ -0,0 +1,136 @@
>> +/*
>> + * 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;
>
> I am really not a fan of the typedefmeharder approach you are taking in
> here, but others may disagree with me.

Isn't typedef'ing structured types part of the qemu coding style 
guidelines? Or is there something beyond that I'm doing that isn't 
agreeable? The named struct + typedef approach seems to be widely used, 
I think to maintain compatibility with utility macros like these while 
still adhering to coding style in using typedefs wherever possible:

qemu-queue.h:
#define QLIST_HEAD(name, type)                       \
struct name {                                        \
         struct type *lh_first;  /* first element */  \
}

>
> Cheers,
> Jes
>

Re: [Qemu-devel] [RFC][PATCH v3 01/21] virtproxy: base data structures and constants

[Anthony Liguori]

On 11/18/2010 09:35 AM, Michael Roth wrote:
>>> +/* 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;
>>
>> I am really not a fan of the typedefmeharder approach you are taking in
>> here, but others may disagree with me.
>
>
> Isn't typedef'ing structured types part of the qemu coding style 
> guidelines?

Yes, I think Jes was just looking for an excuse to say "typedefmeharder" :-)

Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC][PATCH v3 01/21] virtproxy: base data structures and constants

[Jes Sorensen]

On 11/18/10 16:41, Anthony Liguori wrote:
> On 11/18/2010 09:35 AM, Michael Roth wrote:
>>>> +/* 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;
>>>
>>> I am really not a fan of the typedefmeharder approach you are taking in
>>> here, but others may disagree with me.
>>
>> Isn't typedef'ing structured types part of the qemu coding style
>> guidelines?
> 
> Yes, I think Jes was just looking for an excuse to say "typedefmeharder"
> :-)

Actually typedefs are not listed as a must do thing in CODING_STYLE,
fortunately! It's just a really bad habit that is applied all over the
place in QEMU :(

http://www.linuxjournal.com/article/5780?page=0,2
search for typedef, for a lot of good reasoning why we shouldn't do this.

Jes

Re: [Qemu-devel] [RFC][PATCH v3 01/21] virtproxy: base data structures and constants

[Anthony Liguori]

On 11/18/2010 09:51 AM, Jes Sorensen wrote:
>> Yes, I think Jes was just looking for an excuse to say "typedefmeharder"
>> :-)
>>      
> Actually typedefs are not listed as a must do thing in CODING_STYLE,
> fortunately!

That's a bug in CODING_STYLE.

typedefing structures is one of the core characteristics of QEMU coding 
style.

Regards,

Anthony Liguori

>   It's just a really bad habit that is applied all over the
> place in QEMU :(
>
> http://www.linuxjournal.com/article/5780?page=0,2
> search for typedef, for a lot of good reasoning why we shouldn't do this.
>
> Jes
>    

Re: [Qemu-devel] [RFC][PATCH v3 01/21] virtproxy: base data structures and constants

[Jes Sorensen]

On 11/18/10 16:56, Anthony Liguori wrote:
> On 11/18/2010 09:51 AM, Jes Sorensen wrote:
>>> Yes, I think Jes was just looking for an excuse to say "typedefmeharder"
>>> :-)
>>>      
>> Actually typedefs are not listed as a must do thing in CODING_STYLE,
>> fortunately!
> 
> That's a bug in CODING_STYLE.
> 
> typedefing structures is one of the core characteristics of QEMU coding
> style.

And here I was hoping that we could at least keep basic sanity in the
coding style document :(

typedefs are just plain wrong!

Jes

[Qemu-devel] [RFC][PATCH v3 02/21] 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

Re: [Qemu-devel] [RFC][PATCH v3 02/21] virtproxy: qemu-vp, standalone daemon skeleton

[Stefan Hajnoczi]

On Tue, Nov 16, 2010 at 1:15 AM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> 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

Can you move IOHandlerRecord, main_loop_wait(), and friends out of
vl.c and into a portable self-contained file that can be used both
from qemu and virtproxy?

Also, check out 0290b57bdfec83ca78b6d119ea9847bb17943328 which fixes a
main_loo_wait() bug.  It was recently committed and didn't make it
into qemu-vp.c.

It would be unfortunate to duplicate this code.

Stefan

Re: [Qemu-devel] [RFC][PATCH v3 02/21] virtproxy: qemu-vp, standalone daemon skeleton

[Michael Roth]

On 11/18/2010 04:04 AM, Stefan Hajnoczi wrote:
> On Tue, Nov 16, 2010 at 1:15 AM, Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
>> 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
>
> Can you move IOHandlerRecord, main_loop_wait(), and friends out of
> vl.c and into a portable self-contained file that can be used both
> from qemu and virtproxy?
>
> Also, check out 0290b57bdfec83ca78b6d119ea9847bb17943328 which fixes a
> main_loo_wait() bug.  It was recently committed and didn't make it
> into qemu-vp.c.
>
> It would be unfortunate to duplicate this code.
>

Adam and Amit brought this up before as well...at the time it seemed 
like it'd be too invasive, since to be useful we'd need to be able to 
provide our own IOHandlerRecord list to main_loop_wait() and 
qemu_set_fd_handler(), which would require changes to widely used 
interfaces.

Since then Adam suggested something like:

int qemu_set_fd_handler(int fd,
                         IOHandler *fd_read,
                         IOHandler *fd_write,
                         void *opaque)
{
     return qemu_set_fd_handler_generic(vl.c:io_handlers, int fd, ...)
}

And similar for main_loop_wait() and friends, which looks pretty 
reasonable. Moving qemu-char.c:send_all() to a generally accessible 
place like qemu-sockets.c was another good candidate to share. That 
should remove most of the dupes...I'll try to work some patches for 
these into the next round.

> Stefan

Re: [Qemu-devel] [RFC][PATCH v3 02/21] virtproxy: qemu-vp, standalone daemon skeleton

[Jes Sorensen]

On 11/16/10 02:15, Michael Roth wrote:
> 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>

Hi Michael,

A couple of comments:

> +/* 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;

Please move this to a header file. Any chance you could avoid some of
all those ugly typedefs too? I know we have way too many of the already,
but if it doesn't need to be a typedef, it's better not to make it one.

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

The formatting here is really odd, please make sure to align all
arguments, and maybe compress the lines a bit so it doesn't take up as
much realestate when you read the code.

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

Is -1 really an ideal error value to return here?

> +static void main_loop_wait(int nonblocking)
> +{
> +    IOHandlerRecord *ioh;
> +    fd_set rfds, wfds, xfds;
> +    int ret, nfds;
> +    struct timeval tv;

No good, please use qemu_timeval and friends for compatibility.

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

Missing braces.

> +        if (ioh->fd_read &&
> +            (!ioh->fd_read_poll ||
> +             ioh->fd_read_poll(ioh->opaque) != 0)) {

Put the || arguments on the same line so it is easier to read.

> +            FD_SET(ioh->fd, &rfds);
> +            if (ioh->fd > nfds)
> +                nfds = ioh->fd;

Missing braces.

> +        }
> +        if (ioh->fd_write) {
> +            FD_SET(ioh->fd, &wfds);
> +            if (ioh->fd > nfds)
> +                nfds = ioh->fd;

and again.

Cheers,
Jes

[Qemu-devel] [RFC][PATCH v3 03/21] 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 8f18d83..3686c77 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

Re: [Qemu-devel] [RFC][PATCH v3 03/21] virtproxy: add debug functions for virtproxy core

[Jes Sorensen]

On 11/16/10 02:15, 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 8f18d83..3686c77 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)
> +

I wonder if it wouldn't make sense to do this in a more generic way and
stick it in a header file. This type of debug code seems to show up
repeatedly all over the place.

Cheers,
Jes

Re: [Qemu-devel] [RFC][PATCH v3 03/21] virtproxy: add debug functions for virtproxy core

[Stefan Hajnoczi]

On Thu, Nov 18, 2010 at 11:09 AM, Jes Sorensen <Jes.Sorensen@redhat.com> wrote:
> On 11/16/10 02:15, 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 8f18d83..3686c77 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)
>> +
>
> I wonder if it wouldn't make sense to do this in a more generic way and
> stick it in a header file. This type of debug code seems to show up
> repeatedly all over the place.

I wanted to suggest actually using QEMU tracing but given that this
code compiles stand-alone, it may be more trouble to integrate than
it's worth.

For qemu and qemu-tools code we should be using tracing because you
can build it into production without high overhead.  How often do
these #defines get used, not often is my guess because no one wants to
tweak the source and rebuild, especially not in production.

Stefan

Re: [Qemu-devel] [RFC][PATCH v3 03/21] virtproxy: add debug functions for virtproxy core

[Michael Roth]

On 11/18/2010 05:43 AM, Stefan Hajnoczi wrote:
> On Thu, Nov 18, 2010 at 11:09 AM, Jes Sorensen<Jes.Sorensen@redhat.com>  wrote:
>> On 11/16/10 02:15, 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 8f18d83..3686c77 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)
>>> +
>>
>> I wonder if it wouldn't make sense to do this in a more generic way and
>> stick it in a header file. This type of debug code seems to show up
>> repeatedly all over the place.
>
> I wanted to suggest actually using QEMU tracing but given that this
> code compiles stand-alone, it may be more trouble to integrate than
> it's worth.
>
> For qemu and qemu-tools code we should be using tracing because you
> can build it into production without high overhead.  How often do
> these #defines get used, not often is my guess because no one wants to
> tweak the source and rebuild, especially not in production.
>

I'd planned on replacing the TRACE()'s with trace calls eventually. 
Maybe I missed it in the documentation, but do we have a way to enable 
them for qemu-tools or does we need to provide our own hooks and do it 
programmatically?

> Stefan

Re: [Qemu-devel] [RFC][PATCH v3 03/21] virtproxy: add debug functions for virtproxy core

[Stefan Hajnoczi]

On Thu, Nov 18, 2010 at 5:17 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> On 11/18/2010 05:43 AM, Stefan Hajnoczi wrote:
>>
>> On Thu, Nov 18, 2010 at 11:09 AM, Jes Sorensen<Jes.Sorensen@redhat.com>
>>  wrote:
>>>
>>> On 11/16/10 02:15, 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 8f18d83..3686c77 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)
>>>> +
>>>
>>> I wonder if it wouldn't make sense to do this in a more generic way and
>>> stick it in a header file. This type of debug code seems to show up
>>> repeatedly all over the place.
>>
>> I wanted to suggest actually using QEMU tracing but given that this
>> code compiles stand-alone, it may be more trouble to integrate than
>> it's worth.
>>
>> For qemu and qemu-tools code we should be using tracing because you
>> can build it into production without high overhead.  How often do
>> these #defines get used, not often is my guess because no one wants to
>> tweak the source and rebuild, especially not in production.
>>
>
> I'd planned on replacing the TRACE()'s with trace calls eventually. Maybe I
> missed it in the documentation, but do we have a way to enable them for
> qemu-tools or does we need to provide our own hooks and do it
> programmatically?

qemu-tools builds with QEMU tracing.  I wasn't sure whether you're
building in the qemu-tools style or whether qemu-vp is a completely
separate program.

So if qemu-vp is built with all the common qemu-tools make
infrastructure, then you should have tracing in there.

Stefan

[Qemu-devel] [RFC][PATCH v3 04/21] 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 3686c77..2cfd905 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -151,3 +151,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][PATCH v3 05/21] virtproxy, add vp_channel_send_all

[Michael Roth]

This handles sending of data to channel fd (qemu-vp in guest) or the
device associated with the virtproxy chardev for the host depending on
the context. vp_chr_read() wraps qemu_chr_read(), it'll be defined later
in virtproxy-builtin.c, and noop'd in the guest agent via qemu-vp.c.

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

diff --git a/virtproxy.c b/virtproxy.c
index 2cfd905..edca62e 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -152,6 +152,37 @@ static QemuOptsList vp_socket_opts = {
     },
 };
 
+static int vp_channel_send_all(VPDriver *drv, uint8_t *buf, int count)
+{
+    int ret;
+    CharDriverState *chr = drv->chr;
+
+    if (drv->chr != NULL) {
+        /* send data to guest via channel device's read handler */
+        vp_chr_read(chr, buf, count);
+        /* TODO: we assume here the full buffer was written to device
+         * due to the dev write handler being a void function.
+         * can we confirm? Do we need to?
+         */
+        ret = count;
+    } else if (drv->channel_fd != -1) {
+        /* send data to host via channel fd */
+        ret = vp_send_all(drv->channel_fd, buf, count);
+        if (ret == -1) {
+            LOG("error sending data");
+            goto out_bad;
+        }
+    } else {
+        LOG("driver in unknown state");
+        goto out_bad;
+    }
+
+    return ret;
+out_bad:
+    LOG("unable to send to channel");
+    return -1;
+}
+
 /* get VPConn by fd, "client" denotes whether to look for client or server */
 static VPConn *get_conn(const VPDriver *drv, int fd, bool client)
 {
diff --git a/virtproxy.h b/virtproxy.h
index 0203421..1a5e56a 100644
--- a/virtproxy.h
+++ b/virtproxy.h
@@ -30,5 +30,6 @@ int vp_set_fd_handler(int fd,
                         IOHandler *fd_read,
                         IOHandler *fd_write,
                         void *opaque);
+void vp_chr_read(CharDriverState *s, uint8_t *buf, int len);
 
 #endif /* VIRTPROXY_H */
-- 
1.7.0.4

Re: [Qemu-devel] [RFC][PATCH v3 05/21] virtproxy, add vp_channel_send_all

[Stefan Hajnoczi]

> +static int vp_channel_send_all(VPDriver *drv, uint8_t *buf, int count)
> +{
> +    int ret;
> +    CharDriverState *chr = drv->chr;
> +
> +    if (drv->chr != NULL) {
> +        /* send data to guest via channel device's read handler */
> +        vp_chr_read(chr, buf, count);
> +        /* TODO: we assume here the full buffer was written to device
> +         * due to the dev write handler being a void function.
> +         * can we confirm? Do we need to?
> +         */
> +        ret = count;
> +    } else if (drv->channel_fd != -1) {
> +        /* send data to host via channel fd */
> +        ret = vp_send_all(drv->channel_fd, buf, count);
> +        if (ret == -1) {
> +            LOG("error sending data");
> +            goto out_bad;
> +        }
> +    } else {
> +        LOG("driver in unknown state");
> +        goto out_bad;
> +    }
> +
> +    return ret;
> +out_bad:
> +    LOG("unable to send to channel");
> +    return -1;
> +}
> +

Propagate the error return value?  Currently this function discards
the specific error and also doesn't LOG() its value.

Stefan

[Qemu-devel] [RFC][PATCH v3 06/21] virtproxy: add accept handler for communication channel

[Michael Roth]

This accept()'s connections to the socket we told virtproxy to listen
for the channel connection on and sets the appropriate read handler for
the resulting FD. This is only used only for network-based channels and
will most likely be dropped with the introduction of the virtproxy
chardev.

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 edca62e..770b57b 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -199,6 +199,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)
 {
@@ -226,3 +228,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][PATCH v3 07/21] 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. We also provide here a helper
function to process a stream of packets from the channel.

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

diff --git a/virtproxy.c b/virtproxy.c
index 770b57b..091a223 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 */
@@ -152,6 +153,8 @@ static QemuOptsList vp_socket_opts = {
     },
 };
 
+static void vp_channel_read(void *opaque);
+
 static int vp_channel_send_all(VPDriver *drv, uint8_t *buf, int count)
 {
     int ret;
@@ -263,3 +266,96 @@ 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);
 }
+
+/* process a stream of packets coming in from the channel */
+int vp_handle_packet_buf(VPDriver *drv, const void *buf, int count)
+{
+    VPPacket pkt;
+    int ret, buf_offset;
+    char *pkt_ptr;
+    const char *buf_ptr;
+
+    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, drv->buflen: %d", drv->buflen);
+                memcpy(drv->buf, buf_ptr, count);
+                drv->buflen = count;
+                break;
+            }
+        }
+    } else {
+        /* haven't got a full VPPacket yet, buffer for later */
+        TRACE("buffering packet, drv->buflen: %d", drv->buflen);
+        memcpy(drv->buf + drv->buflen, buf, count);
+        drv->buflen += count;
+    }
+    return 0;
+}
+
+/* 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;
+    int count, ret;
+    char buf[VP_CHAN_DATA_LEN];
+
+    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 */
+    }
+
+    ret = vp_handle_packet_buf(drv, buf, count);
+    if (ret != 0) {
+        LOG("error handling packet stream");
+    }
+}
diff --git a/virtproxy.h b/virtproxy.h
index 1a5e56a..8fa0142 100644
--- a/virtproxy.h
+++ b/virtproxy.h
@@ -32,4 +32,7 @@ int vp_set_fd_handler(int fd,
                         void *opaque);
 void vp_chr_read(CharDriverState *s, uint8_t *buf, int len);
 
+/* virtproxy interface */
+int vp_handle_packet_buf(VPDriver *drv, const void *buf, int count);
+
 #endif /* VIRTPROXY_H */
-- 
1.7.0.4

Re: [Qemu-devel] [RFC][PATCH v3 07/21] virtproxy: add read handler for communication channel

[Anthony Liguori]

On 11/15/2010 07:16 PM, 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. We also provide here a helper
> function to process a stream of packets from the channel.
>
> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
> ---
>   virtproxy.c |   96 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   virtproxy.h |    3 ++
>   2 files changed, 99 insertions(+), 0 deletions(-)
>
> diff --git a/virtproxy.c b/virtproxy.c
> index 770b57b..091a223 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 */
> @@ -152,6 +153,8 @@ static QemuOptsList vp_socket_opts = {
>       },
>   };
>
> +static void vp_channel_read(void *opaque);
> +
>   static int vp_channel_send_all(VPDriver *drv, uint8_t *buf, int count)
>   {
>       int ret;
> @@ -263,3 +266,96 @@ 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);
>   }
> +
> +/* process a stream of packets coming in from the channel */
> +int vp_handle_packet_buf(VPDriver *drv, const void *buf, int count)
> +{
> +    VPPacket pkt;
> +    int ret, buf_offset;
> +    char *pkt_ptr;
> +    const char *buf_ptr;
> +
> +    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, drv->buflen: %d", drv->buflen);
> +                memcpy(drv->buf, buf_ptr, count);
> +                drv->buflen = count;
> +                break;
> +            }
> +        }
> +    } else {
> +        /* haven't got a full VPPacket yet, buffer for later */
> +        TRACE("buffering packet, drv->buflen: %d", drv->buflen);
> +        memcpy(drv->buf + drv->buflen, buf, count);
> +        drv->buflen += count;
> +    }
> +    return 0;
> +}
> +
> +/* 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;
> +    int count, ret;
> +    char buf[VP_CHAN_DATA_LEN];
> +
> +    TRACE("called with opaque: %p", drv);
> +
> +    count = read(drv->channel_fd, buf, sizeof(buf));
> +
> +    if (count == -1) {
>    

I think you notice this later in your series but this is the first 
indication that something is fundamentally wrong.

In a tool like this, you should be checking for EAGAIN everywhere and 
handling it gracefully but you assume you have synchronous fds 
everywhere.  This creates the following scenario:

In the host we have process Alice and process Mary.  They both 
communicate with process Bob and process Joe on the guest respectively 
through virtproxy.

Bob and Joe send packets to virtproxy, and virtproxy combines the 
packets into a single channel which it then sends to QEMU for QEMU to 
decode and sent back out to Alice and Mary.  Classic mux/demux.

Bob's communication with Alice is totally independent of Mary's 
communication with Joe.

However, because you multiplex synchronously, if virtproxy tries to send 
a packet from Mary to Joe and the socket buffer between virtproxy and 
Joe is full, then virtproxy blocks.  This means if Alice tries to send a 
message to Bob, the fact that Joe is not responding blocks their 
communication creating an artificial dependency between the two 
communications channels.

It's not just as simple as being async and queueing, because you can't 
queue in memory indefinitely so at some point in time, you simply have 
to stop reading packets.

Now imagine that Joe's socket buffer is full because he's talking to Bob 
through some other mechanism.  Joe being blocked on Bob is no problem 
because Bob has nothing to do with Alice and Joe's communication 
stream.  Eventually, Bob will get Joe what he needs and that will 
unblock Joe and let Alice's communication with Joe continue.

Now imagine that Bob is blocking Joe because he's waiting on a message 
from Mary.  This should be okay, Mary's communication with Bob *should* 
be independent on anything and as soon as Mary sends the message to Bob, 
Bob can talk to Joe, and Joe can talk to Mary.

Except, in virtproxy, the fact that Alice cannot talk to Joe blocks Mary 
from talking to Bob which creates a dead lock.

To be honest, there's no simple solution.  This is a classic queuing 
problem.  You need some form of congestion control to fix this.  That 
means virtproxy needs to be able to say to Alice that Joe is not ready 
to communicate right now and then let her know when she can resume 
communication.  That frees up the shared resource for Mary and Bob to use.

The fundamental problem here is that virtproxy is too generic.  It's 
trying to tackle too hard of a problem.  This isn't a problem with 
virtio-serial because virtio-serial has a dedicated queue for each port 
which allows each port to have it's own back off mechanism.

You can eliminate this problem by doing the following:

1) Have virtagent use two virtio-serial ports.

2) Have virtagent multiplex on it's own when given a single port.  Yes, 
the problem still presents itself but you've limited the communication 
scope which means you can practical avoid any deadlocks.  You only have 
two peers in the channel: qemu and virtagent.  There communication 
involves the following:

QEMU->virtagent RPC
- QEMU wants to send an RPC request.  Until this is entirely completed, 
it will never allow another request to be sent
- virtagent is waiting to receive an RPC request, it gets a packet and 
sees that it's a request
- virtagent processes the request, and sends back a response
- QEMU receives response, processes

virtagent->QEMU RPC
- Same as above with roles reversed

The only thing you need to handle is if QEMU tries to send a request to 
virtagent while virtagent simultaneous tries to send QEMU a request.  
This is simple to handle though because you are allowing one RPC request 
to be queued on either end at a given time.  This is really the key to 
success, by the nature of the communication, we can demultiplex into 
finite buffers.

Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC][PATCH v3 07/21] virtproxy: add read handler for communication channel

[Michael Roth]

On 11/16/2010 05:17 PM, Anthony Liguori wrote:
> Except, in virtproxy, the fact that Alice cannot talk to Joe blocks Mary
> from talking to Bob which creates a dead lock.
>
> To be honest, there's no simple solution. This is a classic queuing
> problem. You need some form of congestion control to fix this. That
> means virtproxy needs to be able to say to Alice that Joe is not ready
> to communicate right now and then let her know when she can resume
> communication. That frees up the shared resource for Mary and Bob to use.
>
> The fundamental problem here is that virtproxy is too generic. It's
> trying to tackle too hard of a problem. This isn't a problem with
> virtio-serial because virtio-serial has a dedicated queue for each port
> which allows each port to have it's own back off mechanism.

So, after a series of half-written rebuttals and working over the 
proposed scenarios with the solutions I'd had in mind, I think I've 
finally reached the same conclusion.

The potential for deadlocks can be traced to the use, or 
re-implementation, of qemu-char.c's send_all(). My thought had been that 
since we make assumptions there that these send_all()'s to processes 
will eventually complete, we can make similar assumptions in virtproxy. 
And if this is not an acceptable assumption, the solution would be a 
general one that would benefit chardev's in general, rather than 
something virtproxy-specific: mainly, an async/non-blocking alternative 
to send_all().

But you're right...whereas with normal chardevs, we have the option of 
throttling/polling for adequately-sized buffers at the device/virtio 
level, with virtproxy we'd need to do this for individual communication 
streams, which would require additional control packets. That, or we'd 
have to start dropping packets bound for a process when the 
corresponding write handler's queue was exhausted, which is not 
something we necessarily have to resort to when using a dedicated 
channel for a stream.

Probably a bit too much beyond the scope of virtagent... virtproxy was 
supposed to make things *easier* :)

>
> You can eliminate this problem by doing the following:
>
> 1) Have virtagent use two virtio-serial ports.
>

How bad is this from an end-user/deployment perspective? My first 
thought would be a shortcut invocation for virtio-serial guests:

./qemu -enable-virtagent

which would be equivalent to:

./qemu
   -chardev virtagent-client
   -chardev virtagent-server
   -device virtio-serial
   -device 
virtserialport,chardev=virtagent-client,name=org.qemu.virtagent-client
   -device 
virtserialport,chardev=virtagent-server,name=org.qemu.virtagent-server

And for alternative channels they'd need to do the verbose invocation, 
which I think works out well since we can't guess a good index for, say, 
isa-serial, or if we could, we'd still need some way to convey what it 
is so they can go start the agent accordingly.

And implementing virtagent-* as a simple chardev wrapper for -chardev 
socket lets us set up default paths for the client/server sockets that 
QEMU talks to send/receive RPCs, as well chardev IDs and invoking 
virtagent init routines. Potentially we can key into events to infer 
when the guest is connected to the other end as well.

Guest agent invocation would then be, for virtio-serial guests:

./virtagent

which would be equivalent to:

./virtagent
   --client virtio-serial:/dev/virtio-ports/org.qemu.virtagent-client
   --server virtio-serial:/dev/virtio-ports/org.qemu.virtagent-server

And for alternative channels, i.e. isa-serial, they'd have to do the 
manual invocation and point to the proper serial ports.

It's flexible, and the virtio-serial case is dead simple.

The major drawback is the potential scarcity of isa-serial ports: 2 out 
of 4 might be too much to ask for. And I think with a ubiquitous agent 
sticking with isa-serial to be safe might be a common deployment 
strategy for a lot of environments.

> 2) Have virtagent multiplex on it's own when given a single port. Yes,
> the problem still presents itself but you've limited the communication
> scope which means you can practical avoid any deadlocks. You only have
> two peers in the channel: qemu and virtagent. There communication
> involves the following:
>
> QEMU->virtagent RPC
> - QEMU wants to send an RPC request. Until this is entirely completed,
> it will never allow another request to be sent
> - virtagent is waiting to receive an RPC request, it gets a packet and
> sees that it's a request
> - virtagent processes the request, and sends back a response
> - QEMU receives response, processes
>
> virtagent->QEMU RPC
> - Same as above with roles reversed
>
> The only thing you need to handle is if QEMU tries to send a request to
> virtagent while virtagent simultaneous tries to send QEMU a request.
> This is simple to handle though because you are allowing one RPC request
> to be queued on either end at a given time. This is really the key to
> success, by the nature of the communication, we can demultiplex into
> finite buffers.

I'll look into this a bit more. But I'm hesitant to revisit a 
multiplexing solution at this late a stage unless 2 isa-serial ports 
seems exceedingly prohibitive. If the design seems simple enough I will 
try to work it into the next RFC, but I think 1) might be more feasible 
at this point. And we can still potentially work in 2) in a later release.

>
> Regards,
>
> Anthony Liguori

Thanks!

-Mike

Re: [Qemu-devel] [RFC][PATCH v3 07/21] virtproxy: add read handler for communication channel

[Stefan Hajnoczi]

As a side-note to the larger control flow questions:

Why not read() into &drv->buf[drv->buflen] and use it directly with
vp_handle_packet().  I don't think we need memcpy or temporary buffers
since we already have a buffer in drv.

There are extra buffers and memcpys here which make the code more complex.

Stefan

[Qemu-devel] [RFC][PATCH v3 08/21] virtproxy: add vp_new() VPDriver constructor

[Michael Roth]

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

diff --git a/virtproxy.c b/virtproxy.c
index 091a223..401d51c 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -359,3 +359,43 @@ static void vp_channel_read(void *opaque)
         LOG("error handling packet stream");
     }
 }
+
+VPDriver *vp_new(enum vp_context ctx, CharDriverState *s, int fd, bool listen)
+{
+    VPDriver *drv = NULL;
+
+    drv = qemu_mallocz(sizeof(VPDriver));
+    drv->listen_fd = -1;
+    drv->channel_fd = -1;
+    drv->chr = s;
+    drv->ctx = ctx;
+    QLIST_INIT(&drv->oforwards);
+    QLIST_INIT(&drv->conns);
+
+    if (ctx == VP_CTX_CHARDEV) {
+        if (drv->chr == NULL) {
+            LOG("invalid virtproxy chardev");
+            goto out_bad;
+        }
+    } else if (ctx == VP_CTX_FD) {
+        if (fd <= 0) {
+            LOG("invalid FD");
+            goto out_bad;
+        } else 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);
+        }
+    } else {
+        LOG("invalid context");
+        goto out_bad;
+    }
+
+    return drv;
+out_bad:
+    qemu_free(drv);
+    return NULL;
+}
diff --git a/virtproxy.h b/virtproxy.h
index 8fa0142..7cd2f76 100644
--- a/virtproxy.h
+++ b/virtproxy.h
@@ -16,8 +16,13 @@
 
 #include "qemu-common.h"
 #include "qemu-queue.h"
+#include "qemu-char.h"
 
 typedef struct VPDriver VPDriver;
+enum vp_context {
+    VP_CTX_CHARDEV, /* in qemu/host, channel is a virtproxy chardev */
+    VP_CTX_FD,      /* in guest, channel is an FD */
+};
 
 /* wrappers for s/vp/qemu/ functions we need */
 int vp_send_all(int fd, const void *buf, int len1);
@@ -33,6 +38,7 @@ int vp_set_fd_handler(int fd,
 void vp_chr_read(CharDriverState *s, uint8_t *buf, int len);
 
 /* virtproxy interface */
+VPDriver *vp_new(enum vp_context ctx, CharDriverState *s, int fd, bool listen);
 int vp_handle_packet_buf(VPDriver *drv, const void *buf, int count);
 
 #endif /* VIRTPROXY_H */
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v3 09/21] 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 |  104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 virtproxy.h |    1 +
 2 files changed, 105 insertions(+), 0 deletions(-)

diff --git a/virtproxy.c b/virtproxy.c
index 401d51c..f189e2c 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -12,6 +12,7 @@
  */
 
 #include "virtproxy.h"
+#include "qemu_socket.h"
 
 #define DEBUG_VP
 
@@ -360,6 +361,71 @@ 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->chr && 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_channel_send_all(drv, (uint8_t*)&pkt, sizeof(VPPacket));
+    if (ret == -1) {
+        LOG("error sending to channel");
+        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(enum vp_context ctx, CharDriverState *s, int fd, bool listen)
 {
     VPDriver *drv = NULL;
@@ -399,3 +465,41 @@ out_bad:
     qemu_free(drv);
     return NULL;
 }
+
+/* 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 7cd2f76..e901600 100644
--- a/virtproxy.h
+++ b/virtproxy.h
@@ -40,5 +40,6 @@ void vp_chr_read(CharDriverState *s, uint8_t *buf, int len);
 /* virtproxy interface */
 VPDriver *vp_new(enum vp_context ctx, CharDriverState *s, int fd, bool listen);
 int vp_handle_packet_buf(VPDriver *drv, const void *buf, int count);
+int vp_set_oforward(VPDriver *drv, int fd, const char *service_id);
 
 #endif /* VIRTPROXY_H */
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v3 10/21] 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 f189e2c..a0bbe7f 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -268,6 +268,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;
+}
+
 /* process a stream of packets coming in from the channel */
 int vp_handle_packet_buf(VPDriver *drv, const void *buf, int count)
 {
-- 
1.7.0.4

Re: [Qemu-devel] [RFC][PATCH v3 10/21] virtproxy: add handler for data packets

[Jes Sorensen]

On 11/16/10 02:16, Michael Roth wrote:
> +    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;
> +    }

-1 isn't a very friendly error value to pass up the stack.

> +    /* 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;
> +    }

Again here, please pass the error codes up the stack so we can later
handle them appropriately.

Cheers,
Jes

[Qemu-devel] [RFC][PATCH v3 11/21] 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 a0bbe7f..0cc8950 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -268,6 +268,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->chr && drv->channel_fd == -1) {
+            TRACE("channel no longer connected, ignoring packet");
+            return -1;
+        }
+
+        ret = vp_channel_send_all(drv, (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

Re: [Qemu-devel] [RFC][PATCH v3 11/21] virtproxy: add handler for control packet

[Jes Sorensen]

On 11/16/10 02:16, Michael Roth wrote:
> 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(-)

[snip]

> +
> +        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;
> +        }

This patch is a perfect example of why -1 as an error message is suboptimal.

> +        closesocket(fd);
> +        vp_set_fd_handler(fd, NULL, NULL, conn);
> +        QLIST_REMOVE(conn, next);
> +        qemu_free(conn);
> +        break;
> +    }
> +    }

You should never have two closing braces in the same column like this -
something is wrong with the formatting.

Cheers,
Jes

Re: [Qemu-devel] [RFC][PATCH v3 11/21] virtproxy: add handler for control packet

[Michael Roth]

On 11/18/2010 05:35 AM, Jes Sorensen wrote:
> On 11/16/10 02:16, Michael Roth wrote:
>> 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(-)
>
> [snip]
>
>> +
>> +        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;
>> +        }
>
> This patch is a perfect example of why -1 as an error message is suboptimal.
>
>> +        closesocket(fd);
>> +        vp_set_fd_handler(fd, NULL, NULL, conn);
>> +        QLIST_REMOVE(conn, next);
>> +        qemu_free(conn);
>> +        break;
>> +    }
>> +    }
>
> You should never have two closing braces in the same column like this -
> something is wrong with the formatting.

That's from using a block for the case
switch () {
case: {
     ...
}
}

Alternative is to indent the "case:", which is "right" I think, but 
aligning those with switch() seems to be pretty standard to conserve space.

>
> Cheers,
> Jes

Re: [Qemu-devel] [RFC][PATCH v3 11/21] virtproxy: add handler for control packet

[Jes Sorensen]

On 11/18/10 17:18, Michael Roth wrote:
> On 11/18/2010 05:35 AM, Jes Sorensen wrote:
>> You should never have two closing braces in the same column like this -
>> something is wrong with the formatting.
> 
> That's from using a block for the case
> switch () {
> case: {
>     ...
> }
> }
> 
> Alternative is to indent the "case:", which is "right" I think, but
> aligning those with switch() seems to be pretty standard to conserve space.

Why do you need braces around the case: { } ? That is not normally used
throughout the code.

Cheers,
Jes

Re: [Qemu-devel] [RFC][PATCH v3 11/21] virtproxy: add handler for control packet

[Michael Roth]

On 11/18/2010 10:22 AM, Jes Sorensen wrote:
> On 11/18/10 17:18, Michael Roth wrote:
>> On 11/18/2010 05:35 AM, Jes Sorensen wrote:
>>> You should never have two closing braces in the same column like this -
>>> something is wrong with the formatting.
>>
>> That's from using a block for the case
>> switch () {
>> case: {
>>      ...
>> }
>> }
>>
>> Alternative is to indent the "case:", which is "right" I think, but
>> aligning those with switch() seems to be pretty standard to conserve space.
>
> Why do you need braces around the case: { } ? That is not normally used
> throughout the code.
>

I use them here to avoid declaring a bunch of variables may not be used 
depending on the case. Although, it might cleaner to use separate 
functions here instead.

> Cheers,
> Jes

[Qemu-devel] [RFC][PATCH v3 12/21] 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 0cc8950..01a36c2 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -464,6 +464,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;
+}
+
 /* process a stream of packets coming in from the channel */
 int vp_handle_packet_buf(VPDriver *drv, const void *buf, int count)
 {
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v3 13/21] 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 01a36c2..b683ee6 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -722,3 +722,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 e901600..83cc905 100644
--- a/virtproxy.h
+++ b/virtproxy.h
@@ -41,5 +41,7 @@ void vp_chr_read(CharDriverState *s, uint8_t *buf, int len);
 VPDriver *vp_new(enum vp_context ctx, CharDriverState *s, int fd, bool listen);
 int vp_handle_packet_buf(VPDriver *drv, const void *buf, int count);
 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][PATCH v3 14/21] virtproxy: use new option list in virtproxy.c

[Michael Roth]

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

diff --git a/virtproxy.c b/virtproxy.c
index b683ee6..9e1745f 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -764,7 +764,7 @@ int vp_set_iforward(VPDriver *drv, const char *service_id, const char *addr,
     /* 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);
+    f->socket_opts = qemu_opts_create(&vp_opts, NULL, 0);
     if (port == NULL) {
         /* no port given, assume unix path */
         qemu_opt_set(f->socket_opts, "path", addr);
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v3 15/21] 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 |  114 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 virtproxy.h |    1 +
 2 files changed, 115 insertions(+), 0 deletions(-)

diff --git a/virtproxy.c b/virtproxy.c
index 9e1745f..f1f6859 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -37,6 +37,40 @@
 #define VP_CHAN_DATA_LEN 4096   /* max bytes channel can send at a time */
 #define VP_MAGIC 0x1F374059
 
+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 of list */ }
+    },
+};
+
+
 /* listening fd, one for each service we're forwarding to remote end */
 typedef struct VPOForward {
     VPDriver *drv;
@@ -233,6 +267,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_channel_send_all(drv, (uint8_t*)&pkt, sizeof(VPPacket));
+    if (ret == -1) {
+        LOG("error sending data over channel: %s", strerror(errno));
+        return;
+    }
+    if (ret != sizeof(VPPacket)) {
+        TRACE("buffer full?");
+        return;
+    }
+}
+
 /* accept handler for communication channel
  *
  * accept()s connection to communication channel (for sockets), and sets
diff --git a/virtproxy.h b/virtproxy.h
index 83cc905..ef168b0 100644
--- a/virtproxy.h
+++ b/virtproxy.h
@@ -23,6 +23,7 @@ enum vp_context {
     VP_CTX_CHARDEV, /* in qemu/host, channel is a virtproxy chardev */
     VP_CTX_FD,      /* in guest, channel is an FD */
 };
+extern QemuOptsList vp_opts;
 
 /* wrappers for s/vp/qemu/ functions we need */
 int vp_send_all(int fd, const void *buf, int len1);
-- 
1.7.0.4

Re: [Qemu-devel] [RFC][PATCH v3 15/21] virtproxy: add read handler for proxied connections

[Jes Sorensen]

On 11/16/10 02:16, Michael Roth wrote:
> +/* 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;
> +    }

A read handler which doesn't pass back error values in any shape or form
does seem a little weird to me?

Cheers,
Jes

[Qemu-devel] [RFC][PATCH v3 16/21] virtproxy: add option parser helper vp_parse()

[Michael Roth]

This routine will parse iforward/oforward options for both qemu-vp and
the virtproxy chardev

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

diff --git a/virtproxy.c b/virtproxy.c
index f1f6859..78cda04 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -895,3 +895,80 @@ int vp_set_iforward(VPDriver *drv, const char *service_id, const char *addr,
 
     return 0;
 }
+
+/* utility function to parse iforward/oforward options for qemu-vp
+ * or virtproxy chardev and put them into QemuOpts
+ */
+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) {
+            LOG("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) {
+            LOG("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) {
+        LOG("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) {
+        LOG("error parsing index");
+        return -1;
+    } else {
+        qemu_opt_set(opts, "index", index);
+    }
+    str += pos;
+    pos = 0;
+
+    return 0;
+}
diff --git a/virtproxy.h b/virtproxy.h
index ef168b0..6a53487 100644
--- a/virtproxy.h
+++ b/virtproxy.h
@@ -44,5 +44,6 @@ int vp_handle_packet_buf(VPDriver *drv, const void *buf, int count);
 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);
+int vp_parse(QemuOpts *opts, const char *str, bool is_channel);
 
 #endif /* VIRTPROXY_H */
-- 
1.7.0.4

Re: [Qemu-devel] [RFC][PATCH v3 16/21] virtproxy: add option parser helper vp_parse()

[Jes Sorensen]

On 11/16/10 02:16, Michael Roth wrote:
> +/* utility function to parse iforward/oforward options for qemu-vp
> + * or virtproxy chardev and put them into QemuOpts
> + */
> +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) {
> +            LOG("error parsing service id");
> +            return -1;

-EINVAL seems more useful than -1

Cheers,
Jes

[Qemu-devel] [RFC][PATCH v3 17/21] virtproxy: add virtproxy-builtin.c for compat defs

[Michael Roth]

Virtproxy relies on routines defined within qemu-vp which mirror various
i/o related operations in qemu to provide similar functionality for the
guest daemon. When building virtproxy as part of qemu rather than
qemu-vp we need these definitions to provide those functions in terms of
the original qemu functions.

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

diff --git a/virtproxy-builtin.c b/virtproxy-builtin.c
new file mode 100644
index 0000000..71fc5bc
--- /dev/null
+++ b/virtproxy-builtin.c
@@ -0,0 +1,38 @@
+/*
+ * virt-proxy - host/guest communication layer builtin definitions
+ *
+ * Copyright IBM Corp. 2010
+ *
+ * Authors:
+ *  Adam Litke        <aglitke@linux.vnet.ibm.com>
+ *  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.
+ *
+ */
+
+/* the following are functions we define in terms of qemu when linked
+ * against qemu/vl.c. these will be added on an as-needed basis
+ */
+
+#include "qemu-char.h"
+#include "qemu_socket.h"
+#include "virtproxy.h"
+
+int vp_set_fd_handler(int fd,
+                        IOHandler *fd_read,
+                        IOHandler *fd_write,
+                        void *opaque)
+{
+    return qemu_set_fd_handler(fd, fd_read, fd_write, opaque);
+}
+
+int vp_send_all(int fd, const void *buf, int len)
+{
+    return send_all(fd, buf, len);
+}
+
+void vp_chr_read(CharDriverState *s, uint8_t *buf, int len) {
+    return qemu_chr_read(s, buf, len);
+}
-- 
1.7.0.4

Re: [Qemu-devel] [RFC][PATCH v3 17/21] virtproxy: add virtproxy-builtin.c for compat defs

[Jes Sorensen]

On 11/16/10 02:16, Michael Roth wrote:
> Virtproxy relies on routines defined within qemu-vp which mirror various
> i/o related operations in qemu to provide similar functionality for the
> guest daemon. When building virtproxy as part of qemu rather than
> qemu-vp we need these definitions to provide those functions in terms of
> the original qemu functions.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  virtproxy-builtin.c |   38 ++++++++++++++++++++++++++++++++++++++
>  1 files changed, 38 insertions(+), 0 deletions(-)
>  create mode 100644 virtproxy-builtin.c
> 
> diff --git a/virtproxy-builtin.c b/virtproxy-builtin.c
> new file mode 100644
> index 0000000..71fc5bc
> --- /dev/null
> +++ b/virtproxy-builtin.c
> @@ -0,0 +1,38 @@
> +/*
> + * virt-proxy - host/guest communication layer builtin definitions
> + *
> + * Copyright IBM Corp. 2010
> + *
> + * Authors:
> + *  Adam Litke        <aglitke@linux.vnet.ibm.com>
> + *  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.
> + *
> + */
> +
> +/* the following are functions we define in terms of qemu when linked
> + * against qemu/vl.c. these will be added on an as-needed basis
> + */
> +
> +#include "qemu-char.h"
> +#include "qemu_socket.h"
> +#include "virtproxy.h"
> +
> +int vp_set_fd_handler(int fd,
> +                        IOHandler *fd_read,
> +                        IOHandler *fd_write,
> +                        void *opaque)

odd alignment again.

Jes

[Qemu-devel] [RFC][PATCH v3 18/21] virtproxy: qemu integration, add virtproxy chardev

[Michael Roth]

This allows us to create a virtproxy instance via a chardev. It can now
be created with something like:

qemu -chardev virtproxy,id=vp1 \
     -device virtio-serial \
     -device virtserialport,chardev=vp1

In the future the ability to add oforwards/iforwards in the command-line
invocation and the monitor will be added. For now we leave it to users
of virtproxy (currently only virtagent) to set up the forwarding
sockets/ports they need via direct virtproxy API calls.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 qemu-char.c   |  130 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qemu-config.c |    6 +++
 2 files changed, 136 insertions(+), 0 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 88997f9..bc7925c 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -1902,6 +1902,135 @@ return_err:
 }
 
 /***********************************************************/
+/* Virtproxy chardev driver */
+
+#include "virtproxy.h"
+
+static int vp_init_oforward(VPDriver *drv, QemuOpts *opts)
+{
+    int ret, fd;
+    const char *service_id;
+
+    if (qemu_opt_get(opts, "host") != NULL) {
+        fd = inet_listen_opts(opts, 0);
+    } else if (qemu_opt_get(opts, "path") != NULL) {
+        fd = unix_listen_opts(opts);
+    } else {
+        fprintf(stderr, "unable to find listening socket host/addr info");
+        return -1;
+    }
+
+    if (fd == -1) {
+        fprintf(stderr, "failed to create FD");
+        return -1;
+    }
+
+    service_id = qemu_opt_get(opts, "service_id");
+    if (service_id == NULL) {
+        fprintf(stderr, "no service_id specified");
+        return -1;
+    }
+
+    ret = vp_set_oforward(drv, fd, service_id);
+    if (ret != 0) {
+        fprintf(stderr, "error adding iforward");
+        return -1;
+    }
+
+    return 0;
+}
+
+static int vp_init_iforward(VPDriver *drv, QemuOpts *opts)
+{
+    const char *service_id, *addr, *port;
+    bool ipv6;
+    int ret;
+
+    service_id = qemu_opt_get(opts, "service_id");
+    if (service_id == NULL) {
+        fprintf(stderr, "no service_id specified");
+        return -1;
+    }
+    addr = qemu_opt_get(opts, "path");
+    port = NULL;
+    if (addr == NULL) {
+        /* map service to a network socket instead */
+        addr = qemu_opt_get(opts, "host");
+        port = qemu_opt_get(opts, "port");
+    }
+
+    ipv6 = qemu_opt_get_bool(opts, "ipv6", 0) ?
+           true : false;
+
+    ret = vp_set_iforward(drv, service_id, addr, port, ipv6);
+    if (ret != 0) {
+        fprintf(stderr, "error adding iforward");
+        return -1;
+    }
+
+    return 0;
+}
+
+static int vp_init_forwards(const char *name, const char *value, void *opaque)
+{
+    QemuOpts *opts = qemu_opts_create(&vp_opts, NULL, 0);
+    VPDriver *drv = opaque;
+    int ret;
+
+    if (strcmp(name, "oforward") != 0 && strcmp(name, "iforward") != 0) {
+        return 0;
+    }
+
+    /* parse opt string into QemuOpts */
+    ret = vp_parse(opts, value, 0);
+    if (ret < 0) {
+        fprintf(stderr, "error parsing virtproxy arguments");
+        return -1;
+    }
+
+    if (strcmp(name, "oforward") == 0) {
+        return vp_init_oforward(drv, opts);
+    } else if (strcmp(name, "iforward")) {
+        return vp_init_iforward(drv, opts);
+    }
+
+    return -1;
+}
+
+static int vp_chr_write(CharDriverState *chr, const uint8_t *buf, int len)
+{
+    VPDriver *drv = chr->opaque;
+    int ret;
+
+    fprintf(stderr, "called, opaque: %p\n", chr);
+
+    ret = vp_handle_packet_buf(drv, buf, len);
+    if (ret == -1) {
+        fprintf(stderr, "error handling data from virtproxy channel");
+    }
+
+    return 0;
+}
+
+static CharDriverState *qemu_chr_open_virtproxy(QemuOpts *opts)
+{
+    CharDriverState *chr = qemu_mallocz(sizeof(CharDriverState));
+    VPDriver *drv = vp_new(VP_CTX_CHARDEV, chr, 0, 0);
+
+    chr->opaque = drv;
+    chr->chr_write = vp_chr_write;
+
+    qemu_chr_generic_open(chr);
+
+    /* parse socket forwarding options */
+    qemu_opt_foreach(opts, vp_init_forwards, drv, 1);
+
+    /* for "info chardev" monitor command */
+    chr->filename = NULL;
+    return chr;
+}
+
+/***********************************************************/
 /* TCP Net console */
 
 typedef struct {
@@ -2408,6 +2537,7 @@ static const struct {
     { .name = "udp",       .open = qemu_chr_open_udp },
     { .name = "msmouse",   .open = qemu_chr_open_msmouse },
     { .name = "vc",        .open = text_console_init },
+    { .name = "virtproxy", .open = qemu_chr_open_virtproxy },
 #ifdef _WIN32
     { .name = "file",      .open = qemu_chr_open_win_file_out },
     { .name = "pipe",      .open = qemu_chr_open_win_pipe },
diff --git a/qemu-config.c b/qemu-config.c
index 52f18be..400e61a 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -146,6 +146,12 @@ static QemuOptsList qemu_chardev_opts = {
         },{
             .name = "signal",
             .type = QEMU_OPT_BOOL,
+        },{
+            .name = "oforward",
+            .type = QEMU_OPT_STRING,
+        },{
+            .name = "iforward",
+            .type = QEMU_OPT_STRING,
         },
         { /* end of list */ }
     },
-- 
1.7.0.4

Re: [Qemu-devel] [RFC][PATCH v3 18/21] virtproxy: qemu integration, add virtproxy chardev

[Jes Sorensen]

On 11/16/10 02:16, Michael Roth wrote:
> This allows us to create a virtproxy instance via a chardev. It can now
> be created with something like:
> 
> qemu -chardev virtproxy,id=vp1 \
>      -device virtio-serial \
>      -device virtserialport,chardev=vp1
> 
> In the future the ability to add oforwards/iforwards in the command-line
> invocation and the monitor will be added. For now we leave it to users
> of virtproxy (currently only virtagent) to set up the forwarding
> sockets/ports they need via direct virtproxy API calls.
> 
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  qemu-char.c   |  130 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qemu-config.c |    6 +++
>  2 files changed, 136 insertions(+), 0 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 88997f9..bc7925c 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -1902,6 +1902,135 @@ return_err:
>  }
>  
>  /***********************************************************/
> +/* Virtproxy chardev driver */
> +
> +#include "virtproxy.h"
> +
> +static int vp_init_oforward(VPDriver *drv, QemuOpts *opts)
> +{
> +    int ret, fd;
> +    const char *service_id;
> +
> +    if (qemu_opt_get(opts, "host") != NULL) {
> +        fd = inet_listen_opts(opts, 0);
> +    } else if (qemu_opt_get(opts, "path") != NULL) {
> +        fd = unix_listen_opts(opts);
> +    } else {
> +        fprintf(stderr, "unable to find listening socket host/addr info");
> +        return -1;

-1 again

Cheers,
Jes

[Qemu-devel] [RFC][PATCH v3 19/21] virtproxy: qemu integration, add virtproxy to Makefile.targets

[Michael Roth]

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

diff --git a/Makefile.target b/Makefile.target
index 91e6e74..f08c435 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -164,7 +164,7 @@ endif #CONFIG_BSD_USER
 # System emulator target
 ifdef CONFIG_SOFTMMU
 
-obj-y = arch_init.o cpus.o monitor.o machine.o gdbstub.o balloon.o
+obj-y = arch_init.o cpus.o monitor.o machine.o gdbstub.o balloon.o virtproxy.o virtproxy-builtin.o
 # virtio has to be here due to weird dependency between PCI and virtio-net.
 # need to fix this properly
 obj-y += virtio-blk.o virtio-balloon.o virtio-net.o virtio-serial-bus.o
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v3 20/21] virtproxy: qemu-vp, main logic

[Michael Roth]

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

diff --git a/qemu-vp.c b/qemu-vp.c
index 5075cdc..cfd2a69 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;
@@ -92,13 +136,19 @@ int vp_send_all(int fd, const void *buf, int len1)
     return len1 - len;
 }
 
+/* qemu_chr_read doesn't get used in guest so noop our wrapper function */
+void vp_chr_read(CharDriverState *s, uint8_t *buf, int len)
+{
+    return;
+}
+
 static void main_loop_wait(int nonblocking)
 {
     IOHandlerRecord *ioh;
     fd_set rfds, wfds, xfds;
     int ret, nfds;
     struct timeval tv;
-    int timeout = 1000;
+    int timeout = 100000;
 
     if (nonblocking) {
         timeout = 0;
@@ -149,3 +199,319 @@ static void main_loop_wait(int nonblocking)
         }
     }
 }
+
+#define VP_ARG_LEN 256
+
+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 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(VP_CTX_FD, NULL, 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;
+}
diff --git a/virtproxy.c b/virtproxy.c
index 78cda04..336c39f 100644
--- a/virtproxy.c
+++ b/virtproxy.c
@@ -164,30 +164,6 @@ struct VPDriver {
     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 */ }
-    },
-};
-
 static void vp_channel_read(void *opaque);
 
 static int vp_channel_send_all(VPDriver *drv, uint8_t *buf, int count)
-- 
1.7.0.4

[Qemu-devel] [RFC][PATCH v3 21/21] 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 02698e9..b4f93eb 100644
--- a/Makefile
+++ b/Makefile
@@ -129,7 +129,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 qemu-vp.o: $(GENERATED_HEADERS)
 
 qemu-img$(EXESUF): qemu-img.o qemu-tool.o qemu-error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) qemu-timer-common.o
 
@@ -137,6 +137,8 @@ qemu-nbd$(EXESUF): qemu-nbd.o qemu-tool.o qemu-error.o $(oslib-obj-y) $(trace-ob
 
 qemu-io$(EXESUF): qemu-io.o cmd.o qemu-tool.o qemu-error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) qemu-timer-common.o
 
+qemu-vp$(EXESUF): qemu-vp.o virtproxy.o qemu-tool.o qemu-error.o qemu-sockets.c $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) qemu-timer-common.o
+
 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 7025d2b..01bde83 100755
--- a/configure
+++ b/configure
@@ -2250,6 +2250,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

Re: [Qemu-devel] [RFC][PATCH v3 00/21] virtproxy: host/guest communication layer

[Stefan Hajnoczi]

On Tue, Nov 16, 2010 at 1:15 AM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
Some basic questions from someone who has never used virtio-serial:

> EXAMPLE USAGE:
>
> note: oforward/iforward chardev options have not yet been converted over from original standalone host daemon implementation so this won't work till then. The examples however have been updated for reference.
>
>  - 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 virtproxy,id=test0, \
>             oforward=http:127.0.0.1:9080,oforward=ssh:127.0.0.1:22 \
>    -device virtconsole,chardev=test0,name=test0 \
>    ...

Is virtconsole just a way of throwing something on the virtio-serial
bus?  A quick peek at hw/virtio-console.c suggests "virtserialport"
could also be used (and would be more intuitive because we don't want
a console, just a serial port)?

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

name=test0 above.  Is this a typo or where does test2 come from?

What does "virtserial-open" mean?  Why not
virtio-serial:/dev/virtio-ports/test2 to match the "-device
virtio-serial" above?  Virtio has a naming issue, every implementation
names things slightly differently :).

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

I don't see 9022 above, should it have been "oforward=ssh:127.0.0.1:9022"?

Stefan

Re: [Qemu-devel] [RFC][PATCH v3 00/21] virtproxy: host/guest communication layer

[Michael Roth]

Wow, good catches. I'd simplified the examples with v2 and failed to 
correct some of the typos I introduced then when sending out v3. Your 
intuition is correct on most of these:

On 11/16/2010 03:57 PM, Stefan Hajnoczi wrote:
> On Tue, Nov 16, 2010 at 1:15 AM, Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
> Some basic questions from someone who has never used virtio-serial:
>
>> EXAMPLE USAGE:
>>
>> note: oforward/iforward chardev options have not yet been converted over from original standalone host daemon implementation so this won't work till then. The examples however have been updated for reference.
>>
>>   - 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 virtproxy,id=test0, \
>>              oforward=http:127.0.0.1:9080,oforward=ssh:127.0.0.1:22 \
>>     -device virtconsole,chardev=test0,name=test0 \
>>     ...
>
> Is virtconsole just a way of throwing something on the virtio-serial
> bus?  A quick peek at hw/virtio-console.c suggests "virtserialport"
> could also be used (and would be more intuitive because we don't want
> a console, just a serial port)?
>

Yup, that's supposed to be -device virtserial

>>     # 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
>
> name=test0 above.  Is this a typo or where does test2 come from?
>

Yup, should be /dev/virtio-ports/test0

> What does "virtserial-open" mean?  Why not
> virtio-serial:/dev/virtio-ports/test2 to match the "-device
> virtio-serial" above?  Virtio has a naming issue, every implementation
> names things slightly differently :).
>

I added a verb because I was trying to stick with the convention for the 
unix-(connect|listen)/tcp-(connect|listen) methods used in earlier 
versions of qemu-vp (when it was run in the host as well as the guest, 
and connected to/listen for data from a channel via a -chardev 
socket,... or direct tcp connection). Now that the host daemon has been 
replaced with a virtproxy chardev these actually don't have much use 
anymore, and the tcp support has been dropped entirely...

So just plain "isa-serial"/"virtserial" might be a bit more intuitive 
now, since now there's a clear mapping between the channel type we 
specify to qemu-vp and the -device used for the channel. I'll go ahead a 
make this change.

>>     # from host, access guest http server
>>     wget http://locahost:9080
>>     # from host, access guest ssh server
>>     ssh localhost -p 9022
>
> I don't see 9022 above, should it have been "oforward=ssh:127.0.0.1:9022"?
>

Yes :) I'll make sure fix these up for the next round. Thanks!

> Stefan

Re: [Qemu-devel] [RFC][PATCH v3 00/21] virtproxy: host/guest communication layer

[Michael Roth]

On 11/16/2010 04:28 PM, Michael Roth wrote:
> Wow, good catches. I'd simplified the examples with v2 and failed to
> correct some of the typos I introduced then when sending out v3. Your
> intuition is correct on most of these:
>
> On 11/16/2010 03:57 PM, Stefan Hajnoczi wrote:
>>
>> Is virtconsole just a way of throwing something on the virtio-serial
>> bus? A quick peek at hw/virtio-console.c suggests "virtserialport"
>> could also be used (and would be more intuitive because we don't want
>> a console, just a serial port)?
>>
>
> Yup, that's supposed to be -device virtserial
>

Argh, -device virtserialport

>>> # 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
>>
>> name=test0 above. Is this a typo or where does test2 come from?
>>
>
> Yup, should be /dev/virtio-ports/test0
>
>> What does "virtserial-open" mean? Why not
>> virtio-serial:/dev/virtio-ports/test2 to match the "-device
>> virtio-serial" above? Virtio has a naming issue, every implementation
>> names things slightly differently :).
>>
>
> I added a verb because I was trying to stick with the convention for the
> unix-(connect|listen)/tcp-(connect|listen) methods used in earlier
> versions of qemu-vp (when it was run in the host as well as the guest,
> and connected to/listen for data from a channel via a -chardev
> socket,... or direct tcp connection). Now that the host daemon has been
> replaced with a virtproxy chardev these actually don't have much use
> anymore, and the tcp support has been dropped entirely...
>
> So just plain "isa-serial"/"virtserial" might be a bit more intuitive
> now, since now there's a clear mapping between the channel type we
> specify to qemu-vp and the -device used for the channel. I'll go ahead a
> make this change.
>

"isa-serial"/"virtio-serial"