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

[Fabien Chouteau <chouteau@adacore.com>]

On 22/08/2011 15:47, Paolo Bonzini wrote:
> On 08/22/2011 03:45 PM, Anthony Liguori wrote:
>>>
>>> Almost: in Win32 you need to use g_io_channel_win32_new_socket. But
>>> indeed on Windows you can only use qemu_set_fd_handler for sockets too.
>>
>> I think that's really only for read/write though.  If you're just
>> polling on I/O, it shouldn't matter IIUC.
>>
>> If someone has a Windows box, they can confirm/deny by using qemu
>> -monitor tcp:localhost:1024,socket,nowait with this patch.
> 
> Actually you're right, it works automagically:
> 
>  * On Win32, this can be used either for files opened with the MSVCRT
>  * (the Microsoft run-time C library) _open() or _pipe, including file
>  * descriptors 0, 1 and 2 (corresponding to stdin, stdout and stderr),
>  * or for Winsock SOCKETs. If the parameter is a legal file
>  * descriptor, it is assumed to be such, otherwise it should be a
>  * SOCKET. This relies on SOCKETs and file descriptors not
>  * overlapping. If you want to be certain, call either
>  * g_io_channel_win32_new_fd() or g_io_channel_win32_new_socket()
>  * instead as appropriate.
> 
> So this patch would even let interested people enable exec migration on Windows.
> 

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

WSAENOTSOCK - Error 10038 - An operation was attempted on something that
is not a socket. The specified socket parameter refers to a file, not a
socket.

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?

Regards,

-- 
Fabien Chouteau