| Message ID | 1299528642-23631-2-git-send-email-mdroth@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 03/07/2011 09:10 PM, Michael Roth wrote: > This allows us to implement an i/o loop outside of vl.c that can > interact with objects that use qemu_set_fd_handler() I must say I really dislike the patches 1..3. It's _really_ getting the QEMU NIH worse. While it is not really possible to get a new shiny mainloop infrastructure in QEMU like snapping fingers (and I'm not sure the glib mainloop will ever happen there), there is no reason not to adopt glib's infrastructure in virtagent. While cooperation between QEMU and virtagent is close, it is IMHO a substantially separate project that can afford starting from a clean slate. If anybody disagrees, I'd be happy to hear their opinion anyway! I'm sorry I'm saying this only now and I've been ignoring this series until v7. Paolo
On 03/07/2011 09:10 PM, Michael Roth wrote: > + > +/* XXX: fd_read_poll should be suppressed, but an API change is > + necessary in the character devices to suppress fd_can_read(). */ > +int qemu_set_fd_handler3(void *ioh_record_list, > + int fd, > + IOCanReadHandler *fd_read_poll, > + IOHandler *fd_read, > + IOHandler *fd_write, > + void *opaque) What's the reason to introduce this additional indirection (and with a void rather than opaque pointer)? A global iohandlers list would be fine in qemu-ioh.c (and it would be a worthwhile patch anyway for QEMU). Paolo
On 03/09/2011 07:58 AM, Paolo Bonzini wrote: > On 03/07/2011 09:10 PM, Michael Roth wrote: >> This allows us to implement an i/o loop outside of vl.c that can >> interact with objects that use qemu_set_fd_handler() > > I must say I really dislike the patches 1..3. It's _really_ getting the > QEMU NIH worse. While it is not really possible to get a new shiny > mainloop infrastructure in QEMU like snapping fingers (and I'm not sure > the glib mainloop will ever happen there), there is no reason not to > adopt glib's infrastructure in virtagent. While cooperation between QEMU > and virtagent is close, it is IMHO a substantially separate project that > can afford starting from a clean slate. > > If anybody disagrees, I'd be happy to hear their opinion anyway! > > I'm sorry I'm saying this only now and I've been ignoring this series > until v7. In the context of virtagent I would agree. The only complication there being that a large part of the event-driven code (the async read/write handlers for instance) is shared between virtagent and the host. Possibility this could be worked around with a set of wrappers..but it's hard to say. But more importantly, I wouldn't think of these changes as being specific to virtagent though. Currently we have a lot of qemu tools that stub out portions of the block code they pull in (qemu_set_fd_handler and whatnot). I think it might be beneficial to future tools/test utilities that they actually be able to drive things like aio and timer events. We just keep stubbing more and more things out in these cases, which I would argue is even worse because it can place artificial constraints on how code is written that happens to get used by such tools. > > Paolo
On 03/09/2011 07:58 AM, Paolo Bonzini wrote: > On 03/07/2011 09:10 PM, Michael Roth wrote: >> This allows us to implement an i/o loop outside of vl.c that can >> interact with objects that use qemu_set_fd_handler() > > I must say I really dislike the patches 1..3. It's _really_ getting > the QEMU NIH worse. While it is not really possible to get a new > shiny mainloop infrastructure in QEMU like snapping fingers (and I'm > not sure the glib mainloop will ever happen there), there is no reason > not to adopt glib's infrastructure in virtagent. I'm 90% in agreement with you but in terms of delivering a Windows guest agent, instead of just having an exe, we're now talking about quite a few extra DLLs. It's not a huge problem and probably makes a ton of sense if virt-agent ever adopts more sophisticated functionality but I wanted to at least raise this point. Regards, Anthony Liguori > While cooperation between QEMU and virtagent is close, it is IMHO a > substantially separate project that can afford starting from a clean > slate. > > If anybody disagrees, I'd be happy to hear their opinion anyway! > > I'm sorry I'm saying this only now and I've been ignoring this series > until v7. > > Paolo > >
On 03/09/2011 03:11 PM, Michael Roth wrote: > > In the context of virtagent I would agree. The only complication there > being that a large part of the event-driven code (the async read/write > handlers for instance) is shared between virtagent and the host. What exactly? The dependencies in 16/16 give: 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-timer.o Compared to other tools, only qemu-sockets.c is added (and timers); overall it is quite self contained and interfaces well with glib's GIOChannels, which provide qemu_set_fd_handler-equivalent functionality. In addition, qemu iohandlers have a lot of unwritten assumptions, for example on Win32 they only work with sockets and not other kinds of file descriptors. Paolo
On 03/09/2011 07:58 AM, Paolo Bonzini wrote: > On 03/07/2011 09:10 PM, Michael Roth wrote: >> This allows us to implement an i/o loop outside of vl.c that can >> interact with objects that use qemu_set_fd_handler() > > I must say I really dislike the patches 1..3. It's _really_ getting > the QEMU NIH worse. While it is not really possible to get a new > shiny mainloop infrastructure in QEMU like snapping fingers (and I'm > not sure the glib mainloop will ever happen there While it's not at the immediate top at my MUST DO list, it's still pretty high FWIW. I think the benefits are huge because it means we can refactor things like the VNC server to just interact with glib which means it can become generally useful outside of QEMU. Regards, Anthony Liguori > ), there is no reason not to adopt glib's infrastructure in > virtagent. While cooperation between QEMU and virtagent is close, it > is IMHO a substantially separate project that can afford starting from > a clean slate. > > If anybody disagrees, I'd be happy to hear their opinion anyway! > > I'm sorry I'm saying this only now and I've been ignoring this series > until v7. > > Paolo
On 03/09/2011 03:40 PM, Anthony Liguori wrote: >> >> I must say I really dislike the patches 1..3. It's _really_ getting >> the QEMU NIH worse. While it is not really possible to get a new >> shiny mainloop infrastructure in QEMU like snapping fingers (and I'm >> not sure the glib mainloop will ever happen there > > While it's not at the immediate top at my MUST DO list, it's still > pretty high FWIW. I think the benefits are huge because it means we can > refactor things like the VNC server to just interact with glib which > means it can become generally useful outside of QEMU. I actually agree, but there are a lot of cleanups to do to the code before it becomes viable. I would be surprised to see it before 0.17 say (maybe a pleasant surprise, but still). In any case, introducing more dependencies from the tools to core QEMU would mean needing wrappers over wrappers over wrappers when QEMU itself is refactored. Perhaps for virtagent something like libnih would be more appropriate? Not sure about its Win32 portability though. Paolo
On 03/09/2011 08:38 AM, Paolo Bonzini wrote: > On 03/09/2011 03:11 PM, Michael Roth wrote: >> >> In the context of virtagent I would agree. The only complication there >> being that a large part of the event-driven code (the async read/write >> handlers for instance) is shared between virtagent and the host. > > What exactly? The dependencies in 16/16 give: > > 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-timer.o These objs: virtagent.o virtagent-server.o virtagent-common.o virtagent-transport.o virtagent-manager.o Are shared by qemu and qemu-va. virtagent.o uses the common timer infrastructure introduced in patch 3, and virtagent-transport/virtagent-common use the iohandler stuff from patch 1/2. On the host, qemu's event loop drives them, and on the guest, qemu-va's event loop drives them. Not sure what level of sharing we can maintain with 2 different event loops. I'm sure it's doable, just not sure what it would end up looking like. I should note that initially all the qemu_set_fd_handler() stuff was wrapped to provide compatibility between separate event loop implementations in qemu/qemu-va. Sharing the event loop code was a widely-held consensus from earlier reviews. I'm not sure glib is so nice that it's worth back-peddling on that. And if we do eventually make qemu's event loop glib-based, consumers of the common code here would get migrated over for free. > > Compared to other tools, only qemu-sockets.c is added (and timers); > overall it is quite self contained and interfaces well with glib's > GIOChannels, which provide qemu_set_fd_handler-equivalent functionality. > > In addition, qemu iohandlers have a lot of unwritten assumptions, for > example on Win32 they only work with sockets and not other kinds of file > descriptors. Hmm, that could be a problem... It seems like a more general one though, that might benefit consumers other than virtagent. So if this is addressed at some point, consumers of the common infrastructure proposed here would all benefit. > > Paolo
On 03/09/2011 04:01 PM, Michael Roth wrote: > > These objs: virtagent.o virtagent-server.o virtagent-common.o > virtagent-transport.o virtagent-manager.o > > Are shared by qemu and qemu-va. Okay, that's what I missed. Then I guess it's a pity but there's a good reason. > It seems like a more general one though, that might benefit consumers > other than virtagent. So if this is addressed at some point, > consumers of the common infrastructure proposed here would all > benefit. I doubt, Win32 sockets are almost unusable (QEMU does "select" on them when it has an event from something else) and chardevs use a separate polling mechanism. Paolo
On 03/09/2011 08:45 AM, Paolo Bonzini wrote: > On 03/09/2011 03:40 PM, Anthony Liguori wrote: >>> >>> I must say I really dislike the patches 1..3. It's _really_ getting >>> the QEMU NIH worse. While it is not really possible to get a new >>> shiny mainloop infrastructure in QEMU like snapping fingers (and I'm >>> not sure the glib mainloop will ever happen there >> >> While it's not at the immediate top at my MUST DO list, it's still >> pretty high FWIW. I think the benefits are huge because it means we can >> refactor things like the VNC server to just interact with glib which >> means it can become generally useful outside of QEMU. > > I actually agree, but there are a lot of cleanups to do to the code > before it becomes viable. I would be surprised to see it before 0.17 > say (maybe a pleasant surprise, but still). > > In any case, introducing more dependencies from the tools to core QEMU > would mean needing wrappers over wrappers over wrappers when QEMU > itself is refactored. > > Perhaps for virtagent something like libnih would be more appropriate? > Not sure about its Win32 portability though. If we do any lib, it should be glib. I'm just playing devil's advocate here in pointing out that adding DLL dependences on Windows is a bit painful although probably unavoidable. Regards, Anthony Liguori > Paolo
diff --git a/Makefile.objs b/Makefile.objs index 9e98a66..4303b95 100644 --- a/Makefile.objs +++ b/Makefile.objs @@ -14,7 +14,7 @@ oslib-obj-$(CONFIG_POSIX) += oslib-posix.o # block-obj-y is code used by both qemu system emulation and qemu-img block-obj-y = cutils.o cache-utils.o qemu-malloc.o qemu-option.o module.o -block-obj-y += nbd.o block.o aio.o aes.o qemu-config.o +block-obj-y += nbd.o block.o aio.o aes.o qemu-config.o qemu-ioh.o block-obj-$(CONFIG_POSIX) += posix-aio-compat.o block-obj-$(CONFIG_LINUX_AIO) += linux-aio.o diff --git a/qemu-char.h b/qemu-char.h index 56d9954..34936a7 100644 --- a/qemu-char.h +++ b/qemu-char.h @@ -7,6 +7,7 @@ #include "qemu-config.h" #include "qobject.h" #include "qstring.h" +#include "qemu-ioh.h" /* character device */ @@ -120,4 +121,7 @@ int qemu_set_fd_handler(int fd, IOHandler *fd_read, IOHandler *fd_write, void *opaque); +void qemu_get_fdset(int *nfds, fd_set *rfds, fd_set *wfds, fd_set *xfds); +void qemu_process_fd_handlers(const fd_set *rfds, const fd_set *wfds, + const fd_set *xfds); #endif diff --git a/qemu-ioh.c b/qemu-ioh.c new file mode 100644 index 0000000..cc71470 --- /dev/null +++ b/qemu-ioh.c @@ -0,0 +1,115 @@ +/* + * 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 "qemu-ioh.h" +#include "qlist.h" + +/* XXX: fd_read_poll should be suppressed, but an API change is + necessary in the character devices to suppress fd_can_read(). */ +int qemu_set_fd_handler3(void *ioh_record_list, + int fd, + IOCanReadHandler *fd_read_poll, + IOHandler *fd_read, + IOHandler *fd_write, + void *opaque) +{ + QLIST_HEAD(, IOHandlerRecord) *io_handlers_ptr = ioh_record_list; + IOHandlerRecord *ioh; + + if (!fd_read && !fd_write) { + QLIST_FOREACH(ioh, io_handlers_ptr, next) { + if (ioh->fd == fd) { + ioh->deleted = 1; + break; + } + } + } else { + QLIST_FOREACH(ioh, io_handlers_ptr, next) { + if (ioh->fd == fd) + goto found; + } + ioh = qemu_mallocz(sizeof(IOHandlerRecord)); + QLIST_INSERT_HEAD(io_handlers_ptr, 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; +} + +/* add entries from ioh record list to fd sets. nfds and fd sets + * should be cleared/reset by caller if desired. set a particular + * fdset to NULL to ignore fd events of that type + */ +void qemu_get_fdset2(void *ioh_record_list, int *nfds, fd_set *rfds, + fd_set *wfds, fd_set *xfds) +{ + QLIST_HEAD(, IOHandlerRecord) *io_handlers = ioh_record_list; + IOHandlerRecord *ioh; + + QLIST_FOREACH(ioh, io_handlers, next) { + if (ioh->deleted) + continue; + if ((rfds != NULL && 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 (wfds != NULL && ioh->fd_write) { + FD_SET(ioh->fd, wfds); + if (ioh->fd > *nfds) + *nfds = ioh->fd; + } + } +} + +/* execute registered handlers for r/w events in the provided fdsets. unset + * handlers are cleaned up here as well + */ +void qemu_process_fd_handlers2(void *ioh_record_list, const fd_set *rfds, + const fd_set *wfds, const fd_set *xfds) +{ + QLIST_HEAD(, IOHandlerRecord) *io_handlers = ioh_record_list; + IOHandlerRecord *ioh, *pioh; + + QLIST_FOREACH_SAFE(ioh, io_handlers, next, pioh) { + if (!ioh->deleted && ioh->fd_read && FD_ISSET(ioh->fd, rfds)) { + ioh->fd_read(ioh->opaque); + } + if (!ioh->deleted && ioh->fd_write && FD_ISSET(ioh->fd, wfds)) { + ioh->fd_write(ioh->opaque); + } + + /* Do this last in case read/write handlers marked it for deletion */ + if (ioh->deleted) { + QLIST_REMOVE(ioh, next); + qemu_free(ioh); + } + } +} diff --git a/qemu-ioh.h b/qemu-ioh.h new file mode 100644 index 0000000..7c6e833 --- /dev/null +++ b/qemu-ioh.h @@ -0,0 +1,34 @@ +#ifndef QEMU_IOH_H +#define QEMU_IOH_H + +#include "qemu-common.h" +#include "qlist.h" + +/* common i/o loop definitions */ + +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; + +/* XXX: fd_read_poll should be suppressed, but an API change is + necessary in the character devices to suppress fd_can_read(). */ +int qemu_set_fd_handler3(void *io_handlers_ptr, + int fd, + IOCanReadHandler *fd_read_poll, + IOHandler *fd_read, + IOHandler *fd_write, + void *opaque); +void qemu_get_fdset2(void *ioh_record_list, int *nfds, fd_set *rfds, + fd_set *wfds, fd_set *xfds); +void qemu_process_fd_handlers2(void *ioh_record_list, const fd_set *rfds, + const fd_set *wfds, const fd_set *xfds); + +#endif diff --git a/vl.c b/vl.c index b436952..dc90774 100644 --- a/vl.c +++ b/vl.c @@ -148,6 +148,7 @@ int main(int argc, char **argv) #include "qemu-config.h" #include "qemu-objects.h" #include "qemu-options.h" +#include "qemu-ioh.h" #ifdef CONFIG_VIRTFS #include "fsdev/qemu-fsdev.h" #endif @@ -1025,18 +1026,6 @@ void pcmcia_info(Monitor *mon) /***********************************************************/ /* I/O handling */ -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); @@ -1049,31 +1038,8 @@ int qemu_set_fd_handler2(int fd, 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; + return qemu_set_fd_handler3(&io_handlers, fd, fd_read_poll, fd_read, + fd_write, opaque); } int qemu_set_fd_handler(int fd, @@ -1084,6 +1050,17 @@ int qemu_set_fd_handler(int fd, return qemu_set_fd_handler2(fd, NULL, fd_read, fd_write, opaque); } +void qemu_get_fdset(int *nfds, fd_set *rfds, fd_set *wfds, fd_set *xfds) +{ + return qemu_get_fdset2(&io_handlers, nfds, rfds, wfds, xfds); +} + +void qemu_process_fd_handlers(const fd_set *rfds, const fd_set *wfds, + const fd_set *xfds) +{ + return qemu_process_fd_handlers2(&io_handlers, rfds, wfds, xfds); +} + /***********************************************************/ /* machine registration */ @@ -1326,7 +1303,6 @@ void qemu_system_vmstop_request(int reason) void main_loop_wait(int nonblocking) { - IOHandlerRecord *ioh; fd_set rfds, wfds, xfds; int ret, nfds; struct timeval tv; @@ -1347,22 +1323,7 @@ void main_loop_wait(int nonblocking) 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; - } - } + qemu_get_fdset(&nfds, &rfds, &wfds, &xfds); tv.tv_sec = timeout / 1000; tv.tv_usec = (timeout % 1000) * 1000; @@ -1373,22 +1334,7 @@ void main_loop_wait(int nonblocking) ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv); qemu_mutex_lock_iothread(); if (ret > 0) { - IOHandlerRecord *pioh; - - QLIST_FOREACH_SAFE(ioh, &io_handlers, next, pioh) { - if (!ioh->deleted && ioh->fd_read && FD_ISSET(ioh->fd, &rfds)) { - ioh->fd_read(ioh->opaque); - } - if (!ioh->deleted && ioh->fd_write && FD_ISSET(ioh->fd, &wfds)) { - ioh->fd_write(ioh->opaque); - } - - /* Do this last in case read/write handlers marked it for deletion */ - if (ioh->deleted) { - QLIST_REMOVE(ioh, next); - qemu_free(ioh); - } - } + qemu_process_fd_handlers(&rfds, &wfds, &xfds); } slirp_select_poll(&rfds, &wfds, &xfds, (ret < 0));
This allows us to implement an i/o loop outside of vl.c that can interact with objects that use qemu_set_fd_handler() Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> --- Makefile.objs | 2 +- qemu-char.h | 4 ++ qemu-ioh.c | 115 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ qemu-ioh.h | 34 +++++++++++++++++ vl.c | 86 ++++++++---------------------------------- 5 files changed, 170 insertions(+), 71 deletions(-) create mode 100644 qemu-ioh.c create mode 100644 qemu-ioh.h