| Message ID | 1289870175-14880-3-git-send-email-mdroth@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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); + } + } + } +}
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