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

[Michael Roth <mdroth@linux.vnet.ibm.com>]

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