Re: [Qemu-devel] [PATCH 2/2] main: switch qemu_set_fd_handler to g_io_add_watch

[Fabien Chouteau]

On 24/11/2011 18:30, Paolo Bonzini wrote:
> On 11/24/2011 06:11 PM, Fabien Chouteau wrote:
>> Hello,
>>
>> I've run into some problems with this patch on Windows. The thing is
>> that select() should be used only with socket file descriptors.
>>
>> If glib_select_fill() put a non-socket file descriptor in rfds or wfds,
>> select() will fail with this error (btw the return value of select is
>> not checked):
> 
> This patch actually has been reverted in commit be08e65.  Is it the revert 
> that is causing problems?  If so, what is it that glib_select_fill puts in 
> rfds and wfds?
> 

OK my bad, I should have checked on the upstream repository.
But I can see that glib_select_fill/poll() are still there.

>> I've look at the patch and I don't see why do you pick file descriptors
>> from g_main_context_query's "poll_fds" to put them in the fd_sets (rfds,
>> wfds...) and then re-build a "poll_fds" to call g_main_context_check and
>> g_main_context_dispatch. From my understanding we can just do:
>>
>> g_main_context_prepare(context,&max_priority);
>>
>> n_poll_fds = g_main_context_query(context, max_priority,&timeout,
>>                                        poll_fds, ARRAY_SIZE(poll_fds));
>>
>> if (g_main_context_check(context, max_priority, poll_fds, n_poll_fds)) {
>>      g_main_context_dispatch(context);
>> }
>>
>> Or even just call g_main_context_iteration(). What do you think?
> 
> You would have to call it in nonblocking mode from a polling handler 
> (qemu_add_polling_cb).
> 
> A better solution is to move the whole main loop polling into 
> os_host_main_loop_wait.
> 
> For POSIX, it would be just a call to 
> glib_select_fill+select+glib_select_poll.  (Everything around these three 
> would stay in the caller, and the fd_sets would be passed to 
> os_host_main_loop_wait).

Are you sure we have to use select()? I would expect Glib to help us
avoid this kind of os-dependent syscalls.

> 
> For Windows, it would work like this (and would not use glib_select_* at all):
> 
> 1) call the polling handlers;
> 
> 2) call select with timeout zero.  If no socket is ready, call WSAEventSelect 
> on the sockets listed in the fd_sets;
> 
> 3) call g_main_context_prepare+query.
> 
> 4) add the event from (2) and the registered wait objects to the poll_fds.  
> Call g_poll on it.  If sockets were ready, force 0 timeout.
> 
> 5) If no sockets were ready, call again select with timeout zero.
> 
> 6) Check the output of g_poll and dispatch the wait objects that are now 
> ready.
> 
> 7) Call g_main_context_check+dispatch.


Again, Glib should help us skip all these complicated os-dependent stuff.

Maybe it's already the plan and I don't want to beat a dead horse, but I
think the best way is to get rid of file descriptors and sockets to use
GIOChannel all the way. Not only for event loop, but also for reads and
writes.

Regards,

-- 
Fabien Chouteau