[Qemu-devel] Re: [RFC][PATCH v6 03/23] Make qemu timers available for tools
On 01/17/11 14:14, Michael Roth wrote: > diff --git a/qemu-ioh.c b/qemu-ioh.c > index cc71470..001e7a2 100644 > --- a/qemu-ioh.c > +++ b/qemu-ioh.c > @@ -22,7 +22,11 @@ > * THE SOFTWARE. > */ > #include "qemu-ioh.h" > +#include "qemu-char.h" > #include "qlist.h" > +#ifdef CONFIG_EVENTFD > +#include > +#endif > > /* XXX: fd_read_poll should be suppressed, but an API change is > necessary in the character devices to suppress fd_can_read(). */ > @@ -113,3 +117,92 @@ void qemu_process_fd_handlers2(void *ioh_record_list, > const fd_set *rfds, > } > } > } > + > +#ifndef _WIN32 > +void iothread_event_increment(int *io_thread_fd) Please split the WIN32 stuff into it's own file, similar to oslib-posix and oslib-win32.c etc. > diff --git a/qemu-ioh.h b/qemu-ioh.h > index 7c6e833..2c714a9 100644 > --- a/qemu-ioh.h > +++ b/qemu-ioh.h > @@ -31,4 +31,13 @@ void qemu_get_fdset2(void *ioh_record_list, int *nfds, > fd_set *rfds, > void qemu_process_fd_handlers2(void *ioh_record_list, const fd_set *rfds, > const fd_set *wfds, const fd_set *xfds); > > + > +#ifndef _WIN32 > +void iothread_event_increment(int *io_thread_fd); > +int iothread_event_init(int *io_thread_fd); > +#else > +int win32_event_init(HANDLE *qemu_event_handle); > +void win32_event_increment(HANDLE *qemu_event_handle); > +#endif Can you not do something slightly nicer that allows for those to be the same prototype for all users? Like define a event_handle_t? > + > +#ifndef _WIN32 > +static int io_thread_fd = -1; Needs splitting into separate files too. > diff --git a/qemu-tool.h b/qemu-tool.h > new file mode 100644 > index 000..fd693cf > --- /dev/null > +++ b/qemu-tool.h > @@ -0,0 +1,26 @@ > +#ifndef QEMU_TOOL_H > +#define QEMU_TOOL_H > + > +#include "qemu-common.h" > + > +#ifdef CONFIG_EVENTFD > +#include > +#endif > + > +typedef void VMStateDescription; > +typedef int VMStateInfo; > + > +#ifndef _WIN32 > +void qemu_event_increment(void); > +int qemu_event_init(void); > +#else > +int qemu_event_init(void); > +void qemu_event_increment(void); > +#endif No matter how long I stare at those prototypes, I fail to see the difference between the win32 and the posix version :) Cheers, Jes
[Qemu-devel] Re: [RFC][PATCH v6 03/23] Make qemu timers available for tools
On 01/21/2011 10:30 AM, Jes Sorensen wrote: On 01/17/11 14:14, Michael Roth wrote: diff --git a/qemu-ioh.c b/qemu-ioh.c index cc71470..001e7a2 100644 --- a/qemu-ioh.c +++ b/qemu-ioh.c @@ -22,7 +22,11 @@ * THE SOFTWARE. */ #include "qemu-ioh.h" +#include "qemu-char.h" #include "qlist.h" +#ifdef CONFIG_EVENTFD +#include +#endif /* XXX: fd_read_poll should be suppressed, but an API change is necessary in the character devices to suppress fd_can_read(). */ @@ -113,3 +117,92 @@ void qemu_process_fd_handlers2(void *ioh_record_list, const fd_set *rfds, } } } + +#ifndef _WIN32 +void iothread_event_increment(int *io_thread_fd) Please split the WIN32 stuff into it's own file, similar to oslib-posix and oslib-win32.c etc. Will look into this, but qemu-ioh.c has common code too so we'd end up with qemu-ioh/qemu-ioh-posix/qemu-ioh-win2.c. We could alternatively have a "#ifndef _WIN32" around functions in qemu-ioh.c that would be replaced by win32-specific ones from qemu-ioh-win32. No strong preference either way, but sometimes I find navigating across too many files more annoying that #ifdefs, and there's not a whole lot in these. diff --git a/qemu-ioh.h b/qemu-ioh.h index 7c6e833..2c714a9 100644 --- a/qemu-ioh.h +++ b/qemu-ioh.h @@ -31,4 +31,13 @@ void qemu_get_fdset2(void *ioh_record_list, int *nfds, fd_set *rfds, void qemu_process_fd_handlers2(void *ioh_record_list, const fd_set *rfds, const fd_set *wfds, const fd_set *xfds); + +#ifndef _WIN32 +void iothread_event_increment(int *io_thread_fd); +int iothread_event_init(int *io_thread_fd); +#else +int win32_event_init(HANDLE *qemu_event_handle); +void win32_event_increment(HANDLE *qemu_event_handle); +#endif Can you not do something slightly nicer that allows for those to be the same prototype for all users? Like define a event_handle_t? Don't see why not. + +#ifndef _WIN32 +static int io_thread_fd = -1; Needs splitting into separate files too. diff --git a/qemu-tool.h b/qemu-tool.h new file mode 100644 index 000..fd693cf --- /dev/null +++ b/qemu-tool.h @@ -0,0 +1,26 @@ +#ifndef QEMU_TOOL_H +#define QEMU_TOOL_H + +#include "qemu-common.h" + +#ifdef CONFIG_EVENTFD +#include +#endif + +typedef void VMStateDescription; +typedef int VMStateInfo; + +#ifndef _WIN32 +void qemu_event_increment(void); +int qemu_event_init(void); +#else +int qemu_event_init(void); +void qemu_event_increment(void); +#endif No matter how long I stare at those prototypes, I fail to see the difference between the win32 and the posix version :) Heh, the ordering of course! :) Not sure how I missed this one. The patch is pretty rough in general, I'll see what I can do about cleaning things up a bit. Cheers, Jes
[Qemu-devel] Re: [RFC][PATCH v6 03/23] Make qemu timers available for tools
On 01/21/11 18:26, Michael Roth wrote: > On 01/21/2011 10:30 AM, Jes Sorensen wrote: >> On 01/17/11 14:14, Michael Roth wrote: >>> diff --git a/qemu-ioh.c b/qemu-ioh.c >>> index cc71470..001e7a2 100644 >>> --- a/qemu-ioh.c >>> +++ b/qemu-ioh.c >>> @@ -22,7 +22,11 @@ >>>* THE SOFTWARE. >>>*/ >>> #include "qemu-ioh.h" >>> +#include "qemu-char.h" >>> #include "qlist.h" >>> +#ifdef CONFIG_EVENTFD >>> +#include >>> +#endif >>> >>> /* XXX: fd_read_poll should be suppressed, but an API change is >>> necessary in the character devices to suppress fd_can_read(). */ >>> @@ -113,3 +117,92 @@ void qemu_process_fd_handlers2(void >>> *ioh_record_list, const fd_set *rfds, >>> } >>> } >>> } >>> + >>> +#ifndef _WIN32 >>> +void iothread_event_increment(int *io_thread_fd) >> >> Please split the WIN32 stuff into it's own file, similar to oslib-posix >> and oslib-win32.c etc. > > Will look into this, but qemu-ioh.c has common code too so we'd end up > with qemu-ioh/qemu-ioh-posix/qemu-ioh-win2.c. We could alternatively > have a "#ifndef _WIN32" around functions in qemu-ioh.c that would be > replaced by win32-specific ones from qemu-ioh-win32. No strong > preference either way, but sometimes I find navigating across too many > files more annoying that #ifdefs, and there's not a whole lot in these. No problem having the three files - it is far better than having #ifdefs. Having the #ifndef that is overloaded by a win32 specific file is bad, it will make it very confusing for anyone reading the code. Cheers, Jes
