[Qemu-devel] broken socket events on win32 qemu

[Qemu-devel] broken socket events on win32 qemu

[Andrew Baumann]

Hi Daniel,

This commit ("char: convert from GIOChannel to QIOChannel"):
https://github.com/qemu/qemu/commit/9894dc0cdcc397ee5b26370bc53da6d360a363c2
... appears to have broken socket events for character devices on Win32. For example, I can no longer connect to a GDB stub (started with: "-gdb tcp:127.0.0.1:1234"), since tcp_chr_accept is never called.

Without having looked very closely at the code, I suspect the problem may be that we've lost the special-case treatment of socket handles as distinct from file descriptors on Win32 (they are different namespaces, and different APIs are needed). The previous version of qemu-char.c special-cased sockets in io_channel_from_socket():

-#ifdef _WIN32
-    chan = g_io_channel_win32_new_socket(fd);
-#else
-    chan = g_io_channel_unix_new(fd);
-#endif

... but I don't see anything equivalent in io/channel-socket.c. Am I looking in the wrong place?

BTW, The same change introduces another problem on win32: server sockets like the GDB example above fail on getpeername() with "Unable to query remote socket address: Unknown error". This seems to be caused by a definition of ENOTCONN that is not WSAENOTCONN. I'm still trying to figure out why that is, and how to best fix it.

Regards,
Andrew

Re: [Qemu-devel] broken socket events on win32 qemu

[Paolo Bonzini]

On 07/03/2016 08:23, Andrew Baumann wrote:
> Without having looked very closely at the code, I suspect the problem
> may be that we've lost the special-case treatment of socket handles as
> distinct from file descriptors on Win32 (they are different namespaces,
> and different APIs are needed). The previous version of qemu-char.c
> special-cased sockets in io_channel_from_socket():
> 
> -#ifdef _WIN32
> -    chan = g_io_channel_win32_new_socket(fd);
> -#else
> -    chan = g_io_channel_unix_new(fd);
> -#endif
> 
> ... but I don't see anything equivalent in io/channel-socket.c. Am I
> looking in the wrong place?

You're right, we need to add a qio_channel_create_socket_watch, which
just calls qio_channel_create_fd_watch on Unix but uses select and
WSAEnumNetworkEvents on Win32.  I have already implemented this trick in
aio-win32.c for networked block devices.

Andrew or Daniel, it would be great if you added a failing testcase to
test-io-channel-socket.c.  I can work on the fix myself.

Paolo

Re: [Qemu-devel] broken socket events on win32 qemu

[Daniel P. Berrange]

On Mon, Mar 07, 2016 at 07:23:12AM +0000, Andrew Baumann wrote:
> Hi Daniel,
> 
> This commit ("char: convert from GIOChannel to QIOChannel"):
> https://github.com/qemu/qemu/commit/9894dc0cdcc397ee5b26370bc53da6d360a363c2
> ... appears to have broken socket events for character devices on Win32.
> For example, I can no longer connect to a GDB stub (started with:
> "-gdb tcp:127.0.0.1:1234"), since tcp_chr_accept is never called.
> 
> Without having looked very closely at the code, I suspect the problem may
> be that we've lost the special-case treatment of socket handles as distinct
> from file descriptors on Win32 (they are different namespaces, and different
> APIs are needed). The previous version of qemu-char.c special-cased sockets
> in io_channel_from_socket():
> 
> -#ifdef _WIN32
> -    chan = g_io_channel_win32_new_socket(fd);
> -#else
> -    chan = g_io_channel_unix_new(fd);
> -#endif
> 
> ... but I don't see anything equivalent in io/channel-socket.c. Am I looking
> in the wrong place?

