| Message ID | 1288798090-7127-3-git-send-email-mdroth@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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.
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.
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.
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
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
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