[Qemu-devel] [PATCH v2 00/18] Multiple fixes & improvements to QIOChannel & Win32

[Qemu-devel] [PATCH v2 00/18] Multiple fixes & improvements to QIOChannel & Win32

[Daniel P. Berrange]

This series started out as an attempt to fix the Win32 problems
identified by Andrew Baumann

   https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg01343.html

It turned into a significantly larger cleanup of some chardev
and osdep win32 portability code.

Patch 1 addresses Andrew's 2nd stated problem - handling of
getpeername() failures, by fixing errno handling on Win32.

Patches 2-7 do some fixes in the test-io-channel-socket test
case so that it is able to run on Win32.

Patches 8-12 are some fixes for the QIOChannel code

Patch 13 is the big one that changes QIOChannelSocket so that
it uses a Win32 specific GSource implementation for creating
watches. This is the key fix for Andrew's 1st stated problem.

At this point tests/test-io-channel-socket passes and

  qemu-system-x86_64.exe  -serial tcp:127.0.0.1:9000,server,nowait -device sga -display non

works on win32 once more.

Patches 14-16 are some cleanups to the chardev code to improve
its clarity. They are not required for fixing any real problem

Patches 17-18 change the way we provide Win32 portability for
sockets APIs inside QEMU. These do fix a number of bugs in the
QEMU code related to mistaken use of errno instead of
socket_error(). None of these bugs appear to be critical issues.

Based on this, I'm proposing 1-13 for QEMU 2.6 release as they
fix critical win32 bugs.

Patches 14-18 can either be included in 2.6 or 2.7 - I'm
ambivalent on which, since they're cleanups / minor fixes.

Changed in v2:

 - Set event flags in qio_channel_socket_set_blocking
   instead of when creating GSource
 - Drop now uneeded patches that set QIOChanel to blocking mode
 - Change WSAEWOULDBLOCK into EAGAIN to avoid needing to check
   EWOULDBLOCK
 - Drop patches that replaced qemu_accept/qemu_socket
 - Drop wrapping of close/ioctl in favour of closesocket/
   ioctlsocket.

Daniel P. Berrange (15):
  osdep: fix socket_error() to work with Mingw64
  io: use bind() to check for IPv4/6 availability
  io: initialize sockets in test program
  io: bind to socket before creating QIOChannelSocket
  io: wait for incoming client in socket test
  io: set correct error object in background reader test thread
  io: assert errors before asserting content in I/O test
  io: fix copy+paste mistake in socket error message
  io: use qemu_accept to ensure SOCK_CLOEXEC is set
  io: remove checking of EWOULDBLOCK
  char: remove qemu_chr_finish_socket_connection method
  char: remove socket_try_connect method
  char: remove qemu_chr_open_socket_fd method
  osdep: add wrappers for socket functions
  osdep: remove use of socket_error() from all code

Paolo Bonzini (3):
  io: pass HANDLE to g_source_add_poll on Win32
  io: introduce qio_channel_create_socket_watch
  io: implement socket watch for win32 using WSAEventSelect+select

 Makefile                       |   4 +-
 block/sheepdog.c               |   5 +-
 include/io/channel-watch.h     |  20 ++-
 include/io/channel.h           |   3 +
 include/qemu/sockets.h         |  17 ---
 include/sysemu/os-posix.h      |   9 ++
 include/sysemu/os-win32.h      | 106 ++++++++++++----
 io/channel-command.c           |   6 +-
 io/channel-file.c              |   6 +-
 io/channel-socket.c            |  84 ++++++++-----
 io/channel-watch.c             | 152 +++++++++++++++++++++-
 io/channel.c                   |  14 +++
 linux-user/flatload.c          |   1 -
 migration/qemu-file-unix.c     |  14 +--
 migration/tcp.c                |   7 +-
 net/socket.c                   |  19 ++-
 qemu-char.c                    |  96 ++++++--------
 slirp/slirp.h                  |   2 -
 slirp/tcp_input.c              |   4 -
 tests/io-channel-helpers.c     |   6 +-
 tests/test-io-channel-socket.c |  92 +++++++-------
 util/oslib-win32.c             | 280 ++++++++++++++++++++++++++++++++++++++++-
 util/qemu-coroutine-io.c       |   6 +-
 util/qemu-sockets.c            |  10 +-
 24 files changed, 719 insertions(+), 244 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH v2 01/18] osdep: fix socket_error() to work with Mingw64

[Daniel P. Berrange]

Historically QEMU has had a socket_error() macro that was
defined to map to WSASocketError(). The os-win32.h header
file would define errno constants that mapped to the
WSA error constants. This worked fine with Mingw32 since
its header files never defined any errno values, nor did
it even provide an errno.h.  So callers of socket_error()
could match on traditional Exxxx constants and it would
all "just work".

With Mingw64 though, things work rather differently. First
there is an errno.h file which defines all the traditional
errno constants you'd expect from a UNIX platform. There
is then a winerror.h which defined the WSA error constants.
Crucially the WSAExxxx errno values in winerror.h do not
match the Exxxx errno values in error.h.

If QEMU had only imported winerror.h it would still work,
but the qemu/osdep.h file unconditionally imports errno.h.
So callers of socket_error() will get now WSAExxxx values
back and compare them to the Exxx constants. This will
always fail silently at runtime.

To solve this QEMU needs to stop assuming the WSAExxxx
constant values match the Exxx constant values. Thus the
socket_error() macro is turned into a small function that
re-maps WSAExxxx values into Exxx.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 include/qemu/sockets.h    |  3 --
 include/sysemu/os-posix.h |  2 ++
 include/sysemu/os-win32.h | 27 +---------------
 util/oslib-win32.c        | 79 ++++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 81 insertions(+), 30 deletions(-)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 0be68de..49499f2 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -7,8 +7,6 @@
 #include <winsock2.h>
 #include <ws2tcpip.h>
 
-#define socket_error() WSAGetLastError()
-
 int inet_aton(const char *cp, struct in_addr *ia);
 
 #else
@@ -20,7 +18,6 @@ int inet_aton(const char *cp, struct in_addr *ia);
 #include <netdb.h>
 #include <sys/un.h>
 
-#define socket_error() errno
 #define closesocket(s) close(s)
 
 #endif /* !_WIN32 */
diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
index 5b9c4d6..e9fec2e 100644
--- a/include/sysemu/os-posix.h
+++ b/include/sysemu/os-posix.h
@@ -34,6 +34,8 @@ void os_daemonize(void);
 void os_setup_post(void);
 int os_mlock(void);
 
+#define socket_error() errno
+
 typedef struct timeval qemu_timeval;
 #define qemu_gettimeofday(tp) gettimeofday(tp, NULL)
 
diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
index fbed346..239771d 100644
--- a/include/sysemu/os-win32.h
+++ b/include/sysemu/os-win32.h
@@ -29,32 +29,6 @@
 #include <winsock2.h>
 #include <windows.h>
 
-/* Workaround for older versions of MinGW. */
-#ifndef ECONNREFUSED
-# define ECONNREFUSED WSAECONNREFUSED
-#endif
-#ifndef EINPROGRESS
-# define EINPROGRESS  WSAEINPROGRESS
-#endif
-#ifndef EHOSTUNREACH
-# define EHOSTUNREACH WSAEHOSTUNREACH
-#endif
-#ifndef EINTR
-# define EINTR        WSAEINTR
-#endif
-#ifndef EINPROGRESS
-# define EINPROGRESS  WSAEINPROGRESS
-#endif
-#ifndef ENETUNREACH
-# define ENETUNREACH  WSAENETUNREACH
-#endif
-#ifndef ENOTCONN
-# define ENOTCONN     WSAENOTCONN
-#endif
-#ifndef EWOULDBLOCK
-# define EWOULDBLOCK  WSAEWOULDBLOCK
-#endif
-
 #if defined(_WIN64)
 /* On w64, setjmp is implemented by _setjmp which needs a second parameter.
  * If this parameter is NULL, longjump does no stack unwinding.
@@ -80,6 +54,7 @@ struct tm *gmtime_r(const time_t *timep, struct tm *result);
 struct tm *localtime_r(const time_t *timep, struct tm *result);
 #endif /* CONFIG_LOCALTIME_R */
 
+int socket_error(void);
 
 static inline void os_setup_signal_handling(void) {}
 static inline void os_daemonize(void) {}
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 438cfa4..1f717ee 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -2,7 +2,7 @@
  * os-win32.c
  *
  * Copyright (c) 2003-2008 Fabrice Bellard
- * Copyright (c) 2010 Red Hat, Inc.
+ * Copyright (c) 2010-2016 Red Hat, Inc.
  *
  * QEMU library functions for win32 which are shared between QEMU and
  * the QEMU tools.
@@ -144,6 +144,83 @@ int socket_set_fast_reuse(int fd)
     return 0;
 }
 
