| Message ID | 1291399402-20366-2-git-send-email-mdroth@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 12/03/10 19:03, 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() > > Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com> This commit message really tells us nothing. Please be more specific about what is in the commit. > 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: Is this moved or new code? If the former, fine, but if it is new code, you might want to leave your own name on the (c). I presume at least some of the changes are (c) 2010? > +/* XXX: fd_read_poll should be suppressed, but an API change is > + necessary in the character devices to suppress fd_can_read(). */ XXX in the comment isn't really of much use. Please make it more explicit, or put your name in if it is a statement you wish to make. > +int qemu_set_fd_handler3(void *ioh_record_list, > + int fd, > + IOCanReadHandler *fd_read_poll, > + IOHandler *fd_read, > + IOHandler *fd_write, > + void *opaque) I am not happy with this addition of numbers to these functions, it doesn't tell us why we have a 3 and how it differs from 2. If 3 is really the backend for implementing 2, maybe it would be better to name it __qemu_set_fd_handler2() and then have macros/wrappers calling into it. Cheers, Jes
On 12/07/2010 07:31 AM, Jes Sorensen wrote: > On 12/03/10 19:03, 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() >> >> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com> > > This commit message really tells us nothing. Please be more specific > about what is in the commit. > Currently, in qemu, the virtagent client/server functionality is driven by vl.c:main_loop_wait(), which implements a select() loop that kicks off handlers registered for various FDs/events via qemu_set_fd_handler(). To share the code with the agent, qemu-va.c, I re-implemented this i/o loop to do same thing, along with vl.c:qemu_set_fd_handler*() and friends. It was big nasty copy/paste job and I think most of the reviewers agreed that the i/o loop code should be shared. This commit moves the shared code into back-end utility functions that get called by vl.c:qemu_set_fd_handler()/qemu_process_fd_handlers() and friends for qemu, and by qemu-tools.c:qemu_set_fd_handler()/qemu_process_fd_handlers() for tools. So now to interact with code that uses qemu_set_fd_handler() you can built a select() loop around these utility functions. >> 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: > > Is this moved or new code? If the former, fine, but if it is new code, > you might want to leave your own name on the (c). I presume at least > some of the changes are (c) 2010? > It's moved out from vl.c, nearly verbatim... >> +/* XXX: fd_read_poll should be suppressed, but an API change is >> + necessary in the character devices to suppress fd_can_read(). */ > > XXX in the comment isn't really of much use. Please make it more > explicit, or put your name in if it is a statement you wish to make. > Even down to the comments :) >> +int qemu_set_fd_handler3(void *ioh_record_list, >> + int fd, >> + IOCanReadHandler *fd_read_poll, >> + IOHandler *fd_read, >> + IOHandler *fd_write, >> + void *opaque) > > I am not happy with this addition of numbers to these functions, it > doesn't tell us why we have a 3 and how it differs from 2. If 3 is > really the backend for implementing 2, maybe it would be better to name > it __qemu_set_fd_handler2() and then have macros/wrappers calling into it. > That was the initial plan, but qemu_set_fd_handler2() is a back-end of sorts for qemu_set_fd_handler(), so I was just trying to be consistent with the naming. Personally I don't have any objections one way or the other. > Cheers, > Jes >
On 12/07/10 15:48, Michael Roth wrote: > On 12/07/2010 07:31 AM, Jes Sorensen wrote: >> On 12/03/10 19:03, 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() >>> >>> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com> >> >> This commit message really tells us nothing. Please be more specific >> about what is in the commit. >> > > Currently, in qemu, the virtagent client/server functionality is driven > by vl.c:main_loop_wait(), which implements a select() loop that kicks > off handlers registered for various FDs/events via qemu_set_fd_handler(). > > To share the code with the agent, qemu-va.c, I re-implemented this i/o > loop to do same thing, along with vl.c:qemu_set_fd_handler*() and > friends. It was big nasty copy/paste job and I think most of the > reviewers agreed that the i/o loop code should be shared. > > This commit moves the shared code into back-end utility functions that > get called by vl.c:qemu_set_fd_handler()/qemu_process_fd_handlers() and > friends for qemu, and by > qemu-tools.c:qemu_set_fd_handler()/qemu_process_fd_handlers() for tools. > > So now to interact with code that uses qemu_set_fd_handler() you can > built a select() loop around these utility functions. Please put some of this in the commit message. >> I am not happy with this addition of numbers to these functions, it >> doesn't tell us why we have a 3 and how it differs from 2. If 3 is >> really the backend for implementing 2, maybe it would be better to name >> it __qemu_set_fd_handler2() and then have macros/wrappers calling into >> it. > > That was the initial plan, but qemu_set_fd_handler2() is a back-end of > sorts for qemu_set_fd_handler(), so I was just trying to be consistent > with the naming. Personally I don't have any objections one way or the > other. Anything to avoid qemu_set_fd_handler17() at some point. I think using __qemu_set_fd_handler() encourages people to modify that code rather than copy it. Cheers, Jes
On Tue, Dec 07, 2010 at 04:02:03PM +0100, Jes Sorensen wrote: > On 12/07/10 15:48, Michael Roth wrote: > > On 12/07/2010 07:31 AM, Jes Sorensen wrote: > >> On 12/03/10 19:03, Michael Roth wrote: > >> I am not happy with this addition of numbers to these functions, it > >> doesn't tell us why we have a 3 and how it differs from 2. If 3 is > >> really the backend for implementing 2, maybe it would be better to name > >> it __qemu_set_fd_handler2() and then have macros/wrappers calling into > >> it. > > > > That was the initial plan, but qemu_set_fd_handler2() is a back-end of > > sorts for qemu_set_fd_handler(), so I was just trying to be consistent > > with the naming. Personally I don't have any objections one way or the > > other. > > Anything to avoid qemu_set_fd_handler17() at some point. I think using > __qemu_set_fd_handler() encourages people to modify that code rather > than copy it. I agree that qemu_set_fd_handler3() could be named something more meaningful. Unfortunately we can't use __qemu_set_fd_handler() because the C language standard reserves identifiers that start with double underscore for the standard library. The Linux kernel gets away with it because the code is freestanding but that doesn't apply to QEMU. Stefan
On 12/08/10 10:15, Stefan Hajnoczi wrote: > On Tue, Dec 07, 2010 at 04:02:03PM +0100, Jes Sorensen wrote: >> Anything to avoid qemu_set_fd_handler17() at some point. I think using >> __qemu_set_fd_handler() encourages people to modify that code rather >> than copy it. > > I agree that qemu_set_fd_handler3() could be named something more > meaningful. Unfortunately we can't use __qemu_set_fd_handler() because > the C language standard reserves identifiers that start with double > underscore for the standard library. The Linux kernel gets away with it > because the code is freestanding but that doesn't apply to QEMU. Hmmm you sure that is actually a rule, rather than a convention? Either way, we can use 3 underscores, or leave the function static, in which case the C library names are a non issue. Cheers, Jes
On Wed, Dec 08, 2010 at 10:17:50AM +0100, Jes Sorensen wrote: > On 12/08/10 10:15, Stefan Hajnoczi wrote: > > On Tue, Dec 07, 2010 at 04:02:03PM +0100, Jes Sorensen wrote: > >> Anything to avoid qemu_set_fd_handler17() at some point. I think using > >> __qemu_set_fd_handler() encourages people to modify that code rather > >> than copy it. > > > > I agree that qemu_set_fd_handler3() could be named something more > > meaningful. Unfortunately we can't use __qemu_set_fd_handler() because > > the C language standard reserves identifiers that start with double > > underscore for the standard library. The Linux kernel gets away with it > > because the code is freestanding but that doesn't apply to QEMU. > > Hmmm you sure that is actually a rule, rather than a convention? > > Either way, we can use 3 underscores, or leave the function static, in > which case the C library names are a non issue. From 7.1.3 Reserved identifiers: "All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use." and "All identifiers that begin with an underscore are always reserved for use as identifiers with file scope in both the ordinary and tag name spaces." That includes three or more underscores too. Stefan
On 12/08/10 10:23, Stefan Hajnoczi wrote: > From 7.1.3 Reserved identifiers: > > "All identifiers that begin with an underscore and either an uppercase > letter or another underscore are always reserved for any use." > > and > > "All identifiers that begin with an underscore are always reserved for > use as identifiers with file scope in both the ordinary and tag name > spaces." > > That includes three or more underscores too. Ok, I never hit problems with this, but ok we can name it do_qemu_set_fd_handler() instead. That would go with the existing naming conventions used in many places throughout the code. Cheers, Jes
On 12/08/2010 03:15 AM, Stefan Hajnoczi wrote: > On Tue, Dec 07, 2010 at 04:02:03PM +0100, Jes Sorensen wrote: > >> On 12/07/10 15:48, Michael Roth wrote: >> >>> On 12/07/2010 07:31 AM, Jes Sorensen wrote: >>> >>>> On 12/03/10 19:03, Michael Roth wrote: >>>> I am not happy with this addition of numbers to these functions, it >>>> doesn't tell us why we have a 3 and how it differs from 2. If 3 is >>>> really the backend for implementing 2, maybe it would be better to name >>>> it __qemu_set_fd_handler2() and then have macros/wrappers calling into >>>> it. >>>> >>> That was the initial plan, but qemu_set_fd_handler2() is a back-end of >>> sorts for qemu_set_fd_handler(), so I was just trying to be consistent >>> with the naming. Personally I don't have any objections one way or the >>> other. >>> >> Anything to avoid qemu_set_fd_handler17() at some point. I think using >> __qemu_set_fd_handler() encourages people to modify that code rather >> than copy it. >> > I agree that qemu_set_fd_handler3() could be named something more > meaningful. Unfortunately we can't use __qemu_set_fd_handler() because > the C language standard reserves identifiers that start with double > underscore for the standard library. The Linux kernel gets away with it > because the code is freestanding but that doesn't apply to QEMU. > fdset_set_handler() or: iohandler_set_handler() Take whatever the structure is that represents the global state, convert it's name to lower case, and use that as the prefix. If this were C++, the namespacing would be obvious because you wouldn't have a choice about how to design the namespace ;-) /me ducks Regards, Anthony Liguori > Stefan >
diff --git a/Makefile.objs b/Makefile.objs index 23b17ce..403bbb0 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 18ad12b..9809ef6 100644 --- a/qemu-char.h +++ b/qemu-char.h @@ -6,6 +6,7 @@ #include "qemu-option.h" #include "qemu-config.h" #include "qobject.h" +#include "qemu-ioh.h" /* character device */ @@ -111,4 +112,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 805e11f..d7024bc 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 @@ -914,18 +915,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); @@ -938,31 +927,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, @@ -973,6 +939,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 */ @@ -1202,7 +1179,6 @@ void qemu_system_powerdown_request(void) void main_loop_wait(int nonblocking) { - IOHandlerRecord *ioh; fd_set rfds, wfds, xfds; int ret, nfds; struct timeval tv; @@ -1223,22 +1199,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; @@ -1249,22 +1210,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