[Qemu-devel] Re: [RFC][PATCH v6 03/23] Make qemu timers available for tools

[Michael Roth]

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<sys/eventfd.h>
> > +#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 0000000..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<sys/eventfd.h>
> > +#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