+
+int socket_error(void)
+{
+    switch (WSAGetLastError()) {
+    case 0:
+        return 0;
+    case WSAEINTR:
+        return EINTR;
+    case WSAEINVAL:
+        return EINVAL;
+    case WSA_INVALID_HANDLE:
+        return EBADF;
+    case WSA_NOT_ENOUGH_MEMORY:
+        return ENOMEM;
+    case WSA_INVALID_PARAMETER:
+        return EINVAL;
+    case WSAENAMETOOLONG:
+        return ENAMETOOLONG;
+    case WSAENOTEMPTY:
+        return ENOTEMPTY;
+    case WSAEWOULDBLOCK:
+         /* not using EWOULDBLOCK as we don't want code to have
+          * to check both EWOULDBLOCK and EAGAIN */
+        return EAGAIN;
+    case WSAEINPROGRESS:
+        return EINPROGRESS;
+    case WSAEALREADY:
+        return EALREADY;
+    case WSAENOTSOCK:
+        return ENOTSOCK;
+    case WSAEDESTADDRREQ:
+        return EDESTADDRREQ;
+    case WSAEMSGSIZE:
+        return EMSGSIZE;
+    case WSAEPROTOTYPE:
+        return EPROTOTYPE;
+    case WSAENOPROTOOPT:
+        return ENOPROTOOPT;
+    case WSAEPROTONOSUPPORT:
+        return EPROTONOSUPPORT;
+    case WSAEOPNOTSUPP:
+        return EOPNOTSUPP;
+    case WSAEAFNOSUPPORT:
+        return EAFNOSUPPORT;
+    case WSAEADDRINUSE:
+        return EADDRINUSE;
+    case WSAEADDRNOTAVAIL:
+        return EADDRNOTAVAIL;
+    case WSAENETDOWN:
+        return ENETDOWN;
+    case WSAENETUNREACH:
+        return ENETUNREACH;
+    case WSAENETRESET:
+        return ENETRESET;
+    case WSAECONNABORTED:
+        return ECONNABORTED;
+    case WSAECONNRESET:
+        return ECONNRESET;
+    case WSAENOBUFS:
+        return ENOBUFS;
+    case WSAEISCONN:
+        return EISCONN;
+    case WSAENOTCONN:
+        return ENOTCONN;
+    case WSAETIMEDOUT:
+        return ETIMEDOUT;
+    case WSAECONNREFUSED:
+        return ECONNREFUSED;
+    case WSAELOOP:
+        return ELOOP;
+    case WSAEHOSTUNREACH:
+        return EHOSTUNREACH;
+    default:
+        return EIO;
+    }
+}
+
 int inet_aton(const char *cp, struct in_addr *ia)
 {
     uint32_t addr = inet_addr(cp);
-- 
2.5.0

[Qemu-devel] [PATCH v2 02/18] io: use bind() to check for IPv4/6 availability

[Daniel P. Berrange]

Currently the test-io-channel-socket.c test uses getifaddrs
to see if an IPv4/6 address is present on any host NIC, as
a way to determine if IPv4/6 sockets can be used. This is
problematic because getifaddrs is not available on Win32.

Rather than testing indirectly via getifaddrs, just create
a socket and try to bind() to the loopback address instead.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 tests/test-io-channel-socket.c | 79 +++++++++++++++++-------------------------
 1 file changed, 31 insertions(+), 48 deletions(-)

diff --git a/tests/test-io-channel-socket.c b/tests/test-io-channel-socket.c
index 8a34056..6098fee 100644
--- a/tests/test-io-channel-socket.c
+++ b/tests/test-io-channel-socket.c
@@ -22,66 +22,49 @@
 #include "io/channel-socket.h"
 #include "io/channel-util.h"
 #include "io-channel-helpers.h"
-#ifdef HAVE_IFADDRS_H
-#include <ifaddrs.h>
-#endif
 
-static int check_protocol_support(bool *has_ipv4, bool *has_ipv6)
+static int check_bind(struct sockaddr *sa, socklen_t salen, bool *has_proto)
 {
-#ifdef HAVE_IFADDRS_H
-    struct ifaddrs *ifaddr = NULL, *ifa;
-    struct addrinfo hints = { 0 };
-    struct addrinfo *ai = NULL;
-    int gaierr;
-
-    *has_ipv4 = *has_ipv6 = false;
+    int fd;
 
-    if (getifaddrs(&ifaddr) < 0) {
-        g_printerr("Failed to lookup interface addresses: %s\n",
-                   strerror(errno));
+    fd = socket(sa->sa_family, SOCK_STREAM, 0);
+    if (fd < 0) {
         return -1;
     }
 
-    for (ifa = ifaddr; ifa != NULL; ifa = ifa->ifa_next) {
-        if (!ifa->ifa_addr) {
-            continue;
-        }
-
-        if (ifa->ifa_addr->sa_family == AF_INET) {
-            *has_ipv4 = true;
-        }
-        if (ifa->ifa_addr->sa_family == AF_INET6) {
-            *has_ipv6 = true;
+    if (bind(fd, sa, salen) < 0) {
+        close(fd);
+        if (errno == EADDRNOTAVAIL) {
+            *has_proto = false;
+            return 0;
         }
+        return -1;
     }
 
-    freeifaddrs(ifaddr);
-
-    hints.ai_flags = AI_PASSIVE | AI_ADDRCONFIG;
-    hints.ai_family = AF_INET6;
-    hints.ai_socktype = SOCK_STREAM;
-
-    gaierr = getaddrinfo("::1", NULL, &hints, &ai);
-    if (gaierr != 0) {
-        if (gaierr == EAI_ADDRFAMILY ||
-            gaierr == EAI_FAMILY ||
-            gaierr == EAI_NONAME) {
-            *has_ipv6 = false;
-        } else {
-            g_printerr("Failed to resolve ::1 address: %s\n",
-                       gai_strerror(gaierr));
-            return -1;
-        }
-    }
+    close(fd);
+    *has_proto = true;
+    return 0;
+}
 
-    freeaddrinfo(ai);
+static int check_protocol_support(bool *has_ipv4, bool *has_ipv6)
+{
+    struct sockaddr_in sin = {
+        .sin_family = AF_INET,
+        .sin_addr = { .s_addr = htonl(INADDR_LOOPBACK) },
+    };
+    struct sockaddr_in6 sin6 = {
+        .sin6_family = AF_INET6,
+        .sin6_addr = IN6ADDR_LOOPBACK_INIT,
+    };
 
-    return 0;
-#else
-    *has_ipv4 = *has_ipv6 = false;
+    if (check_bind((struct sockaddr *)&sin, sizeof(sin), has_ipv4) < 0) {
+        return -1;
+    }
+    if (check_bind((struct sockaddr *)&sin6, sizeof(sin6), has_ipv6) < 0) {
+        return -1;
+    }
 
-    return -1;
-#endif
+    return 0;
 }
 
 
-- 
2.5.0

[Qemu-devel] [PATCH v2 03/18] io: initialize sockets in test program

[Daniel P. Berrange]

The win32 sockets layer requires that socket_init() is called
otherwise nothing will work.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 tests/test-io-channel-socket.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/test-io-channel-socket.c b/tests/test-io-channel-socket.c
index 6098fee..f226e47 100644
--- a/tests/test-io-channel-socket.c
+++ b/tests/test-io-channel-socket.c
@@ -489,6 +489,7 @@ int main(int argc, char **argv)
     bool has_ipv4, has_ipv6;
 
     module_call_init(MODULE_INIT_QOM);
+    socket_init();
 
     g_test_init(&argc, &argv, NULL);
 
-- 
2.5.0

[Qemu-devel] [PATCH v2 04/18] io: bind to socket before creating QIOChannelSocket

[Daniel P. Berrange]

In the QIOChannelSocket test we create a socket file
descriptor and then try to create a QIOChannelSocket.
This works on Linux, but fails on Win32 because it is
not valid to call getsockname() on an unbound socket.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 tests/test-io-channel-socket.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/tests/test-io-channel-socket.c b/tests/test-io-channel-socket.c
index f226e47..4c16da1 100644
--- a/tests/test-io-channel-socket.c
+++ b/tests/test-io-channel-socket.c
@@ -470,10 +470,20 @@ static void test_io_channel_ipv4_fd(void)
 {
     QIOChannel *ioc;
     int fd = -1;
+    struct sockaddr_in sa = {
+        .sin_family = AF_INET,
+        .sin_addr = {
+            .s_addr =  htonl(INADDR_LOOPBACK),
+        }
+        /* Leave port unset for auto-assign */
+    };
+    socklen_t salen = sizeof(sa);
 
     fd = socket(AF_INET, SOCK_STREAM, 0);
     g_assert_cmpint(fd, >, -1);
 
+    g_assert_cmpint(bind(fd, (struct sockaddr *)&sa, salen), ==, 0);
+
     ioc = qio_channel_new_fd(fd, &error_abort);
 
     g_assert_cmpstr(object_get_typename(OBJECT(ioc)),
-- 
2.5.0

[Qemu-devel] [PATCH v2 05/18] io: wait for incoming client in socket test

[Daniel P. Berrange]

Exercise the GSource code for server sockets by calling
qio_channel_wait() prior to accepting the incoming client.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 tests/test-io-channel-socket.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/test-io-channel-socket.c b/tests/test-io-channel-socket.c
index 4c16da1..ae665f5 100644
--- a/tests/test-io-channel-socket.c
+++ b/tests/test-io-channel-socket.c
@@ -114,6 +114,7 @@ static void test_io_channel_setup_sync(SocketAddress *listen_addr,
         QIO_CHANNEL_SOCKET(*src), connect_addr, &error_abort);
     qio_channel_set_delay(*src, false);
 
+    qio_channel_wait(QIO_CHANNEL(lioc), G_IO_IN);
     *dst = QIO_CHANNEL(qio_channel_socket_accept(lioc, &error_abort));
     g_assert(*dst);
 
@@ -181,6 +182,7 @@ static void test_io_channel_setup_async(SocketAddress *listen_addr,
 
     g_assert(!data.err);
 
+    qio_channel_wait(QIO_CHANNEL(lioc), G_IO_IN);
     *dst = QIO_CHANNEL(qio_channel_socket_accept(lioc, &error_abort));
     g_assert(*dst);
 
-- 
2.5.0

[Qemu-devel] [PATCH v2 06/18] io: set correct error object in background reader test thread

[Daniel P. Berrange]

The reader thread was accidentally setting the error pointer
intended for the writer thread. If both threads set errors
this would result in QEMU abort'ing due to the error already
being set.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 tests/io-channel-helpers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/io-channel-helpers.c b/tests/io-channel-helpers.c
index 8440669..d513792 100644
--- a/tests/io-channel-helpers.c
+++ b/tests/io-channel-helpers.c
@@ -132,7 +132,7 @@ static gpointer test_io_thread_reader(gpointer opaque)
 
         if (ret == QIO_CHANNEL_ERR_BLOCK) {
             if (data->blocking) {
-                error_setg(&data->writeerr,
+                error_setg(&data->readerr,
                            "Unexpected I/O blocking");
                 break;
             } else {
-- 
2.5.0

[Qemu-devel] [PATCH v2 07/18] io: assert errors before asserting content in I/O test

[Daniel P. Berrange]

When checking the results of an I/O operation test, assert that
the error objects are NULL before asserting on the content. This
is found to give more useful indication of the problem when
diagnosing test failures.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 tests/io-channel-helpers.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/io-channel-helpers.c b/tests/io-channel-helpers.c
index d513792..a4dedbe 100644
--- a/tests/io-channel-helpers.c
+++ b/tests/io-channel-helpers.c
@@ -233,11 +233,11 @@ void qio_channel_test_run_reader(QIOChannelTest *test,
 
 void qio_channel_test_validate(QIOChannelTest *test)
 {
+    g_assert(test->readerr == NULL);
+    g_assert(test->writeerr == NULL);
     g_assert_cmpint(memcmp(test->input,
                            test->output,
                            test->len), ==, 0);
-    g_assert(test->readerr == NULL);
-    g_assert(test->writeerr == NULL);
 
     g_free(test->inputv);
     g_free(test->outputv);
-- 
2.5.0

[Qemu-devel] [PATCH v2 08/18] io: fix copy+paste mistake in socket error message

[Daniel P. Berrange]

s/write/read/ in the error message reported after
readmsg() fails

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 io/channel-socket.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/io/channel-socket.c b/io/channel-socket.c
index bf66a78..5f087e6 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -569,7 +569,7 @@ static ssize_t qio_channel_socket_readv(QIOChannel *ioc,
                 goto retry;
             } else {
                 error_setg_errno(errp, socket_error(),
-                                 "Unable to write to socket");
+                                 "Unable to read from socket");
                 return -1;
             }
         }
-- 
2.5.0

[Qemu-devel] [PATCH v2 09/18] io: pass HANDLE to g_source_add_poll on Win32

[Daniel P. Berrange]

From: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 io/channel-watch.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/io/channel-watch.c b/io/channel-watch.c
index 931fa4d..5373605 100644
--- a/io/channel-watch.c
+++ b/io/channel-watch.c
@@ -160,7 +160,11 @@ GSource *qio_channel_create_fd_watch(QIOChannel *ioc,
 
     ssource->condition = condition;
 
+#ifdef CONFIG_WIN32
+    ssource->fd.fd = (gint64)_get_osfhandle(fd);
+#else
     ssource->fd.fd = fd;
+#endif
     ssource->fd.events = condition;
 
     g_source_add_poll(source, &ssource->fd);
@@ -186,10 +190,15 @@ GSource *qio_channel_create_fd_pair_watch(QIOChannel *ioc,
 
     ssource->condition = condition;
 
+#ifdef CONFIG_WIN32
+    ssource->fdread.fd = (gint64)_get_osfhandle(fdread);
+    ssource->fdwrite.fd = (gint64)_get_osfhandle(fdwrite);
+#else
     ssource->fdread.fd = fdread;
-    ssource->fdread.events = condition & G_IO_IN;
-
     ssource->fdwrite.fd = fdwrite;
+#endif
+
+    ssource->fdread.events = condition & G_IO_IN;
     ssource->fdwrite.events = condition & G_IO_OUT;
 
     g_source_add_poll(source, &ssource->fdread);
-- 
2.5.0

[Qemu-devel] [PATCH v2 10/18] io: introduce qio_channel_create_socket_watch

[Daniel P. Berrange]

From: Paolo Bonzini <pbonzini@redhat.com>

Sockets are not in the same namespace as file descriptors on Windows.
As an initial step, introduce separate APIs for file descriptor and
socket watches.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 include/io/channel-watch.h | 20 +++++++++++++++++++-
 io/channel-socket.c        |  6 +++---
 io/channel-watch.c         | 15 +++++++++++++++
 3 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/include/io/channel-watch.h b/include/io/channel-watch.h
index 656358a..76d7642 100644
--- a/include/io/channel-watch.h
+++ b/include/io/channel-watch.h
@@ -39,7 +39,7 @@
  * monitor the file descriptor @fd for the
  * I/O conditions in @condition. This is able
  * monitor block devices, character devices,
- * sockets, pipes but not plain files.
+ * pipes but not plain files or, on Win32, sockets.
  *
  * Returns: the new main loop source
  */
@@ -48,6 +48,24 @@ GSource *qio_channel_create_fd_watch(QIOChannel *ioc,
                                      GIOCondition condition);
 
 /**
+ * qio_channel_create_socket_watch:
+ * @ioc: the channel object
+ * @fd: the file descriptor
+ * @condition: the I/O condition
+ *
+ * Create a new main loop source that is able to
+ * monitor the file descriptor @fd for the
+ * I/O conditions in @condition. This is equivalent
+ * to qio_channel_create_fd_watch on POSIX systems
+ * but not on Windows.
+ *
+ * Returns: the new main loop source
+ */
+GSource *qio_channel_create_socket_watch(QIOChannel *ioc,
+                                         int fd,
+                                         GIOCondition condition);
+
+/**
  * qio_channel_create_fd_pair_watch:
  * @ioc: the channel object
  * @fdread: the file descriptor for reading
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 5f087e6..775bb9f 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -714,9 +714,9 @@ static GSource *qio_channel_socket_create_watch(QIOChannel *ioc,
                                                 GIOCondition condition)
 {
     QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
-    return qio_channel_create_fd_watch(ioc,
-                                       sioc->fd,
-                                       condition);
+    return qio_channel_create_socket_watch(ioc,
+                                           sioc->fd,
+                                           condition);
 }
 
 static void qio_channel_socket_class_init(ObjectClass *klass,
diff --git a/io/channel-watch.c b/io/channel-watch.c
index 5373605..dfac8f8 100644
--- a/io/channel-watch.c
+++ b/io/channel-watch.c
@@ -172,6 +172,21 @@ GSource *qio_channel_create_fd_watch(QIOChannel *ioc,
     return source;
 }
 
+#ifdef CONFIG_WIN32
+GSource *qio_channel_create_socket_watch(QIOChannel *ioc,
+                                         int socket,
+                                         GIOCondition condition)
+{
+    abort();
+}
+#else
+GSource *qio_channel_create_socket_watch(QIOChannel *ioc,
+                                         int socket,
+                                         GIOCondition condition)
+{
+    return qio_channel_create_fd_watch(ioc, socket, condition);
+}
+#endif
 
 GSource *qio_channel_create_fd_pair_watch(QIOChannel *ioc,
                                           int fdread,
-- 
2.5.0

[Qemu-devel] [PATCH v2 11/18] io: use qemu_accept to ensure SOCK_CLOEXEC is set

[Daniel P. Berrange]

The QIOChannelSocket code mistakenly uses the bare accept()
function which does not set SOCK_CLOEXEC.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 io/channel-socket.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/io/channel-socket.c b/io/channel-socket.c
index 775bb9f..9b5f2d8 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -343,8 +343,8 @@ qio_channel_socket_accept(QIOChannelSocket *ioc,
 
  retry:
     trace_qio_channel_socket_accept(ioc);
-    cioc->fd = accept(ioc->fd, (struct sockaddr *)&cioc->remoteAddr,
-                      &cioc->remoteAddrLen);
+    cioc->fd = qemu_accept(ioc->fd, (struct sockaddr *)&cioc->remoteAddr,
+                           &cioc->remoteAddrLen);
     if (cioc->fd < 0) {
         trace_qio_channel_socket_accept_fail(ioc);
         if (socket_error() == EINTR) {
-- 
2.5.0

[Qemu-devel] [PATCH v2 12/18] io: remove checking of EWOULDBLOCK

[Daniel P. Berrange]

Since we now canonicalize WSAEWOULDBLOCK into EAGAIN there is
no longer any need to explicitly check EWOULDBLOCK for Win32.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 io/channel-command.c | 6 ++----
 io/channel-file.c    | 6 ++----
 io/channel-socket.c  | 6 ++----
 3 files changed, 6 insertions(+), 12 deletions(-)

diff --git a/io/channel-command.c b/io/channel-command.c
index f53ce0f..604514a 100644
--- a/io/channel-command.c
+++ b/io/channel-command.c
@@ -236,8 +236,7 @@ static ssize_t qio_channel_command_readv(QIOChannel *ioc,
  retry:
     ret = readv(cioc->readfd, iov, niov);
     if (ret < 0) {
-        if (errno == EAGAIN ||
-            errno == EWOULDBLOCK) {
+        if (errno == EAGAIN) {
             return QIO_CHANNEL_ERR_BLOCK;
         }
         if (errno == EINTR) {
@@ -265,8 +264,7 @@ static ssize_t qio_channel_command_writev(QIOChannel *ioc,
  retry:
     ret = writev(cioc->writefd, iov, niov);
     if (ret <= 0) {
-        if (errno == EAGAIN ||
-            errno == EWOULDBLOCK) {
+        if (errno == EAGAIN) {
             return QIO_CHANNEL_ERR_BLOCK;
         }
         if (errno == EINTR) {
diff --git a/io/channel-file.c b/io/channel-file.c
index 19a4325..f28e2b0 100644
--- a/io/channel-file.c
+++ b/io/channel-file.c
@@ -96,8 +96,7 @@ static ssize_t qio_channel_file_readv(QIOChannel *ioc,
  retry:
     ret = readv(fioc->fd, iov, niov);
     if (ret < 0) {
-        if (errno == EAGAIN ||
-            errno == EWOULDBLOCK) {
+        if (errno == EAGAIN) {
             return QIO_CHANNEL_ERR_BLOCK;
         }
         if (errno == EINTR) {
@@ -125,8 +124,7 @@ static ssize_t qio_channel_file_writev(QIOChannel *ioc,
  retry:
     ret = writev(fioc->fd, iov, niov);
     if (ret <= 0) {
-        if (errno == EAGAIN ||
-            errno == EWOULDBLOCK) {
+        if (errno == EAGAIN) {
             return QIO_CHANNEL_ERR_BLOCK;
         }
         if (errno == EINTR) {
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 9b5f2d8..2387d97 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -466,8 +466,7 @@ static ssize_t qio_channel_socket_readv(QIOChannel *ioc,
  retry:
     ret = recvmsg(sioc->fd, &msg, sflags);
     if (ret < 0) {
-        if (socket_error() == EAGAIN ||
-            socket_error() == EWOULDBLOCK) {
+        if (socket_error() == EAGAIN) {
             return QIO_CHANNEL_ERR_BLOCK;
         }
         if (socket_error() == EINTR) {
@@ -526,8 +525,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
  retry:
     ret = sendmsg(sioc->fd, &msg, 0);
     if (ret <= 0) {
-        if (socket_error() == EAGAIN ||
-            socket_error() == EWOULDBLOCK) {
+        if (socket_error() == EAGAIN) {
             return QIO_CHANNEL_ERR_BLOCK;
         }
         if (socket_error() == EINTR) {
-- 
2.5.0

[Qemu-devel] [PATCH v2 13/18] io: implement socket watch for win32 using WSAEventSelect+select

[Daniel P. Berrange]

From: Paolo Bonzini <pbonzini@redhat.com>

On Win32 we cannot directly poll on socket handles. Instead we
create a Win32 event object and associate the socket handle with
the event. When the event signals readyness we then have to
use select to determine which events are ready. Creating Win32
events is moderately heavyweight, so we don't want todo it
every time we create a GSource, so this associates a single
event with a QIOChannel.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 include/io/channel.h |   3 ++
 io/channel-socket.c  |  34 +++++++++++---
 io/channel-watch.c   | 126 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 io/channel.c         |  14 ++++++
 4 files changed, 170 insertions(+), 7 deletions(-)

diff --git a/include/io/channel.h b/include/io/channel.h
index 0a1f1ce..d37acd2 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -78,6 +78,9 @@ typedef gboolean (*QIOChannelFunc)(QIOChannel *ioc,
 struct QIOChannel {
     Object parent;
     unsigned int features; /* bitmask of QIOChannelFeatures */
+#ifdef _WIN32
+    HANDLE event; /* For use with GSource on Win32 */
+#endif
 };
 
 /**
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 2387d97..ae67ab1 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -55,6 +55,10 @@ qio_channel_socket_new(void)
     ioc = QIO_CHANNEL(sioc);
     ioc->features |= (1 << QIO_CHANNEL_FEATURE_SHUTDOWN);
 
+#ifdef WIN32
+    ioc->event = CreateEvent(NULL, FALSE, FALSE, NULL);
+#endif
+
     trace_qio_channel_socket_new(sioc);
 
     return sioc;
@@ -341,6 +345,11 @@ qio_channel_socket_accept(QIOChannelSocket *ioc,
     cioc->remoteAddrLen = sizeof(ioc->remoteAddr);
     cioc->localAddrLen = sizeof(ioc->localAddr);
 
+#ifdef WIN32
+    QIO_CHANNEL(cioc)->event = CreateEvent(NULL, FALSE, FALSE, NULL);
+#endif
+
+
  retry:
     trace_qio_channel_socket_accept(ioc);
     cioc->fd = qemu_accept(ioc->fd, (struct sockaddr *)&cioc->remoteAddr,
@@ -384,7 +393,10 @@ static void qio_channel_socket_finalize(Object *obj)
 {
     QIOChannelSocket *ioc = QIO_CHANNEL_SOCKET(obj);
     if (ioc->fd != -1) {
-        close(ioc->fd);
+#ifdef WIN32
+        WSAEventSelect(ioc->fd, NULL, 0);
+#endif
+        closesocket(ioc->fd);
         ioc->fd = -1;
     }
 }
@@ -634,6 +646,11 @@ qio_channel_socket_set_blocking(QIOChannel *ioc,
         qemu_set_block(sioc->fd);
     } else {
         qemu_set_nonblock(sioc->fd);
+#ifdef WIN32
+        WSAEventSelect(sioc->fd, ioc->event,
+                       FD_READ | FD_ACCEPT | FD_CLOSE |
+                       FD_CONNECT | FD_WRITE | FD_OOB);
+#endif
     }
     return 0;
 }
@@ -669,13 +686,18 @@ qio_channel_socket_close(QIOChannel *ioc,
 {
     QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
 
-    if (closesocket(sioc->fd) < 0) {
+    if (sioc->fd != -1) {
+#ifdef WIN32
+        WSAEventSelect(sioc->fd, NULL, 0);
+#endif
+        if (closesocket(sioc->fd) < 0) {
+            sioc->fd = -1;
+            error_setg_errno(errp, socket_error(),
+                             "Unable to close socket");
+            return -1;
+        }
         sioc->fd = -1;
-        error_setg_errno(errp, socket_error(),
-                         "Unable to close socket");
-        return -1;
     }
-    sioc->fd = -1;
     return 0;
 }
 
diff --git a/io/channel-watch.c b/io/channel-watch.c
index dfac8f8..cf1cdff 100644
--- a/io/channel-watch.c
+++ b/io/channel-watch.c
@@ -30,6 +30,20 @@ struct QIOChannelFDSource {
 };
 
 
+#ifdef CONFIG_WIN32
+typedef struct QIOChannelSocketSource QIOChannelSocketSource;
+struct QIOChannelSocketSource {
+    GSource parent;
+    GPollFD fd;
+    QIOChannel *ioc;
+    SOCKET socket;
+    int revents;
+    GIOCondition condition;
+};
+
+#endif
+
+
 typedef struct QIOChannelFDPairSource QIOChannelFDPairSource;
 struct QIOChannelFDPairSource {
     GSource parent;
@@ -82,6 +96,97 @@ qio_channel_fd_source_finalize(GSource *source)
 }
 
 
+#ifdef CONFIG_WIN32
+static gboolean
+qio_channel_socket_source_prepare(GSource *source G_GNUC_UNUSED,
+                                  gint *timeout)
+{
+    *timeout = -1;
+
+    return FALSE;
+}
+
+
+/*
+ * NB, this impl only works when the socket is in non-blocking
+ * mode on Win32
+ */
+static gboolean
+qio_channel_socket_source_check(GSource *source)
+{
+    static struct timeval tv0;
+
+    QIOChannelSocketSource *ssource = (QIOChannelSocketSource *)source;
+    WSANETWORKEVENTS ev;
+    fd_set rfds, wfds, xfds;
+
+    if (!ssource->condition) {
+        return 0;
+    }
+
+    WSAEnumNetworkEvents(ssource->socket, ssource->ioc->event, &ev);
+
+    FD_ZERO(&rfds);
+    FD_ZERO(&wfds);
+    FD_ZERO(&xfds);
+    if (ssource->condition & G_IO_IN) {
+        FD_SET((SOCKET)ssource->socket, &rfds);
+    }
+    if (ssource->condition & G_IO_OUT) {
+        FD_SET((SOCKET)ssource->socket, &wfds);
+    }
+    if (ssource->condition & G_IO_PRI) {
+        FD_SET((SOCKET)ssource->socket, &xfds);
+    }
+    ssource->revents = 0;
+    if (select(0, &rfds, &wfds, &xfds, &tv0) == 0) {
+        return 0;
+    }
+
+    if (FD_ISSET(ssource->socket, &rfds)) {
+        ssource->revents |= G_IO_IN;
+    }
+    if (FD_ISSET(ssource->socket, &wfds)) {
+        ssource->revents |= G_IO_OUT;
+    }
+    if (FD_ISSET(ssource->socket, &xfds)) {
+        ssource->revents |= G_IO_PRI;
+    }
+
+    return ssource->revents;
+}
+
+
+static gboolean
+qio_channel_socket_source_dispatch(GSource *source,
+                                   GSourceFunc callback,
+                                   gpointer user_data)
+{
+    QIOChannelFunc func = (QIOChannelFunc)callback;
+    QIOChannelSocketSource *ssource = (QIOChannelSocketSource *)source;
+
+    return (*func)(ssource->ioc, ssource->revents, user_data);
+}
+
+
+static void
+qio_channel_socket_source_finalize(GSource *source)
+{
+    QIOChannelSocketSource *ssource = (QIOChannelSocketSource *)source;
+
+    object_unref(OBJECT(ssource->ioc));
+}
+
+
+GSourceFuncs qio_channel_socket_source_funcs = {
+    qio_channel_socket_source_prepare,
+    qio_channel_socket_source_check,
+    qio_channel_socket_source_dispatch,
+    qio_channel_socket_source_finalize
+};
+#endif
+
+
 static gboolean
 qio_channel_fd_pair_source_prepare(GSource *source G_GNUC_UNUSED,
                                    gint *timeout)
@@ -177,7 +282,26 @@ GSource *qio_channel_create_socket_watch(QIOChannel *ioc,
                                          int socket,
                                          GIOCondition condition)
 {
-    abort();
+    GSource *source;
+    QIOChannelSocketSource *ssource;
+
+    source = g_source_new(&qio_channel_socket_source_funcs,
+                          sizeof(QIOChannelSocketSource));
+    ssource = (QIOChannelSocketSource *)source;
+
+    ssource->ioc = ioc;
+    object_ref(OBJECT(ioc));
+
+    ssource->condition = condition;
+    ssource->socket = socket;
+    ssource->revents = 0;
+
+    ssource->fd.fd = (gintptr)ioc->event;
+    ssource->fd.events = G_IO_IN;
+
+    g_source_add_poll(source, &ssource->fd);
+
+    return source;
 }
 #else
 GSource *qio_channel_create_socket_watch(QIOChannel *ioc,
diff --git a/io/channel.c b/io/channel.c
index 3fc09f8..dd6fc0e 100644
--- a/io/channel.c
+++ b/io/channel.c
@@ -274,10 +274,24 @@ void qio_channel_wait(QIOChannel *ioc,
 }
 
 
+#ifdef _WIN32
+static void qio_channel_finalize(Object *obj)
+{
+    QIOChannel *ioc = QIO_CHANNEL(obj);
+
+    if (ioc->event) {
+        CloseHandle(ioc->event);
+    }
+}
+#endif
+
 static const TypeInfo qio_channel_info = {
     .parent = TYPE_OBJECT,
     .name = TYPE_QIO_CHANNEL,
     .instance_size = sizeof(QIOChannel),
+#ifdef _WIN32
+    .instance_finalize = qio_channel_finalize,
+#endif
     .abstract = true,
     .class_size = sizeof(QIOChannelClass),
 };
-- 
2.5.0

[Qemu-devel] [PATCH v2 14/18] char: remove qemu_chr_finish_socket_connection method

[Daniel P. Berrange]

The qemu_chr_finish_socket_connection method is multiplexing two
different actions into one method. Each caller of it though, only
wants one specific action. The code is shorter & clearer if we
thus remove the method and just inline the specific actions
where needed.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qemu-char.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index e0147f3..fe212b4 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3091,20 +3091,6 @@ static void tcp_chr_close(CharDriverState *chr)
     qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
 }
 
-static void qemu_chr_finish_socket_connection(CharDriverState *chr,
-                                              QIOChannelSocket *sioc)
-{
-    TCPCharDriver *s = chr->opaque;
-
-    if (s->is_listen) {
-        s->listen_ioc = sioc;
-        s->listen_tag = qio_channel_add_watch(
-            QIO_CHANNEL(s->listen_ioc), G_IO_IN, tcp_chr_accept, chr, NULL);
-    } else {
-        tcp_chr_new_client(chr, sioc);
-        object_unref(OBJECT(sioc));
-    }
-}
 
 static void qemu_chr_socket_connected(Object *src, Error *err, void *opaque)
 {
@@ -3119,7 +3105,8 @@ static void qemu_chr_socket_connected(Object *src, Error *err, void *opaque)
     }
 
     s->connect_err_reported = false;
-    qemu_chr_finish_socket_connection(chr, sioc);
+    tcp_chr_new_client(chr, sioc);
+    object_unref(OBJECT(sioc));
 }
 
 static bool qemu_chr_open_socket_fd(CharDriverState *chr, Error **errp)
@@ -3131,7 +3118,9 @@ static bool qemu_chr_open_socket_fd(CharDriverState *chr, Error **errp)
         if (qio_channel_socket_listen_sync(sioc, s->addr, errp) < 0) {
             goto fail;
         }
-        qemu_chr_finish_socket_connection(chr, sioc);
+        s->listen_ioc = sioc;
+        s->listen_tag = qio_channel_add_watch(
+            QIO_CHANNEL(s->listen_ioc), G_IO_IN, tcp_chr_accept, chr, NULL);
     } else if (s->reconnect_time) {
         qio_channel_socket_connect_async(sioc, s->addr,
                                          qemu_chr_socket_connected,
@@ -3140,7 +3129,8 @@ static bool qemu_chr_open_socket_fd(CharDriverState *chr, Error **errp)
         if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) {
             goto fail;
         }
-        qemu_chr_finish_socket_connection(chr, sioc);
+        tcp_chr_new_client(chr, sioc);
+        object_unref(OBJECT(sioc));
     }
 
     return true;
-- 
2.5.0

[Qemu-devel] [PATCH v2 15/18] char: remove socket_try_connect method

[Daniel P. Berrange]

The qemu_chr_open_socket_fd() method multiplexes three different
actions into one method. The socket_try_connect() method is one
of its callers, but it only ever want one specific action
performed. By inlining that action into socket_try_connect()
we see that there is not in fact any failure scenario, so there
is not even any reason for socket_try_connect to exist. Just
inline the asynchronous connection attempts directly at the
places that need them. This shortens & clarifies the code.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qemu-char.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index fe212b4..1540463 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3121,10 +3121,6 @@ static bool qemu_chr_open_socket_fd(CharDriverState *chr, Error **errp)
         s->listen_ioc = sioc;
         s->listen_tag = qio_channel_add_watch(
             QIO_CHANNEL(s->listen_ioc), G_IO_IN, tcp_chr_accept, chr, NULL);
-    } else if (s->reconnect_time) {
-        qio_channel_socket_connect_async(sioc, s->addr,
-                                         qemu_chr_socket_connected,
-                                         chr, NULL);
     } else {
         if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) {
             goto fail;
@@ -4248,19 +4244,11 @@ static CharDriverState *qmp_chardev_open_parallel(const char *id,
 
 #endif /* WIN32 */
 
-static void socket_try_connect(CharDriverState *chr)
-{
-    Error *err = NULL;
-
-    if (!qemu_chr_open_socket_fd(chr, &err)) {
-        check_report_connect_error(chr, err);
-    }
-}
-
 static gboolean socket_reconnect_timeout(gpointer opaque)
 {
     CharDriverState *chr = opaque;
     TCPCharDriver *s = chr->opaque;
+    QIOChannelSocket *sioc;
 
     s->reconnect_timer = 0;
 
@@ -4268,7 +4256,10 @@ static gboolean socket_reconnect_timeout(gpointer opaque)
         return false;
     }
 
-    socket_try_connect(chr);
+    sioc = qio_channel_socket_new();
+    qio_channel_socket_connect_async(sioc, s->addr,
+                                     qemu_chr_socket_connected,
+                                     chr, NULL);
 
     return false;
 }
@@ -4288,6 +4279,7 @@ static CharDriverState *qmp_chardev_open_socket(const char *id,
     bool is_waitconnect = sock->has_wait    ? sock->wait    : false;
     int64_t reconnect   = sock->has_reconnect ? sock->reconnect : 0;
     ChardevCommon *common = qapi_ChardevSocket_base(sock);
+    QIOChannelSocket *sioc = NULL;
 
     chr = qemu_chr_alloc(common, errp);
     if (!chr) {
@@ -4358,7 +4350,10 @@ static CharDriverState *qmp_chardev_open_socket(const char *id,
     }
 
     if (s->reconnect_time) {
-        socket_try_connect(chr);
+        sioc = qio_channel_socket_new();
+        qio_channel_socket_connect_async(sioc, s->addr,
+                                         qemu_chr_socket_connected,
+                                         chr, NULL);
     } else if (!qemu_chr_open_socket_fd(chr, errp)) {
         goto error;
     }
-- 
2.5.0

[Qemu-devel] [PATCH v2 16/18] char: remove qemu_chr_open_socket_fd method

[Daniel P. Berrange]

The qemu_chr_open_socket_fd method takes care of either doing a
synchronous socket connect, or creating a listener socket. Part
of the work when creating the listener socket is to register a
watch for incoming clients. The caller of qemu_chr_open_socket_fd
may not want this watch created, as it might be doing a synchronous
wait for the first client. Rather than passing yet more parameters
into qemu_chr_open_socket_fd to let it handle this, just remove
the qemu_chr_open_socket_fd method an inline its functionality
into the caller. This allows for a clearer control flow and shorter
code.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 qemu-char.c | 59 ++++++++++++++++++++++++-----------------------------------
 1 file changed, 24 insertions(+), 35 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 1540463..3bf30b5 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3109,32 +3109,6 @@ static void qemu_chr_socket_connected(Object *src, Error *err, void *opaque)
     object_unref(OBJECT(sioc));
 }
 
-static bool qemu_chr_open_socket_fd(CharDriverState *chr, Error **errp)
-{
-    TCPCharDriver *s = chr->opaque;
-    QIOChannelSocket *sioc = qio_channel_socket_new();
-
-    if (s->is_listen) {
-        if (qio_channel_socket_listen_sync(sioc, s->addr, errp) < 0) {
-            goto fail;
-        }
-        s->listen_ioc = sioc;
-        s->listen_tag = qio_channel_add_watch(
-            QIO_CHANNEL(s->listen_ioc), G_IO_IN, tcp_chr_accept, chr, NULL);
-    } else {
-        if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) {
-            goto fail;
-        }
-        tcp_chr_new_client(chr, sioc);
-        object_unref(OBJECT(sioc));
-    }
-
-    return true;
-
- fail:
-    object_unref(OBJECT(sioc));
-    return false;
-}
 
 /*********************************************************/
 /* Ring buffer chardev */
@@ -4349,25 +4323,40 @@ static CharDriverState *qmp_chardev_open_socket(const char *id,
         s->reconnect_time = reconnect;
     }
 
+    sioc = qio_channel_socket_new();
     if (s->reconnect_time) {
-        sioc = qio_channel_socket_new();
         qio_channel_socket_connect_async(sioc, s->addr,
                                          qemu_chr_socket_connected,
                                          chr, NULL);
-    } else if (!qemu_chr_open_socket_fd(chr, errp)) {
-        goto error;
-    }
-
-    if (is_listen && is_waitconnect) {
-        fprintf(stderr, "QEMU waiting for connection on: %s\n",
-                chr->filename);
-        tcp_chr_accept(QIO_CHANNEL(s->listen_ioc), G_IO_IN, chr);
+    } else if (s->is_listen) {
+        if (qio_channel_socket_listen_sync(sioc, s->addr, errp) < 0) {
+            goto error;
+        }
+        s->listen_ioc = sioc;
+        if (is_waitconnect) {
+            fprintf(stderr, "QEMU waiting for connection on: %s\n",
+                    chr->filename);
+            tcp_chr_accept(QIO_CHANNEL(s->listen_ioc), G_IO_IN, chr);
+        }
         qio_channel_set_blocking(QIO_CHANNEL(s->listen_ioc), false, NULL);
+        if (!s->ioc) {
+            s->listen_tag = qio_channel_add_watch(
+                QIO_CHANNEL(s->listen_ioc), G_IO_IN, tcp_chr_accept, chr, NULL);
+        }
+    } else {
+        if (qio_channel_socket_connect_sync(sioc, s->addr, errp) < 0) {
+            goto error;
+        }
+        tcp_chr_new_client(chr, sioc);
+        object_unref(OBJECT(sioc));
     }
 
     return chr;
 
  error:
+    if (sioc) {
+        object_unref(OBJECT(sioc));
+    }
     if (s->tls_creds) {
         object_unref(OBJECT(s->tls_creds));
     }
-- 
2.5.0

[Qemu-devel] [PATCH v2 17/18] osdep: add wrappers for socket functions

[Daniel P. Berrange]

The windows socket functions look identical to the normal POSIX
sockets functions, but instead of setting errno, the caller needs
to call WSAGetLastError(). QEMU has tried to deal with this
incompatibility by defining a socket_error() method that callers
must use that abstracts the difference between WSAGetLastError()
and errno.

This approach is somewhat error prone though - many callers of
the sockets functions are just using errno directly because it
is easy to forget the need use a QEMU specific wrapper. It is
not always immediately obvious that a particular function will
in fact call into Windows sockets functions, so the dev may not
even realize they need to use socket_error().

This introduces an alternative approach to portability inspired
by the way GNULIB fixes portability problems. We use a macro to
redefine the original socket function names to refer to a QEMU
wrapper function. The wrapper function calls the original Win32
sockets method and then sets errno from the WSAGetLastError()
value.

Thus all code can simply call the normal POSIX sockets APIs are
have standard errno reporting on error, even on Windows. This
makes the socket_error() method obsolete.

We also bring closesocket & ioctlsocket into this approach. Even
though they are non-standard Win32 names, we can't wrap the normal
close/ioctl methods since there's no reliable way to distinguish
between a file descriptor and HANDLE in Win32.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 Makefile                  |   4 +-
 include/qemu/sockets.h    |  14 ----
 include/sysemu/os-posix.h |   9 +++
 include/sysemu/os-win32.h |  79 ++++++++++++++++++
 linux-user/flatload.c     |   1 -
 slirp/slirp.h             |   2 -
 util/oslib-win32.c        | 201 ++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 291 insertions(+), 19 deletions(-)

diff --git a/Makefile b/Makefile
index 70e3ebc..1d076a9 100644
--- a/Makefile
+++ b/Makefile
@@ -238,7 +238,7 @@ qemu-img$(EXESUF): qemu-img.o $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-o
 qemu-nbd$(EXESUF): qemu-nbd.o $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) libqemuutil.a libqemustub.a
 qemu-io$(EXESUF): qemu-io.o $(block-obj-y) $(crypto-obj-y) $(io-obj-y) $(qom-obj-y) libqemuutil.a libqemustub.a
 
-qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o
+qemu-bridge-helper$(EXESUF): qemu-bridge-helper.o libqemuutil.a libqemustub.a
 
 fsdev/virtfs-proxy-helper$(EXESUF): fsdev/virtfs-proxy-helper.o fsdev/9p-marshal.o fsdev/9p-iov-marshal.o libqemuutil.a libqemustub.a
 fsdev/virtfs-proxy-helper$(EXESUF): LIBS += -lcap
@@ -329,7 +329,7 @@ ifneq ($(EXESUF),)
 qemu-ga: qemu-ga$(EXESUF) $(QGA_VSS_PROVIDER) $(QEMU_GA_MSI)
 endif
 
-ivshmem-client$(EXESUF): $(ivshmem-client-obj-y)
+ivshmem-client$(EXESUF): $(ivshmem-client-obj-y) libqemuutil.a libqemustub.a
 	$(call LINK, $^)
 ivshmem-server$(EXESUF): $(ivshmem-server-obj-y) libqemuutil.a libqemustub.a
 	$(call LINK, $^)
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 49499f2..1bd9218 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -3,23 +3,9 @@
 #define QEMU_SOCKET_H
 
 #ifdef _WIN32
-#include <windows.h>
-#include <winsock2.h>
-#include <ws2tcpip.h>
 
 int inet_aton(const char *cp, struct in_addr *ia);
 
-#else
-
-#include <sys/socket.h>
-#include <netinet/in.h>
-#include <netinet/tcp.h>
-#include <arpa/inet.h>
-#include <netdb.h>
-#include <sys/un.h>
-
-#define closesocket(s) close(s)
-
 #endif /* !_WIN32 */
 
 #include "qapi-types.h"
diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
index e9fec2e..53fac98 100644
--- a/include/sysemu/os-posix.h
+++ b/include/sysemu/os-posix.h
@@ -26,6 +26,12 @@
 #ifndef QEMU_OS_POSIX_H
 #define QEMU_OS_POSIX_H
 
+#include <sys/socket.h>
+#include <netinet/in.h>
+#include <netinet/tcp.h>
+#include <arpa/inet.h>
+#include <netdb.h>
+#include <sys/un.h>
 
 void os_set_line_buffering(void);
 void os_set_proc_name(const char *s);
@@ -36,6 +42,9 @@ int os_mlock(void);
 
 #define socket_error() errno
 
+#define closesocket(s) close(s)
+#define ioctlsocket(s, r, v) ioctl(s, r, v)
+
 typedef struct timeval qemu_timeval;
 #define qemu_gettimeofday(tp) gettimeofday(tp, NULL)
 
diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
index 239771d..6905066 100644
--- a/include/sysemu/os-win32.h
+++ b/include/sysemu/os-win32.h
@@ -28,6 +28,7 @@
 
 #include <winsock2.h>
 #include <windows.h>
+#include <ws2tcpip.h>
 
 #if defined(_WIN64)
 /* On w64, setjmp is implemented by _setjmp which needs a second parameter.
@@ -104,4 +105,82 @@ static inline char *realpath(const char *path, char *resolved_path)
     return resolved_path;
 }
 
+
+/* We wrap all the sockets functions so that we can
+ * set errno based on WSAGetLastError()
+ */
+
+#undef connect
+#define connect qemu_connect_wrap
+int qemu_connect_wrap(int sockfd, const struct sockaddr *addr,
+                      socklen_t addrlen);
+
+#undef listen
+#define listen qemu_listen_wrap
+int qemu_listen_wrap(int sockfd, int backlog);
+
+#undef bind
+#define bind qemu_bind_wrap
+int qemu_bind_wrap(int sockfd, const struct sockaddr *addr,
+                   socklen_t addrlen);
+
+#undef socket
+#define socket qemu_socket_wrap
+int qemu_socket_wrap(int domain, int type, int protocol);
+
+#undef accept
+#define accept qemu_accept_wrap
+int qemu_accept_wrap(int sockfd, struct sockaddr *addr,
+                     socklen_t *addrlen);
+
+#undef shutdown
+#define shutdown qemu_shutdown_wrap
+int qemu_shutdown_wrap(int sockfd, int how);
+
+#undef ioctlsocket
+#define ioctlsocket qemu_ioctlsocket_wrap
+int qemu_ioctlsocket_wrap(int fd, int req, void *val);
+
+#undef closesocket
+#define closesocket qemu_closesocket_wrap
+int qemu_closesocket_wrap(int fd);
+
+#undef getsockopt
+#define getsockopt qemu_getsockopt_wrap
+int qemu_getsockopt_wrap(int sockfd, int level, int optname,
+                         void *optval, socklen_t *optlen);
+
+#undef setsockopt
+#define setsockopt qemu_setsockopt_wrap
+int qemu_setsockopt_wrap(int sockfd, int level, int optname,
+                         const void *optval, socklen_t optlen);
+
+#undef getpeername
+#define getpeername qemu_getpeername_wrap
+int qemu_getpeername_wrap(int sockfd, struct sockaddr *addr,
+                          socklen_t *addrlen);
+
+#undef getsockname
+#define getsockname qemu_getsockname_wrap
+int qemu_getsockname_wrap(int sockfd, struct sockaddr *addr,
+                          socklen_t *addrlen);
+
+#undef send
+#define send qemu_send_wrap
+ssize_t qemu_send_wrap(int sockfd, const void *buf, size_t len, int flags);
+
+#undef sendto
+#define sendto qemu_sendto_wrap
+ssize_t qemu_sendto_wrap(int sockfd, const void *buf, size_t len, int flags,
+                         const struct sockaddr *addr, socklen_t addrlen);
+
+#undef recv
+#define recv qemu_recv_wrap
+ssize_t qemu_recv_wrap(int sockfd, void *buf, size_t len, int flags);
+
+#undef recvfrom
+#define recvfrom qemu_recvfrom_wrap
+ssize_t qemu_recvfrom_wrap(int sockfd, void *buf, size_t len, int flags,
+                           struct sockaddr *addr, socklen_t *addrlen);
+
 #endif
diff --git a/linux-user/flatload.c b/linux-user/flatload.c
index a25c797..f9139c3 100644
--- a/linux-user/flatload.c
+++ b/linux-user/flatload.c
@@ -38,7 +38,6 @@
 
 #include "qemu.h"
 #include "flat.h"
-#define ntohl(x) be32_to_cpu(x)
 #include <target_flat.h>
 
 //#define DEBUG
diff --git a/slirp/slirp.h b/slirp/slirp.h
index 07c13b47..a6741e7 100644
--- a/slirp/slirp.h
+++ b/slirp/slirp.h
@@ -14,8 +14,6 @@ typedef char *caddr_t;
 # include <iphlpapi.h>
 
 #else
-# define ioctlsocket ioctl
-# define closesocket(s) close(s)
 # if !defined(__HAIKU__)
 #  define O_BINARY 0
 # endif
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 1f717ee..a6256de 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -581,3 +581,204 @@ pid_t qemu_fork(Error **errp)
                      "cannot fork child process");
     return -1;
 }
+
+
+#undef connect
+int qemu_connect_wrap(int sockfd, const struct sockaddr *addr,
+                      socklen_t addrlen)
+{
+    int ret;
+    ret = connect(sockfd, addr, addrlen);
+    if (ret < 0) {
+        errno = socket_error();
+    }
+    return ret;
+}
+
+
+#undef listen
+int qemu_listen_wrap(int sockfd, int backlog)
+{
+    int ret;
+    ret = listen(sockfd, backlog);
+    if (ret < 0) {
+        errno = socket_error();
+    }
+    return ret;
+}
+
+
+#undef bind
+int qemu_bind_wrap(int sockfd, const struct sockaddr *addr,
+                   socklen_t addrlen)
+{
+    int ret;
+    ret = bind(sockfd, addr, addrlen);
+    if (ret < 0) {
+        errno = socket_error();
+    }
+    return ret;
+}
+
+
+#undef socket
+int qemu_socket_wrap(int domain, int type, int protocol)
+{
+    int ret;
+    ret = socket(domain, type, protocol);
+    if (ret < 0) {
+        errno = socket_error();
+    }
+    return ret;
+}
+
+
+#undef accept
+int qemu_accept_wrap(int sockfd, struct sockaddr *addr,
+                     socklen_t *addrlen)
+{
+    int ret;
+    ret = accept(sockfd, addr, addrlen);
+    if (ret < 0) {
+        errno = socket_error();
+    }
+    return ret;
+}
+
+
+#undef shutdown
+int qemu_shutdown_wrap(int sockfd, int how)
+{
+    int ret;
+    ret = shutdown(sockfd, how);
+    if (ret < 0) {
+        errno = socket_error();
+    }
+    return ret;
+}
+
+
+#undef ioctlsocket
+int qemu_ioctlsocket_wrap(int fd, int req, void *val)
+{
+    int ret;
+    ret = ioctlsocket(fd, req, val);
+    if (ret < 0) {
+        errno = socket_error();
+    }
+    return ret;
+}
+
+
+#undef closesocket
+int qemu_closesocket_wrap(int fd)
+{
+    int ret;
+    ret = closesocket(fd);
+    if (ret < 0) {
+        errno = socket_error();
+    }
+    return ret;
+}
+
+
+#undef getsockopt
+int qemu_getsockopt_wrap(int sockfd, int level, int optname,
+                         void *optval, socklen_t *optlen)
+{
+    int ret;
+    ret = getsockopt(sockfd, level, optname, optval, optlen);
+    if (ret < 0) {
+        errno = socket_error();
+    }
+    return ret;
+}
+
+
+#undef setsockopt
+int qemu_setsockopt_wrap(int sockfd, int level, int optname,
+                         const void *optval, socklen_t optlen)
+{
+    int ret;
+    ret = setsockopt(sockfd, level, optname, optval, optlen);
+    if (ret < 0) {
+        errno = socket_error();
+    }
+    return ret;
+}
+
+
+#undef getpeername
+int qemu_getpeername_wrap(int sockfd, struct sockaddr *addr,
+                          socklen_t *addrlen)
+{
+    int ret;
+    ret = getpeername(sockfd, addr, addrlen);
+    if (ret < 0) {
+        errno = socket_error();
+    }
+    return ret;
+}
+
+
+#undef getsockname
+int qemu_getsockname_wrap(int sockfd, struct sockaddr *addr,
+                          socklen_t *addrlen)
+{
+    int ret;
+    ret = getsockname(sockfd, addr, addrlen);
+    if (ret < 0) {
+        errno = socket_error();
+    }
+    return ret;
+}
+
+
+#undef send
+ssize_t qemu_send_wrap(int sockfd, const void *buf, size_t len, int flags)
+{
+    int ret;
+    ret = send(sockfd, buf, len, flags);
+    if (ret < 0) {
+        errno = socket_error();
+    }
+    return ret;
+}
+
+
+#undef sendto
+ssize_t qemu_sendto_wrap(int sockfd, const void *buf, size_t len, int flags,
+                         const struct sockaddr *addr, socklen_t addrlen)
+{
+    int ret;
+    ret = sendto(sockfd, buf, len, flags, addr, addrlen);
+    if (ret < 0) {
+        errno = socket_error();
+    }
+    return ret;
+}
+
+
+#undef recv
+ssize_t qemu_recv_wrap(int sockfd, void *buf, size_t len, int flags)
+{
+    int ret;
+    ret = recv(sockfd, buf, len, flags);
+    if (ret < 0) {
+        errno = socket_error();
+    }
+    return ret;
+}
+
+
+#undef recvfrom
+ssize_t qemu_recvfrom_wrap(int sockfd, void *buf, size_t len, int flags,
+                           struct sockaddr *addr, socklen_t *addrlen)
+{
+    int ret;
+    ret = recvfrom(sockfd, buf, len, flags, addr, addrlen);
+    if (ret < 0) {
+        errno = socket_error();
+    }
+    return ret;
+}
-- 
2.5.0

[Qemu-devel] [PATCH v2 18/18] osdep: remove use of socket_error() from all code

[Daniel P. Berrange]

Now that QEMU wraps the Win32 sockets methods to automatically
set errno upon failure, there is no reason for callers to use
the socket_error() method. They can rely on accessing errno
even on Win32. Remove all use of socket_error() from general
code, leaving it as a static method in oslib-win32.c only.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 block/sheepdog.c           |  5 ++---
 include/sysemu/os-posix.h  |  2 --
 include/sysemu/os-win32.h  |  2 --
 io/channel-socket.c        | 38 +++++++++++++++++++-------------------
 migration/qemu-file-unix.c | 14 ++++++--------
 migration/tcp.c            |  7 +++----
 net/socket.c               | 19 ++++++++-----------
 slirp/tcp_input.c          |  4 ----
 util/oslib-win32.c         |  2 +-
 util/qemu-coroutine-io.c   |  6 ++----
 util/qemu-sockets.c        | 10 +++++-----
 11 files changed, 46 insertions(+), 63 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 8739acc..05677ed 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -615,14 +615,13 @@ static coroutine_fn int send_co_req(int sockfd, SheepdogReq *hdr, void *data,
     ret = qemu_co_send(sockfd, hdr, sizeof(*hdr));
     if (ret != sizeof(*hdr)) {
         error_report("failed to send a req, %s", strerror(errno));
-        ret = -socket_error();
-        return ret;
+        return -errno;
     }
 
     ret = qemu_co_send(sockfd, data, *wlen);
     if (ret != *wlen) {
-        ret = -socket_error();
         error_report("failed to send a req, %s", strerror(errno));
+        return -errno;
     }
 
     return ret;
diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
index 53fac98..07e3e5a 100644
--- a/include/sysemu/os-posix.h
+++ b/include/sysemu/os-posix.h
@@ -40,8 +40,6 @@ void os_daemonize(void);
 void os_setup_post(void);
 int os_mlock(void);
 
-#define socket_error() errno
-
 #define closesocket(s) close(s)
 #define ioctlsocket(s, r, v) ioctl(s, r, v)
 
diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
index 6905066..17aad3b 100644
--- a/include/sysemu/os-win32.h
+++ b/include/sysemu/os-win32.h
@@ -55,8 +55,6 @@ struct tm *gmtime_r(const time_t *timep, struct tm *result);
 struct tm *localtime_r(const time_t *timep, struct tm *result);
 #endif /* CONFIG_LOCALTIME_R */
 
-int socket_error(void);
-
 static inline void os_setup_signal_handling(void) {}
 static inline void os_daemonize(void) {}
 static inline void os_setup_post(void) {}
diff --git a/io/channel-socket.c b/io/channel-socket.c
index ae67ab1..d005070 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -82,11 +82,11 @@ qio_channel_socket_set_fd(QIOChannelSocket *sioc,
 
     if (getpeername(fd, (struct sockaddr *)&sioc->remoteAddr,
                     &sioc->remoteAddrLen) < 0) {
-        if (socket_error() == ENOTCONN) {
+        if (errno == ENOTCONN) {
             memset(&sioc->remoteAddr, 0, sizeof(sioc->remoteAddr));
             sioc->remoteAddrLen = sizeof(sioc->remoteAddr);
         } else {
-            error_setg_errno(errp, socket_error(),
+            error_setg_errno(errp, errno,
                              "Unable to query remote socket address");
             goto error;
         }
@@ -94,7 +94,7 @@ qio_channel_socket_set_fd(QIOChannelSocket *sioc,
 
     if (getsockname(fd, (struct sockaddr *)&sioc->localAddr,
                     &sioc->localAddrLen) < 0) {
-        error_setg_errno(errp, socket_error(),
+        error_setg_errno(errp, errno,
                          "Unable to query local socket address");
         goto error;
     }
@@ -356,7 +356,7 @@ qio_channel_socket_accept(QIOChannelSocket *ioc,
                            &cioc->remoteAddrLen);
     if (cioc->fd < 0) {
         trace_qio_channel_socket_accept_fail(ioc);
-        if (socket_error() == EINTR) {
+        if (errno == EINTR) {
             goto retry;
         }
         goto error;
@@ -364,7 +364,7 @@ qio_channel_socket_accept(QIOChannelSocket *ioc,
 
     if (getsockname(cioc->fd, (struct sockaddr *)&cioc->localAddr,
                     &cioc->localAddrLen) < 0) {
-        error_setg_errno(errp, socket_error(),
+        error_setg_errno(errp, errno,
                          "Unable to query local socket address");
         goto error;
     }
@@ -478,14 +478,14 @@ static ssize_t qio_channel_socket_readv(QIOChannel *ioc,
  retry:
     ret = recvmsg(sioc->fd, &msg, sflags);
     if (ret < 0) {
-        if (socket_error() == EAGAIN) {
+        if (errno == EAGAIN) {
             return QIO_CHANNEL_ERR_BLOCK;
         }
-        if (socket_error() == EINTR) {
+        if (errno == EINTR) {
             goto retry;
         }
 
-        error_setg_errno(errp, socket_error(),
+        error_setg_errno(errp, errno,
                          "Unable to read from socket");
         return -1;
     }
@@ -537,13 +537,13 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
  retry:
     ret = sendmsg(sioc->fd, &msg, 0);
     if (ret <= 0) {
-        if (socket_error() == EAGAIN) {
+        if (errno == EAGAIN) {
             return QIO_CHANNEL_ERR_BLOCK;
         }
-        if (socket_error() == EINTR) {
+        if (errno == EINTR) {
             goto retry;
         }
-        error_setg_errno(errp, socket_error(),
+        error_setg_errno(errp, errno,
                          "Unable to write to socket");
         return -1;
     }
@@ -569,16 +569,16 @@ static ssize_t qio_channel_socket_readv(QIOChannel *ioc,
                    iov[i].iov_len,
                    0);
         if (ret < 0) {
-            if (socket_error() == EAGAIN) {
+            if (errno == EAGAIN) {
                 if (done) {
                     return done;
                 } else {
                     return QIO_CHANNEL_ERR_BLOCK;
                 }
-            } else if (socket_error() == EINTR) {
+            } else if (errno == EINTR) {
                 goto retry;
             } else {
-                error_setg_errno(errp, socket_error(),
+                error_setg_errno(errp, errno,
                                  "Unable to read from socket");
                 return -1;
             }
@@ -611,16 +611,16 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
                    iov[i].iov_len,
                    0);
         if (ret < 0) {
-            if (socket_error() == EAGAIN) {
+            if (errno == EAGAIN) {
                 if (done) {
                     return done;
                 } else {
                     return QIO_CHANNEL_ERR_BLOCK;
                 }
-            } else if (socket_error() == EINTR) {
+            } else if (errno == EINTR) {
                 goto retry;
             } else {
-                error_setg_errno(errp, socket_error(),
+                error_setg_errno(errp, errno,
                                  "Unable to write to socket");
                 return -1;
             }
@@ -692,7 +692,7 @@ qio_channel_socket_close(QIOChannel *ioc,
 #endif
         if (closesocket(sioc->fd) < 0) {
             sioc->fd = -1;
-            error_setg_errno(errp, socket_error(),
+            error_setg_errno(errp, errno,
                              "Unable to close socket");
             return -1;
         }
@@ -723,7 +723,7 @@ qio_channel_socket_shutdown(QIOChannel *ioc,
     }
 
     if (shutdown(sioc->fd, sockhow) < 0) {
-        error_setg_errno(errp, socket_error(),
+        error_setg_errno(errp, errno,
                          "Unable to shutdown socket");
         return -1;
     }
diff --git a/migration/qemu-file-unix.c b/migration/qemu-file-unix.c
index 61b059b..4474e18 100644
--- a/migration/qemu-file-unix.c
+++ b/migration/qemu-file-unix.c
@@ -53,18 +53,16 @@ static ssize_t socket_writev_buffer(void *opaque, struct iovec *iov, int iovcnt,
         }
 
         if (size > 0) {
-            err = socket_error();
-
-            if (err != EAGAIN && err != EWOULDBLOCK) {
+            if (errno != EAGAIN && errno != EWOULDBLOCK) {
                 error_report("socket_writev_buffer: Got err=%d for (%zu/%zu)",
-                             err, (size_t)size, (size_t)len);
+                             errno, (size_t)size, (size_t)len);
                 /*
                  * If I've already sent some but only just got the error, I
                  * could return the amount validly sent so far and wait for the
                  * next call to report the error, but I'd rather flag the error
                  * immediately.
                  */
-                return -err;
+                return -errno;
             }
 
             /* Emulate blocking */
@@ -99,15 +97,15 @@ static ssize_t socket_get_buffer(void *opaque, uint8_t *buf, int64_t pos,
         if (len != -1) {
             break;
         }
-        if (socket_error() == EAGAIN) {
+        if (errno == EAGAIN) {
             yield_until_fd_readable(s->fd);
-        } else if (socket_error() != EINTR) {
+        } else if (errno != EINTR) {
             break;
         }
     }
 
     if (len == -1) {
-        len = -socket_error();
+        len = -errno;
     }
     return len;
 }
diff --git a/migration/tcp.c b/migration/tcp.c
index e888a4e..e1fa7f8 100644
--- a/migration/tcp.c
+++ b/migration/tcp.c
@@ -59,12 +59,11 @@ static void tcp_accept_incoming_migration(void *opaque)
     socklen_t addrlen = sizeof(addr);
     int s = (intptr_t)opaque;
     QEMUFile *f;
-    int c, err;
+    int c;
 
     do {
         c = qemu_accept(s, (struct sockaddr *)&addr, &addrlen);
-        err = socket_error();
-    } while (c < 0 && err == EINTR);
+    } while (c < 0 && errno == EINTR);
     qemu_set_fd_handler(s, NULL, NULL, NULL);
     closesocket(s);
 
@@ -72,7 +71,7 @@ static void tcp_accept_incoming_migration(void *opaque)
 
     if (c < 0) {
         error_report("could not accept migration connection (%s)",
-                     strerror(err));
+                     strerror(errno));
         return;
     }
 
diff --git a/net/socket.c b/net/socket.c
index e32e3cb..73dc49a 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -145,15 +145,14 @@ static void net_socket_send_completed(NetClientState *nc, ssize_t len)
 static void net_socket_send(void *opaque)
 {
     NetSocketState *s = opaque;
-    int size, err;
+    int size;
     unsigned l;
     uint8_t buf1[NET_BUFSIZE];
     const uint8_t *buf;
 
     size = qemu_recv(s->fd, buf1, sizeof(buf1), 0);
     if (size < 0) {
-        err = socket_error();
-        if (err != EWOULDBLOCK)
+        if (errno != EWOULDBLOCK)
             goto eoc;
     } else if (size == 0) {
         /* end of connection */
@@ -566,7 +565,7 @@ static int net_socket_connect_init(NetClientState *peer,
                                    const char *host_str)
 {
     NetSocketState *s;
-    int fd, connected, ret, err;
+    int fd, connected, ret;
     struct sockaddr_in saddr;
 
     if (parse_host_port(&saddr, host_str) < 0)
@@ -583,14 +582,12 @@ static int net_socket_connect_init(NetClientState *peer,
     for(;;) {
         ret = connect(fd, (struct sockaddr *)&saddr, sizeof(saddr));
         if (ret < 0) {
-            err = socket_error();
-            if (err == EINTR || err == EWOULDBLOCK) {
-            } else if (err == EINPROGRESS) {
-                break;
-#ifdef _WIN32
-            } else if (err == WSAEALREADY || err == WSAEINVAL) {
+            if (errno == EINTR || errno == EWOULDBLOCK) {
+                /* continue */
+            } else if (errno == EINPROGRESS ||
+                       errno == EALREADY ||
+                       errno == EINVAL) {
                 break;
-#endif
             } else {
                 perror("connect");
                 closesocket(fd);
diff --git a/slirp/tcp_input.c b/slirp/tcp_input.c
index 2027a75..03be56e 100644
--- a/slirp/tcp_input.c
+++ b/slirp/tcp_input.c
@@ -586,11 +586,7 @@ findso:
 	  }
 
 	  if ((tcp_fconnect(so, so->so_ffamily) == -1) &&
-#if defined(_WIN32)
-              socket_error() != WSAEWOULDBLOCK
-#else
               (errno != EINPROGRESS) && (errno != EWOULDBLOCK)
-#endif
           ) {
 	    u_char code=ICMP_UNREACH_NET;
 	    DEBUG_MISC((dfd, " tcp fconnect errno = %d-%s\n",
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index a6256de..a3f0664 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -145,7 +145,7 @@ int socket_set_fast_reuse(int fd)
 }
 
 
-int socket_error(void)
+static int socket_error(void)
 {
     switch (WSAGetLastError()) {
     case 0:
diff --git a/util/qemu-coroutine-io.c b/util/qemu-coroutine-io.c
index 0d5041c..91b9357 100644
--- a/util/qemu-coroutine-io.c
+++ b/util/qemu-coroutine-io.c
@@ -35,18 +35,16 @@ qemu_co_sendv_recvv(int sockfd, struct iovec *iov, unsigned iov_cnt,
 {
     size_t done = 0;
     ssize_t ret;
-    int err;
     while (done < bytes) {
         ret = iov_send_recv(sockfd, iov, iov_cnt,
                             offset + done, bytes - done, do_send);
         if (ret > 0) {
             done += ret;
         } else if (ret < 0) {
-            err = socket_error();
-            if (err == EAGAIN || err == EWOULDBLOCK) {
+            if (errno == EAGAIN || errno == EWOULDBLOCK) {
                 qemu_coroutine_yield();
             } else if (done == 0) {
-                return -err;
+                return -errno;
             } else {
                 break;
             }
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index ad7c00c..fd37ac2 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -268,7 +268,7 @@ static void wait_for_connect(void *opaque)
 
     do {
         rc = qemu_getsockopt(s->fd, SOL_SOCKET, SO_ERROR, &val, &valsize);
-    } while (rc == -1 && socket_error() == EINTR);
+    } while (rc == -1 && errno == EINTR);
 
     /* update rc to contain error */
     if (!rc && val) {
@@ -330,7 +330,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
     do {
         rc = 0;
         if (connect(sock, addr->ai_addr, addr->ai_addrlen) < 0) {
-            rc = -socket_error();
+            rc = -errno;
         }
     } while (rc == -EINTR);
 
@@ -787,7 +787,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp,
     do {
         rc = 0;
         if (connect(sock, (struct sockaddr *) &un, sizeof(un)) < 0) {
-            rc = -socket_error();
+            rc = -errno;
         }
     } while (rc == -EINTR);
 
@@ -1082,7 +1082,7 @@ SocketAddress *socket_local_address(int fd, Error **errp)
     socklen_t sslen = sizeof(ss);
 
     if (getsockname(fd, (struct sockaddr *)&ss, &sslen) < 0) {
-        error_setg_errno(errp, socket_error(), "%s",
+        error_setg_errno(errp, errno, "%s",
                          "Unable to query local socket address");
         return NULL;
     }
@@ -1097,7 +1097,7 @@ SocketAddress *socket_remote_address(int fd, Error **errp)
     socklen_t sslen = sizeof(ss);
 
     if (getpeername(fd, (struct sockaddr *)&ss, &sslen) < 0) {
-        error_setg_errno(errp, socket_error(), "%s",
+        error_setg_errno(errp, errno, "%s",
                          "Unable to query remote socket address");
         return NULL;
     }
-- 
2.5.0

Re: [Qemu-devel] [PATCH v2 00/18] Multiple fixes & improvements to QIOChannel & Win32

[Paolo Bonzini]

On 10/03/2016 18:26, Daniel P. Berrange wrote:
> This series started out as an attempt to fix the Win32 problems
> identified by Andrew Baumann
> 
>    https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg01343.html
> 
> It turned into a significantly larger cleanup of some chardev
> and osdep win32 portability code.
> 
> Patch 1 addresses Andrew's 2nd stated problem - handling of
> getpeername() failures, by fixing errno handling on Win32.
> 
> Patches 2-7 do some fixes in the test-io-channel-socket test
> case so that it is able to run on Win32.
> 
> Patches 8-12 are some fixes for the QIOChannel code
> 
> Patch 13 is the big one that changes QIOChannelSocket so that
> it uses a Win32 specific GSource implementation for creating
> watches. This is the key fix for Andrew's 1st stated problem.
> 
> At this point tests/test-io-channel-socket passes and
> 
>   qemu-system-x86_64.exe  -serial tcp:127.0.0.1:9000,server,nowait -device sga -display non
> 
> works on win32 once more.
> 
> Patches 14-16 are some cleanups to the chardev code to improve
> its clarity. They are not required for fixing any real problem
> 
> Patches 17-18 change the way we provide Win32 portability for
> sockets APIs inside QEMU. These do fix a number of bugs in the
> QEMU code related to mistaken use of errno instead of
> socket_error(). None of these bugs appear to be critical issues.
> 
> Based on this, I'm proposing 1-13 for QEMU 2.6 release as they
> fix critical win32 bugs.
> 
> Patches 14-18 can either be included in 2.6 or 2.7 - I'm
> ambivalent on which, since they're cleanups / minor fixes.

Thanks, please submit all of them in a pull request for 2.6.

We can then clean up EAGAIN vs. EWOULDBLOCK and add a checkpatch rule to
prevent further introduction of EWOULDBLOCK.

Paolo

Re: [Qemu-devel] [PATCH v2 00/18] Multiple fixes & improvements to QIOChannel & Win32

[Andrew Baumann]

Hi folks,

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Sent: Thursday, 10 March 2016 9:37 AM
> 
> On 10/03/2016 18:26, Daniel P. Berrange wrote:
> > This series started out as an attempt to fix the Win32 problems
> > identified by Andrew Baumann
> >
> >    https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg01343.html
> >
> > It turned into a significantly larger cleanup of some chardev
> > and osdep win32 portability code.
[...]

Sorry for chiming in a bit late here. I've tested these patches (the complete set, not individually), and they do appear to fix my immediate problem: socket char devices now work again. So thank you!

However, I'm now seeing a problem I don't believe we had before: very slow responses to GDB commands. From looking at a packet capture (using a localhost tcp socket between qemu and my gdb client), it seems that a couple of operations will go through just fine, and then there is a 1 second delay between my client's request and qemu's response. After fiddling with poll timeouts, it became clear that we were noticing the socket events when waking up from the poll, but the events themselves were still not waking us. It turns out that we were not calling WSAEventSelect on the accept path. At least, the following patch fixed the problem for me:

diff --git a/qemu-char.c b/qemu-char.c
index 3bf30b5..c1be622 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -3047,6 +3047,7 @@ static gboolean tcp_chr_accept(QIOChannel *channel,
         return TRUE;
     }

+    qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
     tcp_chr_new_client(chr, sioc);

     object_unref(OBJECT(sioc));

However, I'd note that both callers of tcp_chr_new_client() make the same call to set blocking to false immediately before calling tcp_chr_new_client(). Furthermore, the doc comment for qio_channel_set_blocking() appears to suggest that non-blocking mode is the default. If that's true, maybe you don't even want to rely on the caller explicitly setting blocking to false?

Cheers,
Andrew

Re: [Qemu-devel] [PATCH v2 00/18] Multiple fixes & improvements to QIOChannel & Win32

[Daniel P. Berrange]

On Fri, Mar 11, 2016 at 11:51:29PM +0000, Andrew Baumann wrote:
> Hi folks,
> 
> > From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> > Sent: Thursday, 10 March 2016 9:37 AM
> > 
> > On 10/03/2016 18:26, Daniel P. Berrange wrote:
> > > This series started out as an attempt to fix the Win32 problems
> > > identified by Andrew Baumann
> > >
> > >    https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg01343.html
> > >
> > > It turned into a significantly larger cleanup of some chardev
> > > and osdep win32 portability code.
> [...]
> 
> Sorry for chiming in a bit late here. I've tested these patches
> (the complete set, not individually), and they do appear to fix my
> immediate problem: socket char devices now work again. So thank you!

Thanks for confirming this, these patches have now merged into
git msater.

> However, I'm now seeing a problem I don't believe we had before:
> very slow responses to GDB commands. From looking at a packet
> capture (using a localhost tcp socket between qemu and my gdb
> client), it seems that a couple of operations will go through
> just fine, and then there is a 1 second delay between my client's
> request and qemu's response. After fiddling with poll timeouts,
> it became clear that we were noticing the socket events when
> waking up from the poll, but the events themselves were still
> not waking us. It turns out that we were not calling WSAEventSelect
> on the accept path. At least, the following patch fixed the
> problem for me:
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 3bf30b5..c1be622 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -3047,6 +3047,7 @@ static gboolean tcp_chr_accept(QIOChannel *channel,
>          return TRUE;
>      }
> 
> +    qio_channel_set_blocking(QIO_CHANNEL(sioc), false, NULL);
>      tcp_chr_new_client(chr, sioc);
> 
>      object_unref(OBJECT(sioc));
> 
> However, I'd note that both callers of tcp_chr_new_client()
> make the same call to set blocking to false immediately before
> calling tcp_chr_new_client(). Furthermore, the doc comment for
> qio_channel_set_blocking() appears to suggest that non-blocking
> mode is the default. If that's true, maybe you don't even want
> to rely on the caller explicitly setting blocking to false?

No, the docs don't intend to suggest that - the default is in
fact blocking mode, so its correct to place it into nonblocking
mode explicitly.

I think I didn't notice the problem you describe because my original
patch series had us call WSAEventSelect when creating the watch. This
indirectly puts Win32 sockets into non-blocking mode. The patches which
just merged however no longer call WSAEventSelect when creating the
watch, instead requiring the caller to explicitly set the socket into
non-blocking mode. So I think your suggested addition here is probably
the right way to address this. I'll investigate and respond with a
followup patch as needed.

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