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

2011-01-23 Thread Jes Sorensen 
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
 +#includesys/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.

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

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

2011-01-21 Thread Jes Sorensen 
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.

 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 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 :)

Cheers,
Jes

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

2011-01-21 Thread 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
+#includesys/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 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
+#includesys/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