Patchwork [RFC,v3,02/21] virtproxy: qemu-vp, standalone daemon skeleton

login
register
mail settings
Submitter Michael Roth
Date Nov. 16, 2010, 1:15 a.m.
Message ID <1289870175-14880-3-git-send-email-mdroth@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/71330/
State New
Headers show

Comments

Michael Roth - Nov. 16, 2010, 1:15 a.m.
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
Stefan Hajnoczi - Nov. 18, 2010, 10:04 a.m.
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
Jes Sorensen - Nov. 18, 2010, 11:04 a.m.
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
Michael Roth - Nov. 18, 2010, 3:46 p.m.
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

Patch

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