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

login
register
mail settings
Submitter Michael Roth
Date Nov. 3, 2010, 3:27 p.m.
Message ID <1288798090-7127-3-git-send-email-mdroth@linux.vnet.ibm.com>
Download mbox | patch
Permalink /patch/70016/
State New
Headers show

Comments

Michael Roth - Nov. 3, 2010, 3:27 p.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
Adam Litke - Nov. 3, 2010, 10:47 p.m.
On Wed, 2010-11-03 at 10:27 -0500, Michael Roth wrote:
> +/* mirror qemu I/O-related code for standalone daemon */
> +typedef struct IOHandlerRecord {
> +    int fd;
> +    IOCanReadHandler *fd_read_poll;
> +    IOHandler *fd_read;
> +    IOHandler *fd_write;
> +    int deleted;
> +    void *opaque;
> +    /* temporary data */
> +    struct pollfd *ufd;
> +    QLIST_ENTRY(IOHandlerRecord) next;
> +} IOHandlerRecord;

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

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

Copied from vl.c -- export it instead.

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

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

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

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

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

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

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

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

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

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

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

To be clear I basically re-implement the following:

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

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

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

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

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

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

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

-Mike

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