No, you are correct, this is broken for the reason you describe. Seems
this is the one key feature I forgot to add unit test coverage for :-(

> BTW, The same change introduces another problem on win32: server sockets
> like the GDB example above fail on getpeername() with "Unable to query
>  remote socket address: Unknown error". This seems to be caused by a
> definition of ENOTCONN that is not WSAENOTCONN. I'm still trying to
> figure out why that is, and how to best fix it.

Can you say how you are building QEMU ? Are you using mingw to do a cross
compile for Win32, or something else ?

Looking at my local Mingw64 install, the errno definitions look potentially
problematic· The QEMU  socket_error() method is quite crude - it simply
expands to WSAGetLastError(), so any code calling it is assuming that the
WSAExxxxx constants match the Exxxx constants. QEMU has a header which
sets up such a mapping, but it only does so conditionally. eg in
include/sysemu/os-win32.h

  #ifndef ENOTCONN
  # define ENOTCONN     WSAENOTCONN
  #endif

The current versions of mingw64 I have installed though has a winerror.h
which defines

  #define WSABASEERR 10000
  #define WSAENOTCONN (WSABASEERR + 57)

And a separate  errno.h that defines

  #ifndef ENOTCONN
  #define ENOTCONN 126
  #endif

This obviously does not match the WSAENOTCONN value

So my guess would be that QEMU is pulling in the mingw64 errno.h values
and so QEMU's own  os-win32.h hack is not getting activated.

Really, I think the problem is QEMU's socket_error() compat wrapper. It
is fundamentally not reliable to assume WSAExxxx == Exxxx values, which
is what socket_error() forces callers todo.

I think we should we re-implement socket_error() for win32 to do this

   int socket_error(void) {
       switch (WSAGetLastError()) {
           case WSAENOTCONN:
	       return ENOTCONN;
	   case WSAECONNREFUSED:
	       return ECONNREFUSED;
	   case WSAEBADF:
	       return EBADF;
	    ....etc for the other errno mpappings w eneed to care about...
       }
   }

this is what GLib itself does internally for addressing this problem
(see g_io_error_from_win32_error in gio/gioerror.c)

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|

Re: [Qemu-devel] broken socket events on win32 qemu

[Paolo Bonzini]

On 07/03/2016 11:19, Daniel P. Berrange wrote:
> On Mon, Mar 07, 2016 at 07:23:12AM +0000, Andrew Baumann wrote:
>> Hi Daniel,
>>
>> This commit ("char: convert from GIOChannel to QIOChannel"):
>> https://github.com/qemu/qemu/commit/9894dc0cdcc397ee5b26370bc53da6d360a363c2
>> ... appears to have broken socket events for character devices on Win32.
>> For example, I can no longer connect to a GDB stub (started with:
>> "-gdb tcp:127.0.0.1:1234"), since tcp_chr_accept is never called.
>>
>> Without having looked very closely at the code, I suspect the problem may
>> be that we've lost the special-case treatment of socket handles as distinct
>> from file descriptors on Win32 (they are different namespaces, and different
>> APIs are needed). The previous version of qemu-char.c special-cased sockets
>> in io_channel_from_socket():
>>
>> -#ifdef _WIN32
>> -    chan = g_io_channel_win32_new_socket(fd);
>> -#else
>> -    chan = g_io_channel_unix_new(fd);
>> -#endif
>>
>> ... but I don't see anything equivalent in io/channel-socket.c. Am I looking
>> in the wrong place?
> 
> No, you are correct, this is broken for the reason you describe. Seems
> this is the one key feature I forgot to add unit test coverage for :-(
> 
>> BTW, The same change introduces another problem on win32: server sockets
>> like the GDB example above fail on getpeername() with "Unable to query
>>  remote socket address: Unknown error". This seems to be caused by a
>> definition of ENOTCONN that is not WSAENOTCONN. I'm still trying to
>> figure out why that is, and how to best fix it.
> 
> Can you say how you are building QEMU ? Are you using mingw to do a cross
> compile for Win32, or something else ?

FWIW, Wine works fine for me.  I just copy /mingw from Fedora's sysroot
into Wine's C:\mingw.

Paolo

Re: [Qemu-devel] broken socket events on win32 qemu

[Andrew Baumann]

> From: Daniel P. Berrange [mailto:berrange@redhat.com]
> Sent: Monday, 7 March 2016 2:19 AM
> 
> On Mon, Mar 07, 2016 at 07:23:12AM +0000, Andrew Baumann wrote:
> > BTW, The same change introduces another problem on win32: server
> sockets
> > like the GDB example above fail on getpeername() with "Unable to query
> >  remote socket address: Unknown error". This seems to be caused by a
> > definition of ENOTCONN that is not WSAENOTCONN. I'm still trying to
> > figure out why that is, and how to best fix it.
> 
> Can you say how you are building QEMU ? Are you using mingw to do a cross
> compile for Win32, or something else ?

I'm building natively using an unhealthy mixture of mingw-w64 and gcc installed from win-builds, 32-bit tools from a native mingw/msys installation, and probably some Cygwin stuff that leaked into my PATH as well. (It's a setup that evolved, not one I'd advise replicating! :)

> Looking at my local Mingw64 install, the errno definitions look potentially
> problematic· The QEMU  socket_error() method is quite crude - it simply
> expands to WSAGetLastError(), so any code calling it is assuming that the
> WSAExxxxx constants match the Exxxx constants. QEMU has a header which
> sets up such a mapping, but it only does so conditionally. eg in
> include/sysemu/os-win32.h
> 
>   #ifndef ENOTCONN
>   # define ENOTCONN     WSAENOTCONN
>   #endif
> 
> The current versions of mingw64 I have installed though has a winerror.h
> which defines
> 
>   #define WSABASEERR 10000
>   #define WSAENOTCONN (WSABASEERR + 57)
> 
> And a separate  errno.h that defines
> 
>   #ifndef ENOTCONN
>   #define ENOTCONN 126
>   #endif
> 
> This obviously does not match the WSAENOTCONN value
> 
> So my guess would be that QEMU is pulling in the mingw64 errno.h values
> and so QEMU's own  os-win32.h hack is not getting activated.

Yes, I’m seeing the same thing. I will test your patch, but it looks like the right thing to do for the socket errors.

Cheers,
Andrew