[Qemu-devel] [PATCH v1 00/21] Multiple fixes & improves to QIOChannel & Win32

[Qemu-devel] [PATCH v1 00/21] Multiple fixes & improves 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.

Patch 8 is a simple error message fix.

Patch 9 adds missing EWOULDBLOCK handling to QIOChannelSocket

Patches 10-12 change QIOChannelSocket so that it uses a Win32
specific GSource implementation for creating watches. This is
the key fix for Andrew's 1st stated problem.

Patch 13 fixes a further problem identified with chardev on
Win32 - the 'wait' option behaves the same as 'nowait'.

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-20 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(), or close() instead fo closesocket() and
accept() instead of qemu_accept(). None of these bugs appear
to be critical issues.

Patch 21 improves error reporting with error_abort

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

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

Daniel P. Berrange (18):
  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: add missing EWOULDBLOCK checks in Win32 I/O code paths
  char: ensure listener socket is in blocking mode when waiting
  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 Win32 specific closesocket/ioctlsocket
  osdep: remove use of socket_error() from all code
  osdep: remove direct use of qemu_socket & qemu_accept
  error: ensure errno detail is printed with error_abort

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                        |  41 +++--
 contrib/ivshmem-server/ivshmem-server.c |   4 +-
 include/io/channel-watch.h              |  20 ++-
 include/io/channel.h                    |   3 +
 include/qemu/sockets.h                  |  19 --
 include/sysemu/os-posix.h               |  17 ++
 include/sysemu/os-win32.h               | 106 ++++++++---
 io/channel-socket.c                     |  63 ++++---
 io/channel-watch.c                      | 162 ++++++++++++++++-
 io/channel.c                            |  14 ++
 migration/qemu-file-unix.c              |  16 +-
 migration/tcp.c                         |  13 +-
 migration/unix.c                        |   2 +-
 net/socket.c                            |  49 +++--
 qemu-char.c                             |  97 ++++------
 qga/channel-posix.c                     |   4 +-
 slirp/ip_icmp.c                         |   4 +-
 slirp/misc.c                            |   8 +-
 slirp/slirp.h                           |   2 -
 slirp/socket.c                          |   4 +-
 slirp/tcp_input.c                       |   4 -
 slirp/tcp_subr.c                        |   8 +-
 slirp/udp.c                             |   6 +-
 tests/io-channel-helpers.c              |   6 +-
 tests/test-io-channel-socket.c          |  92 +++++-----
 util/error.c                            |  40 ++---
 util/osdep.c                            |  42 -----
 util/oslib-posix.c                      |  39 ++++
 util/oslib-win32.c                      | 304 +++++++++++++++++++++++++++++++-
 util/qemu-coroutine-io.c                |   6 +-
 util/qemu-sockets.c                     |  32 ++--
 32 files changed, 867 insertions(+), 364 deletions(-)

-- 
2.5.0

[Qemu-devel] [PATCH v1 01/21] 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.

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        | 77 ++++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 79 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..37b143b 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,81 @@ 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:
+        return EWOULDBLOCK;
+    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 v1 02/21] 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.

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 v1 03/21] io: initialize sockets in test program

[Daniel P. Berrange]

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

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 v1 04/21] 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.

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 v1 05/21] 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.

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 v1 06/21] 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.

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 v1 07/21] 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.

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 v1 08/21] io: fix copy+paste mistake in socket error message

[Daniel P. Berrange]

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

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 v1 09/21] io: add missing EWOULDBLOCK checks in Win32 I/O code paths

[Daniel P. Berrange]

On Win32 EWOULDBLOCK is not the same as EAGAIN, so we must
check both errnos after send/recv. Some places already
checked both, a couple of cases were previously missed.

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

diff --git a/io/channel-socket.c b/io/channel-socket.c
index 5f087e6..1de5cc0 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -559,7 +559,8 @@ static ssize_t qio_channel_socket_readv(QIOChannel *ioc,
                    iov[i].iov_len,
                    0);
         if (ret < 0) {
-            if (socket_error() == EAGAIN) {
+            if (socket_error() == EAGAIN ||
+                socket_error() == EWOULDBLOCK) {
                 if (done) {
                     return done;
                 } else {
@@ -601,7 +602,8 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
                    iov[i].iov_len,
                    0);
         if (ret < 0) {
-            if (socket_error() == EAGAIN) {
+            if (socket_error() == EAGAIN ||
+                socket_error() == EWOULDBLOCK) {
                 if (done) {
                     return done;
                 } else {
-- 
2.5.0

[Qemu-devel] [PATCH v1 10/21] io: pass HANDLE to g_source_add_poll on Win32

[Daniel P. Berrange]

From: Paolo Bonzini <pbonzini@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 v1 11/21] 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 1de5cc0..6f7f594 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -716,9 +716,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 v1 12/21] 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: Daniel P. Berrange <berrange@redhat.com>
---
 include/io/channel.h |   3 ++
 io/channel-socket.c  |   9 ++++
 io/channel-watch.c   | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 io/channel.c         |  14 ++++++
 4 files changed, 161 insertions(+), 1 deletion(-)

diff --git a/include/io/channel.h b/include/io/channel.h
index 0a1f1ce..20b973a 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 Win23 */
+#endif
 };
 
 /**
diff --git a/io/channel-socket.c b/io/channel-socket.c
index 6f7f594..ff49853 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
+    ((QIOChannel *)cioc)->event = CreateEvent(NULL, FALSE, FALSE, NULL);
+#endif
+
+
  retry:
     trace_qio_channel_socket_accept(ioc);
     cioc->fd = accept(ioc->fd, (struct sockaddr *)&cioc->remoteAddr,
diff --git a/io/channel-watch.c b/io/channel-watch.c
index dfac8f8..c5ee69f 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,104 @@ 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;
+}
+
+
+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);
+
+    /* WSAEnumNetworkEvents is edge-triggered, so we need a separate
+     * call to select to find which events are actually available.
+     * However, if there were no reported events at the time of the last
+     * call, and no new events since then, we know that the socket is
+     * quiescent.
+     */
+    if (!ssource->revents && !ev.lNetworkEvents) {
+        return 0;
+    }
+
+    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;
+
+    WSAEventSelect(ssource->socket, NULL, 0);
+    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 +289,29 @@ 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);
+    WSAEventSelect(ssource->socket, ioc->event,
+                   FD_READ | FD_ACCEPT | FD_CLOSE |
+                   FD_CONNECT | FD_WRITE | FD_OOB);
+
+    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 v1 13/21] char: ensure listener socket is in blocking mode when waiting

[Daniel P. Berrange]

When the network chardev is configured with 'server' and 'wait'
options, QEMU must block startup until the first client connects
to the server. This implies that the listener socket must be in
blocking mode. Unfortnantely on Win32 the socket is initially
in non-blocking mode, so we're not waiting for the first client.

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

diff --git a/qemu-char.c b/qemu-char.c
index e0147f3..18890f7 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -4376,6 +4376,7 @@ static CharDriverState *qmp_chardev_open_socket(const char *id,
     if (is_listen && is_waitconnect) {
         fprintf(stderr, "QEMU waiting for connection on: %s\n",
                 chr->filename);
+        qio_channel_set_blocking(QIO_CHANNEL(s->listen_ioc), true, NULL);
         tcp_chr_accept(QIO_CHANNEL(s->listen_ioc), G_IO_IN, chr);
         qio_channel_set_blocking(QIO_CHANNEL(s->listen_ioc), false, NULL);
     }
-- 
2.5.0

[Qemu-devel] [PATCH v1 14/21] 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 18890f7..917750b 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 v1 15/21] 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.

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 917750b..579c620 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 v1 16/21] 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 | 61 +++++++++++++++++++++++++------------------------------------
 1 file changed, 25 insertions(+), 36 deletions(-)

diff --git a/qemu-char.c b/qemu-char.c
index 579c620..fee2540 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,26 +4323,41 @@ static CharDriverState *qmp_chardev_open_socket(const char *id,
         s->reconnect_time = reconnect;
     }
 
+    sioc = qio_channel_socket_new();
+    qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
     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);
-        qio_channel_set_blocking(QIO_CHANNEL(s->listen_ioc), true, NULL);
-        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 v1 17/21] 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.

The only two methods which are non-trivial to handle are close()
and ioctl() since both of these logically apply both sockets and
regular file handles. The wrappers use a no-op getsockopt() call
to determine if the file handle is a socket or not, and thus
decide which underlying Win32 method to call.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 Makefile                  |   4 +-
 include/qemu/sockets.h    |  10 ---
 include/sysemu/os-posix.h |   6 ++
 include/sysemu/os-win32.h |  79 +++++++++++++++++
 util/oslib-win32.c        | 219 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 306 insertions(+), 12 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..fec2254 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -3,21 +3,11 @@
 #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 */
diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
index e9fec2e..718f35e 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);
diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
index 239771d..97503ce 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 ioctl
+#define ioctl qemu_ioctl_wrap
+int qemu_ioctl_wrap(int fd, int req, void *val);
+
+#undef close
+#define close qemu_close_wrap
+int qemu_close_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/util/oslib-win32.c b/util/oslib-win32.c
index 37b143b..b8c9cbe 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -579,3 +579,222 @@ 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;
+}
+
+
+static bool qemu_is_socket(int fd)
+{
+    int optval = 0;
+    socklen_t optlen;
+    optlen = sizeof(optval);
+    return getsockopt(fd, SOL_SOCKET, SO_TYPE,
+                      (char *)&optval, &optlen) == 0;
+}
+
+#undef ioctl
+int qemu_ioctl_wrap(int fd, int req, void *val)
+{
+    int ret;
+    if (qemu_is_socket(fd)) {
+        ret = ioctlsocket(fd, req, val);
+        if (ret < 0) {
+            errno = socket_error();
+        }
+    } else {
+        errno = EINVAL;
+        ret = -1;
+    }
+    return ret;
+}
+
+
+#undef close
+int qemu_close_wrap(int fd)
+{
+    int ret;
+    if (qemu_is_socket(fd)) {
+        ret = closesocket(fd);
+        if (ret < 0) {
+            errno = socket_error();
+        }
+    } else {
+        ret = close(fd);
+    }
+    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 v1 18/21] osdep: remove use of Win32 specific closesocket/ioctlsocket

[Daniel P. Berrange]

Now that QEMU replaces the close() and ioctl() methods with
a wrapper that can transparently handle both sockets and
regular file handles on Win32, there is no need to ever
use the closesocket/ioctlsocket methods. The code can simply
use the normal POSIX methods.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 block/sheepdog.c           | 36 ++++++++++++++++++------------------
 include/qemu/sockets.h     |  4 ----
 io/channel-socket.c        |  2 +-
 migration/qemu-file-unix.c |  2 +-
 migration/tcp.c            |  4 ++--
 net/socket.c               | 20 ++++++++++----------
 slirp/ip_icmp.c            |  2 +-
 slirp/misc.c               |  4 ++--
 slirp/slirp.h              |  2 --
 slirp/socket.c             |  2 +-
 slirp/tcp_subr.c           |  6 +++---
 slirp/udp.c                |  2 +-
 util/qemu-sockets.c        | 12 ++++++------
 13 files changed, 46 insertions(+), 52 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 8739acc..26d7bed 100644
--- a/block/sheepdog.c
+++ b/block/sheepdog.c
@@ -1143,7 +1143,7 @@ static int find_vdi_name(BDRVSheepdogState *s, const char *filename,
 
     ret = 0;
 out:
-    closesocket(fd);
+    close(fd);
     return ret;
 }
 
@@ -1340,7 +1340,7 @@ static int reload_inode(BDRVSheepdogState *s, uint32_t snapid, const char *tag)
 
 out:
     g_free(inode);
-    closesocket(fd);
+    close(fd);
 
     return ret;
 }
@@ -1488,7 +1488,7 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags,
     ret = read_object(fd, s->aio_context, buf, vid_to_vdi_oid(vid),
                       0, SD_INODE_SIZE, 0, s->cache_flags);
 
-    closesocket(fd);
+    close(fd);
 
     if (ret) {
         error_setg(errp, "Can't read snapshot inode");
@@ -1508,7 +1508,7 @@ out:
     aio_set_fd_handler(bdrv_get_aio_context(bs), s->fd,
                        false, NULL, NULL, NULL);
     if (s->fd >= 0) {
-        closesocket(s->fd);
+        close(s->fd);
     }
     qemu_opts_del(opts);
     g_free(buf);
@@ -1546,7 +1546,7 @@ static void sd_reopen_commit(BDRVReopenState *state)
     if (s->fd) {
         aio_set_fd_handler(s->aio_context, s->fd, false,
                            NULL, NULL, NULL);
-        closesocket(s->fd);
+        close(s->fd);
     }
 
     s->fd = re_s->fd;
@@ -1570,7 +1570,7 @@ static void sd_reopen_abort(BDRVReopenState *state)
     if (re_s->fd) {
         aio_set_fd_handler(s->aio_context, re_s->fd, false,
                            NULL, NULL, NULL);
-        closesocket(re_s->fd);
+        close(re_s->fd);
     }
 
     g_free(state->opaque);
@@ -1616,7 +1616,7 @@ static int do_sd_create(BDRVSheepdogState *s, uint32_t *vdi_id, int snapshot,
 
     ret = do_req(fd, s->aio_context, (SheepdogReq *)&hdr, buf, &wlen, &rlen);
 
-    closesocket(fd);
+    close(fd);
 
     if (ret) {
         error_setg_errno(errp, -ret, "create failed");
@@ -1879,7 +1879,7 @@ static int sd_create(const char *filename, QemuOpts *opts,
 
         ret = do_req(fd, s->aio_context, (SheepdogReq *)&hdr,
                      NULL, &wlen, &rlen);
-        closesocket(fd);
+        close(fd);
         if (ret) {
             error_setg_errno(errp, -ret, "failed to get cluster default");
             goto out;
@@ -1945,7 +1945,7 @@ static void sd_close(BlockDriverState *bs)
     ret = do_req(fd, s->aio_context, (SheepdogReq *)&hdr,
                  s->name, &wlen, &rlen);
 
-    closesocket(fd);
+    close(fd);
 
     if (!ret && rsp->result != SD_RES_SUCCESS &&
         rsp->result != SD_RES_VDI_NOT_LOCKED) {
@@ -1954,7 +1954,7 @@ static void sd_close(BlockDriverState *bs)
 
     aio_set_fd_handler(bdrv_get_aio_context(bs), s->fd,
                        false, NULL, NULL, NULL);
-    closesocket(s->fd);
+    close(s->fd);
     g_free(s->host_spec);
 }
 
@@ -2063,7 +2063,7 @@ static bool sd_delete(BDRVSheepdogState *s)
 
     ret = do_req(fd, s->aio_context, (SheepdogReq *)&hdr,
                  s->name, &wlen, &rlen);
-    closesocket(fd);
+    close(fd);
     if (ret) {
         return false;
     }
@@ -2120,7 +2120,7 @@ static int sd_create_branch(BDRVSheepdogState *s)
     ret = read_object(fd, s->aio_context, buf, vid_to_vdi_oid(vid),
                       s->inode.nr_copies, SD_INODE_SIZE, 0, s->cache_flags);
 
-    closesocket(fd);
+    close(fd);
 
     if (ret < 0) {
         goto out;
@@ -2432,7 +2432,7 @@ static int sd_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info)
 
 cleanup:
     g_free(inode);
-    closesocket(fd);
+    close(fd);
     return ret;
 }
 
@@ -2534,7 +2534,7 @@ static bool remove_objects(BDRVSheepdogState *s)
     }
 
 out:
-    closesocket(fd);
+    close(fd);
     return result;
 }
 
@@ -2590,7 +2590,7 @@ static int sd_snapshot_delete(BlockDriverState *bs,
 
     ret = do_req(fd, s->aio_context, (SheepdogReq *)&hdr,
                  buf, &wlen, &rlen);
-    closesocket(fd);
+    close(fd);
     if (ret) {
         return ret;
     }
@@ -2643,7 +2643,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
     ret = do_req(fd, s->aio_context, (SheepdogReq *)&req,
                  vdi_inuse, &wlen, &rlen);
 
-    closesocket(fd);
+    close(fd);
     if (ret) {
         goto out;
     }
@@ -2691,7 +2691,7 @@ static int sd_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab)
         }
     }
 
-    closesocket(fd);
+    close(fd);
 out:
     *psn_tab = sn_tab;
 
@@ -2753,7 +2753,7 @@ static int do_load_save_vmstate(BDRVSheepdogState *s, uint8_t *data,
     }
     ret = size;
 cleanup:
-    closesocket(fd);
+    close(fd);
     return ret;
 }
 
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index fec2254..1bd9218 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -6,10 +6,6 @@
 
 int inet_aton(const char *cp, struct in_addr *ia);
 
-#else
-
-#define closesocket(s) close(s)
-
 #endif /* !_WIN32 */
 
 #include "qapi-types.h"
diff --git a/io/channel-socket.c b/io/channel-socket.c
index ff49853..dced54d 100644
--- a/io/channel-socket.c
+++ b/io/channel-socket.c
@@ -682,7 +682,7 @@ qio_channel_socket_close(QIOChannel *ioc,
 {
     QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc);
 
-    if (closesocket(sioc->fd) < 0) {
+    if (close(sioc->fd) < 0) {
         sioc->fd = -1;
         error_setg_errno(errp, socket_error(),
                          "Unable to close socket");
diff --git a/migration/qemu-file-unix.c b/migration/qemu-file-unix.c
index 61b059b..e374ba2 100644
--- a/migration/qemu-file-unix.c
+++ b/migration/qemu-file-unix.c
@@ -115,7 +115,7 @@ static ssize_t socket_get_buffer(void *opaque, uint8_t *buf, int64_t pos,
 static int socket_close(void *opaque)
 {
     QEMUFileSocket *s = opaque;
-    closesocket(s->fd);
+    close(s->fd);
     g_free(s);
     return 0;
 }
diff --git a/migration/tcp.c b/migration/tcp.c
index e888a4e..3a31202 100644
--- a/migration/tcp.c
+++ b/migration/tcp.c
@@ -66,7 +66,7 @@ static void tcp_accept_incoming_migration(void *opaque)
         err = socket_error();
     } while (c < 0 && err == EINTR);
     qemu_set_fd_handler(s, NULL, NULL, NULL);
-    closesocket(s);
+    close(s);
 
     DPRINTF("accepted migration\n");
 
@@ -86,7 +86,7 @@ static void tcp_accept_incoming_migration(void *opaque)
     return;
 
 out:
-    closesocket(c);
+    close(c);
 }
 
 void tcp_start_incoming_migration(const char *host_port, Error **errp)
diff --git a/net/socket.c b/net/socket.c
index e32e3cb..fa18500 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -163,7 +163,7 @@ static void net_socket_send(void *opaque)
         if (s->listen_fd != -1) {
             qemu_set_fd_handler(s->listen_fd, net_socket_accept, NULL, s);
         }
-        closesocket(s->fd);
+        close(s->fd);
 
         s->fd = -1;
         s->state = 0;
@@ -325,7 +325,7 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
     return fd;
 fail:
     if (fd >= 0)
-        closesocket(fd);
+        close(fd);
     return -1;
 }
 
@@ -340,7 +340,7 @@ static void net_socket_cleanup(NetClientState *nc)
     }
     if (s->listen_fd != -1) {
         qemu_set_fd_handler(s->listen_fd, NULL, NULL, NULL);
-        closesocket(s->listen_fd);
+        close(s->listen_fd);
         s->listen_fd = -1;
     }
 }
@@ -417,7 +417,7 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
     return s;
 
 err:
-    closesocket(fd);
+    close(fd);
     return NULL;
 }
 
@@ -473,7 +473,7 @@ static NetSocketState *net_socket_fd_init(NetClientState *peer,
         (socklen_t *)&optlen)< 0) {
         fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n",
                 fd);
-        closesocket(fd);
+        close(fd);
         return NULL;
     }
     switch(so_type) {
@@ -540,13 +540,13 @@ static int net_socket_listen_init(NetClientState *peer,
     ret = bind(fd, (struct sockaddr *)&saddr, sizeof(saddr));
     if (ret < 0) {
         perror("bind");
-        closesocket(fd);
+        close(fd);
         return -1;
     }
     ret = listen(fd, 0);
     if (ret < 0) {
         perror("listen");
-        closesocket(fd);
+        close(fd);
         return -1;
     }
 
@@ -593,7 +593,7 @@ static int net_socket_connect_init(NetClientState *peer,
 #endif
             } else {
                 perror("connect");
-                closesocket(fd);
+                close(fd);
                 return -1;
             }
         } else {
@@ -675,13 +675,13 @@ static int net_socket_udp_init(NetClientState *peer,
 
     ret = socket_set_fast_reuse(fd);
     if (ret < 0) {
-        closesocket(fd);
+        close(fd);
         return -1;
     }
     ret = bind(fd, (struct sockaddr *)&laddr, sizeof(laddr));
     if (ret < 0) {
         perror("bind");
-        closesocket(fd);
+        close(fd);
         return -1;
     }
     qemu_set_nonblock(fd);
diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c
index ace3982..6c7fc63 100644
--- a/slirp/ip_icmp.c
+++ b/slirp/ip_icmp.c
@@ -110,7 +110,7 @@ static int icmp_send(struct socket *so, struct mbuf *m, int hlen)
 
 void icmp_detach(struct socket *so)
 {
-    closesocket(so->s);
+    close(so->s);
     sofree(so);
 }
 
diff --git a/slirp/misc.c b/slirp/misc.c
index e2eea2e..d7e39af 100644
--- a/slirp/misc.c
+++ b/slirp/misc.c
@@ -139,7 +139,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
 		    bind(s, (struct sockaddr *)&addr, addrlen) < 0 ||
 		    listen(s, 1) < 0) {
 			error_report("Error: inet socket: %s", strerror(errno));
-			closesocket(s);
+			close(s);
 
 			return 0;
 		}
@@ -213,7 +213,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
                 do {
                     so->s = accept(s, (struct sockaddr *)&addr, &addrlen);
                 } while (so->s < 0 && errno == EINTR);
-                closesocket(s);
+                close(s);
                 socket_set_fast_reuse(so->s);
                 opt = 1;
                 qemu_setsockopt(so->s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(int));
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/slirp/socket.c b/slirp/socket.c
index 2b5453e..f1b9190 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -491,7 +491,7 @@ sorecvfrom(struct socket *so)
 	   */
 	  len = M_FREEROOM(m);
 	  /* if (so->so_fport != htons(53)) { */
-	  ioctlsocket(so->s, FIONREAD, &n);
+	  ioctl(so->s, FIONREAD, &n);
 
 	  if (n > len) {
 	    n = (m->m_data - m->m_dat) + m->m_len + n + 1;
diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index b1aa1f2..6e07426 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -265,7 +265,7 @@ tcp_close(struct tcpcb *tp)
 	/* clobber input socket cache if we're closing the cached connection */
 	if (so == slirp->tcp_last_so)
 		slirp->tcp_last_so = &slirp->tcb;
-	closesocket(so->s);
+	close(so->s);
 	sbfree(&so->so_rcv);
 	sbfree(&so->so_snd);
 	sofree(so);
@@ -394,7 +394,7 @@ void tcp_connect(struct socket *inso)
         so = socreate(slirp);
         if (so == NULL) {
             /* If it failed, get rid of the pending connection */
-            closesocket(accept(inso->s, (struct sockaddr *)&addr, &addrlen));
+            close(accept(inso->s, (struct sockaddr *)&addr, &addrlen));
             return;
         }
         if (tcp_attach(so) < 0) {
@@ -425,7 +425,7 @@ void tcp_connect(struct socket *inso)
     /* Close the accept() socket, set right state */
     if (inso->so_state & SS_FACCEPTONCE) {
         /* If we only accept once, close the accept() socket */
-        closesocket(so->s);
+        close(so->s);
 
         /* Don't select it yet, even though we have an FD */
         /* if it's not FACCEPTONCE, it's already NOFDREF */
diff --git a/slirp/udp.c b/slirp/udp.c
index 6b39cab..6ec884a 100644
--- a/slirp/udp.c
+++ b/slirp/udp.c
@@ -291,7 +291,7 @@ udp_attach(struct socket *so, unsigned short af)
 void
 udp_detach(struct socket *so)
 {
-	closesocket(so->s);
+	close(so->s);
 	sofree(so);
 }
 
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index ad7c00c..cb811b0 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -211,7 +211,7 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
                 }
             }
         }
-        closesocket(slisten);
+        close(slisten);
     }
     freeaddrinfo(res);
     return -1;
@@ -219,7 +219,7 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
 listen:
     if (listen(slisten,1) != 0) {
         error_setg_errno(errp, errno, "Failed to listen on socket");
-        closesocket(slisten);
+        close(slisten);
         freeaddrinfo(res);
         return -1;
     }
@@ -279,7 +279,7 @@ static void wait_for_connect(void *opaque)
     /* connect error */
     if (rc < 0) {
         error_setg_errno(&err, errno, "Error connecting to socket");
-        closesocket(s->fd);
+        close(s->fd);
         s->fd = rc;
     }
 
@@ -340,7 +340,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
         *in_progress = true;
     } else if (rc < 0) {
         error_setg_errno(errp, errno, "Failed to connect socket");
-        closesocket(sock);
+        close(sock);
         return -1;
     }
     return sock;
@@ -530,7 +530,7 @@ static int inet_dgram_saddr(InetSocketAddress *sraddr,
 
 err:
     if (-1 != sock)
-        closesocket(sock);
+        close(sock);
     if (local)
         freeaddrinfo(local);
     if (peer)
@@ -751,7 +751,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
     return sock;
 
 err:
-    closesocket(sock);
+    close(sock);
     return -1;
 }
 
-- 
2.5.0

[Qemu-devel] [PATCH v1 19/21] 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        | 46 +++++++++++++++++++++++-----------------------
 migration/qemu-file-unix.c | 14 ++++++--------
 migration/tcp.c            |  7 +++----
 net/socket.c               | 19 ++++++++-----------
 slirp/tcp_input.c          |  4 ----
 util/oslib-win32.c         |  6 +++---
 util/qemu-coroutine-io.c   |  6 ++----
 util/qemu-sockets.c        | 10 +++++-----
 11 files changed, 52 insertions(+), 69 deletions(-)

diff --git a/block/sheepdog.c b/block/sheepdog.c
index 26d7bed..fbe37bd 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 718f35e..4f529d3 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
-
 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 97503ce..af08fba 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 dced54d..555fdaf 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;
     }
@@ -475,15 +475,15 @@ 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 (errno == EAGAIN ||
+            errno == EWOULDBLOCK) {
             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;
     }
@@ -535,14 +535,14 @@ 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 (errno == EAGAIN ||
+            errno == EWOULDBLOCK) {
             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;
     }
@@ -568,17 +568,17 @@ static ssize_t qio_channel_socket_readv(QIOChannel *ioc,
                    iov[i].iov_len,
                    0);
         if (ret < 0) {
-            if (socket_error() == EAGAIN ||
-                socket_error() == EWOULDBLOCK) {
+            if (errno == EAGAIN ||
+                errno == EWOULDBLOCK) {
                 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,17 +611,17 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc,
                    iov[i].iov_len,
                    0);
         if (ret < 0) {
-            if (socket_error() == EAGAIN ||
-                socket_error() == EWOULDBLOCK) {
+            if (errno == EAGAIN ||
+                errno == EWOULDBLOCK) {
                 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;
             }
@@ -684,7 +684,7 @@ qio_channel_socket_close(QIOChannel *ioc,
 
     if (close(sioc->fd) < 0) {
         sioc->fd = -1;
-        error_setg_errno(errp, socket_error(),
+        error_setg_errno(errp, errno,
                          "Unable to close socket");
         return -1;
     }
@@ -714,7 +714,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 e374ba2..0d2c4ad 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 3a31202..886ccfb 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);
     close(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 fa18500..b2d7a14 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");
                 close(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 b8c9cbe..77db075 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -124,13 +124,13 @@ void qemu_set_block(int fd)
 {
     unsigned long opt = 0;
     WSAEventSelect(fd, NULL, 0);
-    ioctlsocket(fd, FIONBIO, &opt);
+    ioctl(fd, FIONBIO, &opt);
 }
 
 void qemu_set_nonblock(int fd)
 {
     unsigned long opt = 1;
-    ioctlsocket(fd, FIONBIO, &opt);
+    ioctl(fd, FIONBIO, &opt);
     qemu_fd_register(fd);
 }
 
@@ -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 cb811b0..ae3c804 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

[Qemu-devel] [PATCH v1 20/21] osdep: remove direct use of qemu_socket & qemu_accept

[Daniel P. Berrange]

The qemu_socket & qemu_accept wrappers exist to ensure
the O_CLOEXEC flag is always set on new sockets. All
code is supposed to use these methods instead of the
normal POSIX socket/accept methods. This proves rather
error prone though with a number of places forgetting
to call the QEMU specific wrappers.

QEMU is already using a technique to transparently
wrap sockets APIs on Win32, avoiding the need for callers
to remember to use QEMU specific wrappers. This switches
over to that technique for socket/accept on POSIX platforms,
removing the need for callers to use the qemu_socket and
qemu_accept methods directly.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 contrib/ivshmem-server/ivshmem-server.c |  4 ++--
 include/qemu/sockets.h                  |  2 --
 include/sysemu/os-posix.h               | 11 +++++++++
 migration/tcp.c                         |  2 +-
 migration/unix.c                        |  2 +-
 net/socket.c                            | 10 ++++----
 qga/channel-posix.c                     |  4 ++--
 slirp/ip_icmp.c                         |  2 +-
 slirp/misc.c                            |  4 ++--
 slirp/socket.c                          |  2 +-
 slirp/tcp_subr.c                        |  2 +-
 slirp/udp.c                             |  4 ++--
 util/osdep.c                            | 42 ---------------------------------
 util/oslib-posix.c                      | 39 ++++++++++++++++++++++++++++++
 util/oslib-win32.c                      |  4 ++++
 util/qemu-sockets.c                     | 10 ++++----
 16 files changed, 77 insertions(+), 67 deletions(-)

diff --git a/contrib/ivshmem-server/ivshmem-server.c b/contrib/ivshmem-server/ivshmem-server.c
index bfd0fad..1b72ca0 100644
--- a/contrib/ivshmem-server/ivshmem-server.c
+++ b/contrib/ivshmem-server/ivshmem-server.c
@@ -142,8 +142,8 @@ ivshmem_server_handle_new_conn(IvshmemServer *server)
 
     /* accept the incoming connection */
     unaddr_len = sizeof(unaddr);
-    newfd = qemu_accept(server->sock_fd,
-                        (struct sockaddr *)&unaddr, &unaddr_len);
+    newfd = accept(server->sock_fd,
+                   (struct sockaddr *)&unaddr, &unaddr_len);
 
     if (newfd < 0) {
         IVSHMEM_SERVER_DEBUG(server, "cannot accept() %s\n", strerror(errno));
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 1bd9218..331cf5a 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -11,8 +11,6 @@ int inet_aton(const char *cp, struct in_addr *ia);
 #include "qapi-types.h"
 
 /* misc helpers */
-int qemu_socket(int domain, int type, int protocol);
-int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen);
 int socket_set_cork(int fd, int v);
 int socket_set_nodelay(int fd);
 void qemu_set_block(int fd);
diff --git a/include/sysemu/os-posix.h b/include/sysemu/os-posix.h
index 4f529d3..80cc84c 100644
--- a/include/sysemu/os-posix.h
+++ b/include/sysemu/os-posix.h
@@ -56,4 +56,15 @@ int qemu_utimens(const char *path, const qemu_timespec *times);
 
 bool is_daemonized(void);
 
+/* Some sockets wrappers to handle O_CLOEXEC */
+
+#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);
+
 #endif
diff --git a/migration/tcp.c b/migration/tcp.c
index 886ccfb..ce01e96 100644
--- a/migration/tcp.c
+++ b/migration/tcp.c
@@ -62,7 +62,7 @@ static void tcp_accept_incoming_migration(void *opaque)
     int c;
 
     do {
-        c = qemu_accept(s, (struct sockaddr *)&addr, &addrlen);
+        c = accept(s, (struct sockaddr *)&addr, &addrlen);
     } while (c < 0 && errno == EINTR);
     qemu_set_fd_handler(s, NULL, NULL, NULL);
     close(s);
diff --git a/migration/unix.c b/migration/unix.c
index d9aac36..b59ac96 100644
--- a/migration/unix.c
+++ b/migration/unix.c
@@ -62,7 +62,7 @@ static void unix_accept_incoming_migration(void *opaque)
     int c, err;
 
     do {
-        c = qemu_accept(s, (struct sockaddr *)&addr, &addrlen);
+        c = accept(s, (struct sockaddr *)&addr, &addrlen);
         err = errno;
     } while (c < 0 && err == EINTR);
     qemu_set_fd_handler(s, NULL, NULL, NULL);
diff --git a/net/socket.c b/net/socket.c
index b2d7a14..19addba 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -262,7 +262,7 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
         return -1;
 
     }
-    fd = qemu_socket(PF_INET, SOCK_DGRAM, 0);
+    fd = socket(PF_INET, SOCK_DGRAM, 0);
     if (fd < 0) {
         perror("socket(PF_INET, SOCK_DGRAM)");
         return -1;
@@ -497,7 +497,7 @@ static void net_socket_accept(void *opaque)
 
     for(;;) {
         len = sizeof(saddr);
-        fd = qemu_accept(s->listen_fd, (struct sockaddr *)&saddr, &len);
+        fd = accept(s->listen_fd, (struct sockaddr *)&saddr, &len);
         if (fd < 0 && errno != EINTR) {
             return;
         } else if (fd >= 0) {
@@ -527,7 +527,7 @@ static int net_socket_listen_init(NetClientState *peer,
     if (parse_host_port(&saddr, host_str) < 0)
         return -1;
 
-    fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
+    fd = socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {
         perror("socket");
         return -1;
@@ -571,7 +571,7 @@ static int net_socket_connect_init(NetClientState *peer,
     if (parse_host_port(&saddr, host_str) < 0)
         return -1;
 
-    fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
+    fd = socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {
         perror("socket");
         return -1;
@@ -664,7 +664,7 @@ static int net_socket_udp_init(NetClientState *peer,
         return -1;
     }
 
-    fd = qemu_socket(PF_INET, SOCK_DGRAM, 0);
+    fd = socket(PF_INET, SOCK_DGRAM, 0);
     if (fd < 0) {
         perror("socket(PF_INET, SOCK_DGRAM)");
         return -1;
diff --git a/qga/channel-posix.c b/qga/channel-posix.c
index 7ad3c00..c2d9440 100644
--- a/qga/channel-posix.c
+++ b/qga/channel-posix.c
@@ -31,8 +31,8 @@ static gboolean ga_channel_listen_accept(GIOChannel *channel,
 
     g_assert(channel != NULL);
 
-    client_fd = qemu_accept(g_io_channel_unix_get_fd(channel),
-                            (struct sockaddr *)&addr, &addrlen);
+    client_fd = accept(g_io_channel_unix_get_fd(channel),
+                       (struct sockaddr *)&addr, &addrlen);
     if (client_fd == -1) {
         g_warning("error converting fd to gsocket: %s", strerror(errno));
         goto out;
diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c
index 6c7fc63..e465f2e 100644
--- a/slirp/ip_icmp.c
+++ b/slirp/ip_icmp.c
@@ -79,7 +79,7 @@ static int icmp_send(struct socket *so, struct mbuf *m, int hlen)
     struct ip *ip = mtod(m, struct ip *);
     struct sockaddr_in addr;
 
-    so->s = qemu_socket(AF_INET, SOCK_DGRAM, IPPROTO_ICMP);
+    so->s = socket(AF_INET, SOCK_DGRAM, IPPROTO_ICMP);
     if (so->s == -1) {
         return -1;
     }
diff --git a/slirp/misc.c b/slirp/misc.c
index d7e39af..3c62c33 100644
--- a/slirp/misc.c
+++ b/slirp/misc.c
@@ -135,7 +135,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
 		addr.sin_port = 0;
 		addr.sin_addr.s_addr = INADDR_ANY;
 
-		if ((s = qemu_socket(AF_INET, SOCK_STREAM, 0)) < 0 ||
+		if ((s = socket(AF_INET, SOCK_STREAM, 0)) < 0 ||
 		    bind(s, (struct sockaddr *)&addr, addrlen) < 0 ||
 		    listen(s, 1) < 0) {
 			error_report("Error: inet socket: %s", strerror(errno));
@@ -162,7 +162,7 @@ fork_exec(struct socket *so, const char *ex, int do_pty)
                  * Connect to the socket
                  * XXX If any of these fail, we're in trouble!
                  */
-                s = qemu_socket(AF_INET, SOCK_STREAM, 0);
+                s = socket(AF_INET, SOCK_STREAM, 0);
                 addr.sin_addr = loopback_addr;
                 do {
                     ret = connect(s, (struct sockaddr *)&addr, addrlen);
diff --git a/slirp/socket.c b/slirp/socket.c
index f1b9190..e469e69 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -630,7 +630,7 @@ tcp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr,
 	addr.sin_addr.s_addr = haddr;
 	addr.sin_port = hport;
 
-	if (((s = qemu_socket(AF_INET,SOCK_STREAM,0)) < 0) ||
+	if (((s = socket(AF_INET, SOCK_STREAM, 0)) < 0) ||
 	    (socket_set_fast_reuse(s) < 0) ||
 	    (bind(s,(struct sockaddr *)&addr, sizeof(addr)) < 0) ||
 	    (listen(s,1) < 0)) {
diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
index 6e07426..dbbe742 100644
--- a/slirp/tcp_subr.c
+++ b/slirp/tcp_subr.c
@@ -332,7 +332,7 @@ int tcp_fconnect(struct socket *so, unsigned short af)
   DEBUG_CALL("tcp_fconnect");
   DEBUG_ARG("so = %p", so);
 
-  ret = so->s = qemu_socket(af, SOCK_STREAM, 0);
+  ret = so->s = socket(af, SOCK_STREAM, 0);
   if (ret >= 0) {
     int opt, s=so->s;
     struct sockaddr_storage addr;
diff --git a/slirp/udp.c b/slirp/udp.c
index 6ec884a..f2ca25c 100644
--- a/slirp/udp.c
+++ b/slirp/udp.c
@@ -280,7 +280,7 @@ int udp_output(struct socket *so, struct mbuf *m,
 int
 udp_attach(struct socket *so, unsigned short af)
 {
-  so->s = qemu_socket(af, SOCK_DGRAM, 0);
+  so->s = socket(af, SOCK_DGRAM, 0);
   if (so->s != -1) {
     so->so_expire = curtime + SO_EXPIRE;
     insque(so, &so->slirp->udb);
@@ -329,7 +329,7 @@ udp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr,
 	if (!so) {
 	    return NULL;
 	}
-	so->s = qemu_socket(AF_INET,SOCK_DGRAM,0);
+	so->s = socket(AF_INET, SOCK_DGRAM, 0);
 	so->so_expire = curtime + SO_EXPIRE;
 	insque(so, &slirp->udb);
 
diff --git a/util/osdep.c b/util/osdep.c
index 8356bdd..27898cf 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -267,48 +267,6 @@ ssize_t qemu_write_full(int fd, const void *buf, size_t count)
     return total;
 }
 
-/*
- * Opens a socket with FD_CLOEXEC set
- */
-int qemu_socket(int domain, int type, int protocol)
-{
-    int ret;
-
-#ifdef SOCK_CLOEXEC
-    ret = socket(domain, type | SOCK_CLOEXEC, protocol);
-    if (ret != -1 || errno != EINVAL) {
-        return ret;
-    }
-#endif
-    ret = socket(domain, type, protocol);
-    if (ret >= 0) {
-        qemu_set_cloexec(ret);
-    }
-
-    return ret;
-}
-
-/*
- * Accept a connection and set FD_CLOEXEC
- */
-int qemu_accept(int s, struct sockaddr *addr, socklen_t *addrlen)
-{
-    int ret;
-
-#ifdef CONFIG_ACCEPT4
-    ret = accept4(s, addr, addrlen, SOCK_CLOEXEC);
-    if (ret != -1 || errno != ENOSYS) {
-        return ret;
-    }
-#endif
-    ret = accept(s, addr, addrlen);
-    if (ret >= 0) {
-        qemu_set_cloexec(ret);
-    }
-
-    return ret;
-}
-
 void qemu_set_hw_version(const char *version)
 {
     hw_version = version;
diff --git a/util/oslib-posix.c b/util/oslib-posix.c
index 7615be4..c6ec8be 100644
--- a/util/oslib-posix.c
+++ b/util/oslib-posix.c
@@ -509,3 +509,42 @@ pid_t qemu_fork(Error **errp)
     }
     return pid;
 }
+
+
+#undef socket
+int qemu_socket_wrap(int domain, int type, int protocol)
+{
+    int ret;
+
+#ifdef SOCK_CLOEXEC
+    ret = socket(domain, type | SOCK_CLOEXEC, protocol);
+    if (ret != -1 || errno != EINVAL) {
+        return ret;
+    }
+#endif
+    ret = socket(domain, type, protocol);
+    if (ret >= 0) {
+        qemu_set_cloexec(ret);
+    }
+
+    return ret;
+}
+
+#undef accept
+int qemu_accept_wrap(int s, struct sockaddr *addr, socklen_t *addrlen)
+{
+    int ret;
+
+#ifdef CONFIG_ACCEPT4
+    ret = accept4(s, addr, addrlen, SOCK_CLOEXEC);
+    if (ret != -1 || errno != ENOSYS) {
+        return ret;
+    }
+#endif
+    ret = accept(s, addr, addrlen);
+    if (ret >= 0) {
+        qemu_set_cloexec(ret);
+    }
+
+    return ret;
+}
diff --git a/util/oslib-win32.c b/util/oslib-win32.c
index 77db075..0212360 100644
--- a/util/oslib-win32.c
+++ b/util/oslib-win32.c
@@ -626,6 +626,8 @@ int qemu_socket_wrap(int domain, int type, int protocol)
     ret = socket(domain, type, protocol);
     if (ret < 0) {
         errno = socket_error();
+    } else {
+        qemu_set_cloexec(ret);
     }
     return ret;
 }
@@ -639,6 +641,8 @@ int qemu_accept_wrap(int sockfd, struct sockaddr *addr,
     ret = accept(sockfd, addr, addrlen);
     if (ret < 0) {
         errno = socket_error();
+    } else {
+        qemu_set_cloexec(ret);
     }
     return ret;
 }
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index ae3c804..89ef32d 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -180,7 +180,7 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
         getnameinfo((struct sockaddr*)e->ai_addr,e->ai_addrlen,
 		        uaddr,INET6_ADDRSTRLEN,uport,32,
 		        NI_NUMERICHOST | NI_NUMERICSERV);
-        slisten = qemu_socket(e->ai_family, e->ai_socktype, e->ai_protocol);
+        slisten = socket(e->ai_family, e->ai_socktype, e->ai_protocol);
         if (slisten < 0) {
             if (!e->ai_next) {
                 error_setg_errno(errp, errno, "Failed to create socket");
@@ -317,7 +317,7 @@ static int inet_connect_addr(struct addrinfo *addr, bool *in_progress,
 
     *in_progress = false;
 
-    sock = qemu_socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
+    sock = socket(addr->ai_family, addr->ai_socktype, addr->ai_protocol);
     if (sock < 0) {
         error_setg_errno(errp, errno, "Failed to create socket");
         return -1;
@@ -505,7 +505,7 @@ static int inet_dgram_saddr(InetSocketAddress *sraddr,
     }
 
     /* create socket */
-    sock = qemu_socket(peer->ai_family, peer->ai_socktype, peer->ai_protocol);
+    sock = socket(peer->ai_family, peer->ai_socktype, peer->ai_protocol);
     if (sock < 0) {
         error_setg_errno(errp, errno, "Failed to create socket");
         goto err;
@@ -694,7 +694,7 @@ static int unix_listen_saddr(UnixSocketAddress *saddr,
     struct sockaddr_un un;
     int sock, fd;
 
-    sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
+    sock = socket(PF_UNIX, SOCK_STREAM, 0);
     if (sock < 0) {
         error_setg_errno(errp, errno, "Failed to create Unix socket");
         return -1;
@@ -767,7 +767,7 @@ static int unix_connect_saddr(UnixSocketAddress *saddr, Error **errp,
         return -1;
     }
 
-    sock = qemu_socket(PF_UNIX, SOCK_STREAM, 0);
+    sock = socket(PF_UNIX, SOCK_STREAM, 0);
     if (sock < 0) {
         error_setg_errno(errp, errno, "Failed to create socket");
         return -1;
-- 
2.5.0

[Qemu-devel] [PATCH v1 21/21] error: ensure errno detail is printed with error_abort

[Daniel P. Berrange]

When &error_abort is passed in, the error reporting code
will print the current error message and then abort() the
process. Unfortunately at the time it aborts, we've not
yet appended the errno detail. This makes debugging certain
problems significantly harder as the log is incomplete.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 util/error.c | 40 +++++++++++++++++++---------------------
 1 file changed, 19 insertions(+), 21 deletions(-)

diff --git a/util/error.c b/util/error.c
index 471b8b3..47f93af 100644
--- a/util/error.c
+++ b/util/error.c
@@ -44,7 +44,8 @@ static void error_handle_fatal(Error **errp, Error *err)
 
 static void error_setv(Error **errp,
                        const char *src, int line, const char *func,
-                       ErrorClass err_class, const char *fmt, va_list ap)
+                       ErrorClass err_class, const char *fmt, va_list ap,
+                       const char *suffix)
 {
     Error *err;
     int saved_errno = errno;
@@ -56,6 +57,11 @@ static void error_setv(Error **errp,
 
     err = g_malloc0(sizeof(*err));
     err->msg = g_strdup_vprintf(fmt, ap);
+    if (suffix) {
+        char *msg = err->msg;
+        err->msg = g_strdup_printf("%s: %s", msg, suffix);
+        g_free(msg);
+    }
     err->err_class = err_class;
     err->src = src;
     err->line = line;
@@ -74,7 +80,7 @@ void error_set_internal(Error **errp,
     va_list ap;
 
     va_start(ap, fmt);
-    error_setv(errp, src, line, func, err_class, fmt, ap);
+    error_setv(errp, src, line, func, err_class, fmt, ap, NULL);
     va_end(ap);
 }
 
@@ -85,7 +91,7 @@ void error_setg_internal(Error **errp,
     va_list ap;
 
     va_start(ap, fmt);
-    error_setv(errp, src, line, func, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
+    error_setv(errp, src, line, func, ERROR_CLASS_GENERIC_ERROR, fmt, ap, NULL);
     va_end(ap);
 }
 
@@ -94,7 +100,6 @@ void error_setg_errno_internal(Error **errp,
                                int os_errno, const char *fmt, ...)
 {
     va_list ap;
-    char *msg;
     int saved_errno = errno;
 
     if (errp == NULL) {
@@ -102,15 +107,10 @@ void error_setg_errno_internal(Error **errp,
     }
 
     va_start(ap, fmt);
-    error_setv(errp, src, line, func, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
+    error_setv(errp, src, line, func, ERROR_CLASS_GENERIC_ERROR, fmt, ap,
+               os_errno != 0 ? strerror(os_errno) : NULL);
     va_end(ap);
 
-    if (os_errno != 0) {
-        msg = (*errp)->msg;
-        (*errp)->msg = g_strdup_printf("%s: %s", msg, strerror(os_errno));
-        g_free(msg);
-    }
-
     errno = saved_errno;
 }
 
@@ -174,24 +174,22 @@ void error_setg_win32_internal(Error **errp,
                                int win32_err, const char *fmt, ...)
 {
     va_list ap;
-    char *msg1, *msg2;
+    char *suffix = NULL;
 
     if (errp == NULL) {
         return;
     }
 
+    if (win32_err != 0) {
+        suffix = g_win32_error_message(win32_err);
+    }
+
     va_start(ap, fmt);
-    error_setv(errp, src, line, func, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
+    error_setv(errp, src, line, func, ERROR_CLASS_GENERIC_ERROR,
+               fmt, ap, suffix);
     va_end(ap);
 
-    if (win32_err != 0) {
-        msg1 = (*errp)->msg;
-        msg2 = g_win32_error_message(win32_err);
-        (*errp)->msg = g_strdup_printf("%s: %s (error: %x)", msg1, msg2,
-                                       (unsigned)win32_err);
-        g_free(msg2);
-        g_free(msg1);
-    }
+    g_free(suffix);
 }
 
 #endif
-- 
2.5.0

Re: [Qemu-devel] [PATCH v1 12/21] io: implement socket watch for win32 using WSAEventSelect+select

[Paolo Bonzini]

On 09/03/2016 18:28, Daniel P. Berrange wrote:
> From: Paolo Bonzini <pbonzini@redhat.com>

Reviewing my own patch looks weird. :)

> 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: Daniel P. Berrange <berrange@redhat.com>
> ---
>  include/io/channel.h |   3 ++
>  io/channel-socket.c  |   9 ++++
>  io/channel-watch.c   | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  io/channel.c         |  14 ++++++
>  4 files changed, 161 insertions(+), 1 deletion(-)
> 
> diff --git a/include/io/channel.h b/include/io/channel.h
> index 0a1f1ce..20b973a 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 Win23 */

Even s390 would have Win24 and Win31 but not Win23. :)

> +#endif
>  };
>  
>  /**
> diff --git a/io/channel-socket.c b/io/channel-socket.c
> index 6f7f594..ff49853 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
> +    ((QIOChannel *)cioc)->event = CreateEvent(NULL, FALSE, FALSE, NULL);
> +#endif

QIO_CHANNEL(cioc)->event?

> +    WSAEventSelect(ssource->socket, NULL, 0);

This should probably be moved in qio_channel_socket_finalize.

> 
> +    /* WSAEnumNetworkEvents is edge-triggered, so we need a separate
> +     * call to select to find which events are actually available.
> +     * However, if there were no reported events at the time of the last
> +     * call, and no new events since then, we know that the socket is
> +     * quiescent.
> +     */
> +    if (!ssource->revents && !ev.lNetworkEvents) {
> +        return 0;
> +    }
> +

This is unfortunately unsafe, because:

1) WSAEventSelect clears all pending events (so the next
WSAEnumNetworkEvents returns no FD_READ, unless you have read all data
in the buffer with recv)

2) setting a socket to non-blocking should call WSAEventSelect (see
below), but cannot e.g. set ->revents to ~0 for all existing GSource.

It's probably possible to add a generation count or something like that,
but for now please revert this part (I had made it a separate commit in
my prototype because I wasn't sure if it was okay).

> +    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);
> +    WSAEventSelect(ssource->socket, ioc->event,
> +                   FD_READ | FD_ACCEPT | FD_CLOSE |
> +                   FD_CONNECT | FD_WRITE | FD_OOB);

This should be moved where the socket is made non-blocking in
qio_channel_socket_set_blocking (because qemu_qemu_set_nonblock also
calls WSAEventSelect).

It's probably worth adding a comment that
qio_channel_socket_source_check only works in non-blocking mode for Windows.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH v1 13/21] char: ensure listener socket is in blocking mode when waiting

[Paolo Bonzini]

On 09/03/2016 18:28, Daniel P. Berrange wrote:
> When the network chardev is configured with 'server' and 'wait'
> options, QEMU must block startup until the first client connects
> to the server. This implies that the listener socket must be in
> blocking mode. Unfortnantely on Win32 the socket is initially
> in non-blocking mode, so we're not waiting for the first client.

Shall we set the blocking/non-blocking mode uniformly for all platforms?
 Or is that something has created a GSource and this put the socket in
non-blocking mode?

Paolo

> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  qemu-char.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index e0147f3..18890f7 100644
> --- a/qemu-char.c
> +++ b/qemu-char.c
> @@ -4376,6 +4376,7 @@ static CharDriverState *qmp_chardev_open_socket(const char *id,
>      if (is_listen && is_waitconnect) {
>          fprintf(stderr, "QEMU waiting for connection on: %s\n",
>                  chr->filename);
> +        qio_channel_set_blocking(QIO_CHANNEL(s->listen_ioc), true, NULL);
>          tcp_chr_accept(QIO_CHANNEL(s->listen_ioc), G_IO_IN, chr);
>          qio_channel_set_blocking(QIO_CHANNEL(s->listen_ioc), false, NULL);
>      }
> 

Re: [Qemu-devel] [PATCH v1 14/21] char: remove qemu_chr_finish_socket_connection method

[Paolo Bonzini]

On 09/03/2016 18:28, Daniel P. Berrange wrote:
> @@ -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,

Aha, yes, this could be it.  If you move WSAEventSelect to
qio_channel_set_blocking, the previous patch will probably become
unnecessary.

Paolo

Re: [Qemu-devel] [PATCH v1 16/21] char: remove qemu_chr_open_socket_fd method

[Paolo Bonzini]

On 09/03/2016 18:28, Daniel P. Berrange wrote:
> 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 | 61 +++++++++++++++++++++++++------------------------------------
>  1 file changed, 25 insertions(+), 36 deletions(-)
> 
> diff --git a/qemu-char.c b/qemu-char.c
> index 579c620..fee2540 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,26 +4323,41 @@ static CharDriverState *qmp_chardev_open_socket(const char *id,
>          s->reconnect_time = reconnect;
>      }
>  
> +    sioc = qio_channel_socket_new();
> +    qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);

This might not be needed after all.

Paolo

>      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);
> -        qio_channel_set_blocking(QIO_CHANNEL(s->listen_ioc), true, NULL);
> -        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));
>      }
> 

Re: [Qemu-devel] [PATCH v1 19/21] osdep: remove use of socket_error() from all code

[Paolo Bonzini]

On 09/03/2016 18:28, Daniel P. Berrange wrote:
> diff --git a/util/oslib-win32.c b/util/oslib-win32.c
> index b8c9cbe..77db075 100644
> --- a/util/oslib-win32.c
> +++ b/util/oslib-win32.c
> @@ -124,13 +124,13 @@ void qemu_set_block(int fd)
>  {
>      unsigned long opt = 0;
>      WSAEventSelect(fd, NULL, 0);
> -    ioctlsocket(fd, FIONBIO, &opt);
> +    ioctl(fd, FIONBIO, &opt);
>  }
>  
>  void qemu_set_nonblock(int fd)
>  {
>      unsigned long opt = 1;
> -    ioctlsocket(fd, FIONBIO, &opt);
> +    ioctl(fd, FIONBIO, &opt);
>      qemu_fd_register(fd);
>  }

These should have been in patch 18, but see my answer to patch 17.

Paolo

Re: [Qemu-devel] [PATCH v1 20/21] osdep: remove direct use of qemu_socket & qemu_accept

[Paolo Bonzini]

On 09/03/2016 18:28, Daniel P. Berrange wrote:
> The qemu_socket & qemu_accept wrappers exist to ensure
> the O_CLOEXEC flag is always set on new sockets. All
> code is supposed to use these methods instead of the
> normal POSIX socket/accept methods. This proves rather
> error prone though with a number of places forgetting
> to call the QEMU specific wrappers.
> 
> QEMU is already using a technique to transparently
> wrap sockets APIs on Win32, avoiding the need for callers
> to remember to use QEMU specific wrappers. This switches
> over to that technique for socket/accept on POSIX platforms,
> removing the need for callers to use the qemu_socket and
> qemu_accept methods directly.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  contrib/ivshmem-server/ivshmem-server.c |  4 ++--
>  include/qemu/sockets.h                  |  2 --
>  include/sysemu/os-posix.h               | 11 +++++++++
>  migration/tcp.c                         |  2 +-
>  migration/unix.c                        |  2 +-
>  net/socket.c                            | 10 ++++----
>  qga/channel-posix.c                     |  4 ++--
>  slirp/ip_icmp.c                         |  2 +-
>  slirp/misc.c                            |  4 ++--
>  slirp/socket.c                          |  2 +-
>  slirp/tcp_subr.c                        |  2 +-
>  slirp/udp.c                             |  4 ++--
>  util/osdep.c                            | 42 ---------------------------------
>  util/oslib-posix.c                      | 39 ++++++++++++++++++++++++++++++
>  util/oslib-win32.c                      |  4 ++++
>  util/qemu-sockets.c                     | 10 ++++----
>  16 files changed, 77 insertions(+), 67 deletions(-)
> 
> diff --git a/contrib/ivshmem-server/ivshmem-server.c b/contrib/ivshmem-server/ivshmem-server.c
> index bfd0fad..1b72ca0 100644
> --- a/contrib/ivshmem-server/ivshmem-server.c
> +++ b/contrib/ivshmem-server/ivshmem-server.c
> @@ -142,8 +142,8 @@ ivshmem_server_handle_new_conn(IvshmemServer *server)
>  
>      /* accept the incoming connection */
>      unaddr_len = sizeof(unaddr);
> -    newfd = qemu_accept(server->sock_fd,
> -                        (struct sockaddr *)&unaddr, &unaddr_len);
> +    newfd = accept(server->sock_fd,
> +                   (struct sockaddr *)&unaddr, &unaddr_len);
>  
>      if (newfd < 0) {
>          IVSHMEM_SERVER_DEBUG(server, "cannot accept() %s\n", strerror(errno));

I disagree with this change, it adds too much magic.  Someone may see a

   fcntl(fd, F_SETFD, 0);

after a socket() call and think it's unnecessary.

Paolo

Re: [Qemu-devel] [PATCH v1 21/21] error: ensure errno detail is printed with error_abort

[Paolo Bonzini]

Ccing error.c maintainer.

Paolo

On 09/03/2016 18:28, Daniel P. Berrange wrote:
> When &error_abort is passed in, the error reporting code
> will print the current error message and then abort() the
> process. Unfortunately at the time it aborts, we've not
> yet appended the errno detail. This makes debugging certain
> problems significantly harder as the log is incomplete.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  util/error.c | 40 +++++++++++++++++++---------------------
>  1 file changed, 19 insertions(+), 21 deletions(-)
> 
> diff --git a/util/error.c b/util/error.c
> index 471b8b3..47f93af 100644
> --- a/util/error.c
> +++ b/util/error.c
> @@ -44,7 +44,8 @@ static void error_handle_fatal(Error **errp, Error *err)
>  
>  static void error_setv(Error **errp,
>                         const char *src, int line, const char *func,
> -                       ErrorClass err_class, const char *fmt, va_list ap)
> +                       ErrorClass err_class, const char *fmt, va_list ap,
> +                       const char *suffix)
>  {
>      Error *err;
>      int saved_errno = errno;
> @@ -56,6 +57,11 @@ static void error_setv(Error **errp,
>  
>      err = g_malloc0(sizeof(*err));
>      err->msg = g_strdup_vprintf(fmt, ap);
> +    if (suffix) {
> +        char *msg = err->msg;
> +        err->msg = g_strdup_printf("%s: %s", msg, suffix);
> +        g_free(msg);
> +    }
>      err->err_class = err_class;
>      err->src = src;
>      err->line = line;
> @@ -74,7 +80,7 @@ void error_set_internal(Error **errp,
>      va_list ap;
>  
>      va_start(ap, fmt);
> -    error_setv(errp, src, line, func, err_class, fmt, ap);
> +    error_setv(errp, src, line, func, err_class, fmt, ap, NULL);
>      va_end(ap);
>  }
>  
> @@ -85,7 +91,7 @@ void error_setg_internal(Error **errp,
>      va_list ap;
>  
>      va_start(ap, fmt);
> -    error_setv(errp, src, line, func, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
> +    error_setv(errp, src, line, func, ERROR_CLASS_GENERIC_ERROR, fmt, ap, NULL);
>      va_end(ap);
>  }
>  
> @@ -94,7 +100,6 @@ void error_setg_errno_internal(Error **errp,
>                                 int os_errno, const char *fmt, ...)
>  {
>      va_list ap;
> -    char *msg;
>      int saved_errno = errno;
>  
>      if (errp == NULL) {
> @@ -102,15 +107,10 @@ void error_setg_errno_internal(Error **errp,
>      }
>  
>      va_start(ap, fmt);
> -    error_setv(errp, src, line, func, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
> +    error_setv(errp, src, line, func, ERROR_CLASS_GENERIC_ERROR, fmt, ap,
> +               os_errno != 0 ? strerror(os_errno) : NULL);
>      va_end(ap);
>  
> -    if (os_errno != 0) {
> -        msg = (*errp)->msg;
> -        (*errp)->msg = g_strdup_printf("%s: %s", msg, strerror(os_errno));
> -        g_free(msg);
> -    }
> -
>      errno = saved_errno;
>  }
>  
> @@ -174,24 +174,22 @@ void error_setg_win32_internal(Error **errp,
>                                 int win32_err, const char *fmt, ...)
>  {
>      va_list ap;
> -    char *msg1, *msg2;
> +    char *suffix = NULL;
>  
>      if (errp == NULL) {
>          return;
>      }
>  
> +    if (win32_err != 0) {
> +        suffix = g_win32_error_message(win32_err);
> +    }
> +
>      va_start(ap, fmt);
> -    error_setv(errp, src, line, func, ERROR_CLASS_GENERIC_ERROR, fmt, ap);
> +    error_setv(errp, src, line, func, ERROR_CLASS_GENERIC_ERROR,
> +               fmt, ap, suffix);
>      va_end(ap);
>  
> -    if (win32_err != 0) {
> -        msg1 = (*errp)->msg;
> -        msg2 = g_win32_error_message(win32_err);
> -        (*errp)->msg = g_strdup_printf("%s: %s (error: %x)", msg1, msg2,
> -                                       (unsigned)win32_err);
> -        g_free(msg2);
> -        g_free(msg1);
> -    }
> +    g_free(suffix);
>  }
>  
>  #endif
> 

Re: [Qemu-devel] [PATCH v1 17/21] osdep: add wrappers for socket functions

[Paolo Bonzini]

On 09/03/2016 18:28, Daniel P. Berrange wrote:
> +static bool qemu_is_socket(int fd)
> +{
> +    int optval = 0;
> +    socklen_t optlen;
> +    optlen = sizeof(optval);
> +    return getsockopt(fd, SOL_SOCKET, SO_TYPE,
> +                      (char *)&optval, &optlen) == 0;

I think it's possible for a socket (which is actually a HANDLE) and a
file descriptor with the same numeric value to exist at the same time.
It's unlikely but it cannot be ruled out.

gnulib does this getsockopt (or at least could plausibly do it, I didn't
check :)) because it opens a libc file descriptor for every socket.
However, this only breaks ioctl and close and patch 18; neither ioctl
nor close's errors are ever checked in QEMU.
Doing the libc file descriptor in wrapping makes the select and poll
wrappers much more complex.  gnulib in fact even makes them work on
files, consoles, etc. besides pipes to provide a fuller POSIX layer.  In
my opinion this is beyond QEMU's scope and not really in line with
QEMU's attempts to use Win32 natively whenever applicable (e.g. in
qemu-char.c and block/raw-win32.c).

However, I'm okay with the other wrapping that you do in this patch.
Being able to drop socket_error() is a big improvement.

Paolo

> +}
> +
> +#undef ioctl
> +int qemu_ioctl_wrap(int fd, int req, void *val)
> +{
> +    int ret;
> +    if (qemu_is_socket(fd)) {
> +        ret = ioctlsocket(fd, req, val);
> +        if (ret < 0) {
> +            errno = socket_error();
> +        }
> +    } else {
> +        errno = EINVAL;
> +        ret = -1;
> +    }
> +    return ret;
> +}
> +
> +
> +#undef close
> +int qemu_close_wrap(int fd)
> +{
> +    int ret;
> +    if (qemu_is_socket(fd)) {
> +        ret = closesocket(fd);
> +        if (ret < 0) {
> +            errno = socket_error();
> +        }
> +    } else {
> +        ret = close(fd);
> +    }
> +    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;
> +}
> 

Re: [Qemu-devel] [PATCH v1 00/21] Multiple fixes & improves to QIOChannel & Win32

[Paolo Bonzini]

On 09/03/2016 18:28, 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.
> 
> Patch 8 is a simple error message fix.
> 
> Patch 9 adds missing EWOULDBLOCK handling to QIOChannelSocket
> 
> Patches 10-12 change QIOChannelSocket so that it uses a Win32
> specific GSource implementation for creating watches. This is
> the key fix for Andrew's 1st stated problem.
> 
> Patch 13 fixes a further problem identified with chardev on
> Win32 - the 'wait' option behaves the same as 'nowait'.
> 
> 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-20 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(), or close() instead fo closesocket() and
> accept() instead of qemu_accept(). None of these bugs appear
> to be critical issues.
> 
> Patch 21 improves error reporting with error_abort
> 
> Based on this, I'm proposing 1-13 for QEMU 2.6 release as they
> fix critical win32 bugs.
> 
> Patches 14-21 can either be included in 2.6 or 2.7 - I'm
> ambivalent on which, since they're cleanups / minor fixes.
> 
> Daniel P. Berrange (18):
>   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: add missing EWOULDBLOCK checks in Win32 I/O code paths
>   char: ensure listener socket is in blocking mode when waiting
>   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 Win32 specific closesocket/ioctlsocket
>   osdep: remove use of socket_error() from all code
>   osdep: remove direct use of qemu_socket & qemu_accept
>   error: ensure errno detail is printed with error_abort
> 
> 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                        |  41 +++--
>  contrib/ivshmem-server/ivshmem-server.c |   4 +-
>  include/io/channel-watch.h              |  20 ++-
>  include/io/channel.h                    |   3 +
>  include/qemu/sockets.h                  |  19 --
>  include/sysemu/os-posix.h               |  17 ++
>  include/sysemu/os-win32.h               | 106 ++++++++---
>  io/channel-socket.c                     |  63 ++++---
>  io/channel-watch.c                      | 162 ++++++++++++++++-
>  io/channel.c                            |  14 ++
>  migration/qemu-file-unix.c              |  16 +-
>  migration/tcp.c                         |  13 +-
>  migration/unix.c                        |   2 +-
>  net/socket.c                            |  49 +++--
>  qemu-char.c                             |  97 ++++------
>  qga/channel-posix.c                     |   4 +-
>  slirp/ip_icmp.c                         |   4 +-
>  slirp/misc.c                            |   8 +-
>  slirp/slirp.h                           |   2 -
>  slirp/socket.c                          |   4 +-
>  slirp/tcp_input.c                       |   4 -
>  slirp/tcp_subr.c                        |   8 +-
>  slirp/udp.c                             |   6 +-
>  tests/io-channel-helpers.c              |   6 +-
>  tests/test-io-channel-socket.c          |  92 +++++-----
>  util/error.c                            |  40 ++---
>  util/osdep.c                            |  42 -----
>  util/oslib-posix.c                      |  39 ++++
>  util/oslib-win32.c                      | 304 +++++++++++++++++++++++++++++++-
>  util/qemu-coroutine-io.c                |   6 +-
>  util/qemu-sockets.c                     |  32 ++--
>  32 files changed, 867 insertions(+), 364 deletions(-)
> 

For everything I didn't comment on:

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH v1 12/21] io: implement socket watch for win32 using WSAEventSelect+select

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 840 bytes --]

On 03/09/2016 10:47 AM, Paolo Bonzini wrote:
> On 09/03/2016 18:28, Daniel P. Berrange wrote:
>> From: Paolo Bonzini <pbonzini@redhat.com>
> 
> Reviewing my own patch looks weird. :)
> 
>> 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: Daniel P. Berrange <berrange@redhat.com>
>> ---

Especially when it lacks your S-o-b :)

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v1 12/21] io: implement socket watch for win32 using WSAEventSelect+select

[Paolo Bonzini]

[-- Attachment #1.1: Type: text/plain, Size: 921 bytes --]



On 09/03/2016 20:59, Eric Blake wrote:
> On 03/09/2016 10:47 AM, Paolo Bonzini wrote:
>> > On 09/03/2016 18:28, Daniel P. Berrange wrote:
>>> >> From: Paolo Bonzini <pbonzini@redhat.com>
>> > 
>> > Reviewing my own patch looks weird. :)
>> > 
>>> >> 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: Daniel P. Berrange <berrange@redhat.com>
>>> >> ---
> Especially when it lacks your S-o-b :)

I'm innocent! :)

https://github.com/bonzini/qemu/commit/win32-qio-watch^

Paolo


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v1 21/21] error: ensure errno detail is printed with error_abort

[Markus Armbruster]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> When &error_abort is passed in, the error reporting code
> will print the current error message and then abort() the
> process. Unfortunately at the time it aborts, we've not
> yet appended the errno detail. This makes debugging certain
> problems significantly harder as the log is incomplete.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

I can take this through my tree, but it's perhaps easier to let it flow
along with the rest of your series.

Re: [Qemu-devel] [PATCH v1 21/21] error: ensure errno detail is printed with error_abort

[Daniel P. Berrange]

On Thu, Mar 10, 2016 at 09:55:37AM +0100, Markus Armbruster wrote:
> "Daniel P. Berrange" <berrange@redhat.com> writes:
> 
> > When &error_abort is passed in, the error reporting code
> > will print the current error message and then abort() the
> > process. Unfortunately at the time it aborts, we've not
> > yet appended the errno detail. This makes debugging certain
> > problems significantly harder as the log is incomplete.
> >
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
> I can take this through my tree, but it's perhaps easier to let it flow
> along with the rest of your series.

Nah, it isn't critical to the rest of this series. Its just tacked
on as I noticed it while debugging tests, so you might as well just
take it through your tree as normal.

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] [PATCH v1 12/21] io: implement socket watch for win32 using WSAEventSelect+select

[Daniel P. Berrange]

On Wed, Mar 09, 2016 at 10:24:51PM +0100, Paolo Bonzini wrote:
> 
> 
> On 09/03/2016 20:59, Eric Blake wrote:
> > On 03/09/2016 10:47 AM, Paolo Bonzini wrote:
> >> > On 09/03/2016 18:28, Daniel P. Berrange wrote:
> >>> >> From: Paolo Bonzini <pbonzini@redhat.com>
> >> > 
> >> > Reviewing my own patch looks weird. :)
> >> > 
> >>> >> 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: Daniel P. Berrange <berrange@redhat.com>
> >>> >> ---
> > Especially when it lacks your S-o-b :)
> 
> I'm innocent! :)
> 
> https://github.com/bonzini/qemu/commit/win32-qio-watch^

Since I made non-trivial changes to this commit, I thought it was
corrrect to remove your S-o-b, to avoid claiming that you'd already
signed off on the changes I made. Was that not the right thing
todo ?


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] [PATCH v1 12/21] io: implement socket watch for win32 using WSAEventSelect+select

[Paolo Bonzini]

On 10/03/2016 10:41, Daniel P. Berrange wrote:
>> > 
>> > https://github.com/bonzini/qemu/commit/win32-qio-watch^
> Since I made non-trivial changes to this commit, I thought it was
> corrrect to remove your S-o-b, to avoid claiming that you'd already
> signed off on the changes I made. Was that not the right thing
> todo ?

You should have then also removed the authorship.  I think in this case
leaving the author and the s-o-b was the right thing to do, followed by
removing the authorship and leaving the s-o-b.  Signed-off-by is more of
a legal thing than a "I think that these changes are good for QEMU".

Paolo

Re: [Qemu-devel] [PATCH v1 12/21] io: implement socket watch for win32 using WSAEventSelect+select

[Daniel P. Berrange]

On Wed, Mar 09, 2016 at 06:47:29PM +0100, Paolo Bonzini wrote:
> On 09/03/2016 18:28, Daniel P. Berrange wrote:
> > From: Paolo Bonzini <pbonzini@redhat.com>
> 
> Reviewing my own patch looks weird. :)
> 
> > 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: Daniel P. Berrange <berrange@redhat.com>
> > ---
> >  include/io/channel.h |   3 ++
> >  io/channel-socket.c  |   9 ++++
> >  io/channel-watch.c   | 136 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  io/channel.c         |  14 ++++++
> >  4 files changed, 161 insertions(+), 1 deletion(-)


> > diff --git a/io/channel-socket.c b/io/channel-socket.c
> > index 6f7f594..ff49853 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
> > +    ((QIOChannel *)cioc)->event = CreateEvent(NULL, FALSE, FALSE, NULL);
> > +#endif
> 
> QIO_CHANNEL(cioc)->event?
> 
> > +    WSAEventSelect(ssource->socket, NULL, 0);
> 
> This should probably be moved in qio_channel_socket_finalize.

Yes, moved that.

> > +    /* WSAEnumNetworkEvents is edge-triggered, so we need a separate
> > +     * call to select to find which events are actually available.
> > +     * However, if there were no reported events at the time of the last
> > +     * call, and no new events since then, we know that the socket is
> > +     * quiescent.
> > +     */
> > +    if (!ssource->revents && !ev.lNetworkEvents) {
> > +        return 0;
> > +    }
> > +
> 
> This is unfortunately unsafe, because:
> 
> 1) WSAEventSelect clears all pending events (so the next
> WSAEnumNetworkEvents returns no FD_READ, unless you have read all data
> in the buffer with recv)
> 
> 2) setting a socket to non-blocking should call WSAEventSelect (see
> below), but cannot e.g. set ->revents to ~0 for all existing GSource.
> 
> It's probably possible to add a generation count or something like that,
> but for now please revert this part (I had made it a separate commit in
> my prototype because I wasn't sure if it was okay).

Ok, removing this chunk

> 
> > +    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);
> > +    WSAEventSelect(ssource->socket, ioc->event,
> > +                   FD_READ | FD_ACCEPT | FD_CLOSE |
> > +                   FD_CONNECT | FD_WRITE | FD_OOB);
> 
> This should be moved where the socket is made non-blocking in
> qio_channel_socket_set_blocking (because qemu_qemu_set_nonblock also
> calls WSAEventSelect).

Ok, I've moved that too.

> It's probably worth adding a comment that
> qio_channel_socket_source_check only works in non-blocking mode for Windows.

And added this.

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] [PATCH v1 14/21] char: remove qemu_chr_finish_socket_connection method

[Daniel P. Berrange]

On Wed, Mar 09, 2016 at 06:49:43PM +0100, Paolo Bonzini wrote:
> 
> 
> On 09/03/2016 18:28, Daniel P. Berrange wrote:
> > @@ -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,
> 
> Aha, yes, this could be it.  If you move WSAEventSelect to
> qio_channel_set_blocking, the previous patch will probably become
> unnecessary.

Yes, I've tested that and you're correct, so I've dropped the previous
patch.

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] [PATCH v1 16/21] char: remove qemu_chr_open_socket_fd method

[Daniel P. Berrange]

On Wed, Mar 09, 2016 at 06:53:29PM +0100, Paolo Bonzini wrote:
> 
> 
> On 09/03/2016 18:28, Daniel P. Berrange wrote:
> > 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 | 61 +++++++++++++++++++++++++------------------------------------
> >  1 file changed, 25 insertions(+), 36 deletions(-)
> > 
> > diff --git a/qemu-char.c b/qemu-char.c
> > index 579c620..fee2540 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,26 +4323,41 @@ static CharDriverState *qmp_chardev_open_socket(const char *id,
> >          s->reconnect_time = reconnect;
> >      }
> >  
> > +    sioc = qio_channel_socket_new();
> > +    qio_channel_set_blocking(QIO_CHANNEL(sioc), true, NULL);
> 
> This might not be needed after all.

Indeed, it isn't needed anymore.


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] [PATCH v1 17/21] osdep: add wrappers for socket functions

[Daniel P. Berrange]

On Wed, Mar 09, 2016 at 07:04:53PM +0100, Paolo Bonzini wrote:
> 
> 
> On 09/03/2016 18:28, Daniel P. Berrange wrote:
> > +static bool qemu_is_socket(int fd)
> > +{
> > +    int optval = 0;
> > +    socklen_t optlen;
> > +    optlen = sizeof(optval);
> > +    return getsockopt(fd, SOL_SOCKET, SO_TYPE,
> > +                      (char *)&optval, &optlen) == 0;
> 
> I think it's possible for a socket (which is actually a HANDLE) and a
> file descriptor with the same numeric value to exist at the same time.
> It's unlikely but it cannot be ruled out.

Aieee, nice one realizing that problem !

> gnulib does this getsockopt (or at least could plausibly do it, I didn't
> check :)) because it opens a libc file descriptor for every socket.
> However, this only breaks ioctl and close and patch 18; neither ioctl
> nor close's errors are ever checked in QEMU.
> Doing the libc file descriptor in wrapping makes the select and poll
> wrappers much more complex.  gnulib in fact even makes them work on
> files, consoles, etc. besides pipes to provide a fuller POSIX layer.  In
> my opinion this is beyond QEMU's scope and not really in line with
> QEMU's attempts to use Win32 natively whenever applicable (e.g. in
> qemu-char.c and block/raw-win32.c).
> 
> However, I'm okay with the other wrapping that you do in this patch.
> Being able to drop socket_error() is a big improvement.

Yep, so I've removed the wrapping of close/ioctl, and in their place
I'm wrapping closesocket/ioctlsocket to address errno/socket_error
handling in those.

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] [PATCH v1 18/21] osdep: remove use of Win32 specific closesocket/ioctlsocket

[Daniel P. Berrange]

On Wed, Mar 09, 2016 at 05:28:21PM +0000, Daniel P. Berrange wrote:
> Now that QEMU replaces the close() and ioctl() methods with
> a wrapper that can transparently handle both sockets and
> regular file handles on Win32, there is no need to ever
> use the closesocket/ioctlsocket methods. The code can simply
> use the normal POSIX methods.
> 
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
>  block/sheepdog.c           | 36 ++++++++++++++++++------------------
>  include/qemu/sockets.h     |  4 ----
>  io/channel-socket.c        |  2 +-
>  migration/qemu-file-unix.c |  2 +-
>  migration/tcp.c            |  4 ++--
>  net/socket.c               | 20 ++++++++++----------
>  slirp/ip_icmp.c            |  2 +-
>  slirp/misc.c               |  4 ++--
>  slirp/slirp.h              |  2 --
>  slirp/socket.c             |  2 +-
>  slirp/tcp_subr.c           |  6 +++---
>  slirp/udp.c                |  2 +-
>  util/qemu-sockets.c        | 12 ++++++------
>  13 files changed, 46 insertions(+), 52 deletions(-)

Given feedback from Paolo on the previous patch, I'm dropping this
patch.


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] [PATCH v1 01/21] osdep: fix socket_error() to work with Mingw64

[Paolo Bonzini]

On 09/03/2016 18:28, Daniel P. Berrange wrote:
> +    case WSAEWOULDBLOCK:
> +        return EWOULDBLOCK;

On mingw EAGAIN != EWOULDBLOCK.  Can you instead return EAGAIN here, so
that we can get rid of the "... || errno == EWOULDBLOCK" everywhere?

Paolo

Re: [Qemu-devel] [PATCH v1 01/21] osdep: fix socket_error() to work with Mingw64

[Daniel P. Berrange]

On Thu, Mar 10, 2016 at 05:12:36PM +0100, Paolo Bonzini wrote:
> 
> 
> On 09/03/2016 18:28, Daniel P. Berrange wrote:
> > +    case WSAEWOULDBLOCK:
> > +        return EWOULDBLOCK;
> 
> On mingw EAGAIN != EWOULDBLOCK.  Can you instead return EAGAIN here, so
> that we can get rid of the "... || errno == EWOULDBLOCK" everywhere?

Oh yes, that's a good idea.

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] [PATCH v1 12/21] io: implement socket watch for win32 using WSAEventSelect+select

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]

On 03/10/2016 02:54 AM, Paolo Bonzini wrote:
> 
> 
> On 10/03/2016 10:41, Daniel P. Berrange wrote:
>>>>
>>>> https://github.com/bonzini/qemu/commit/win32-qio-watch^
>> Since I made non-trivial changes to this commit, I thought it was
>> corrrect to remove your S-o-b, to avoid claiming that you'd already
>> signed off on the changes I made. Was that not the right thing
>> todo ?
> 
> You should have then also removed the authorship.  I think in this case
> leaving the author and the s-o-b was the right thing to do, followed by
> removing the authorship and leaving the s-o-b.  Signed-off-by is more of
> a legal thing than a "I think that these changes are good for QEMU".

I've seen this pattern of keeping original authorship and S-o-b, coupled
with an extension to the commit message, in the qemu.git history,
something like:

original title

original message
S-o-b: original
[make the following additional changes]
S-o-b: second author

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v1 21/21] error: ensure errno detail is printed with error_abort

[Markus Armbruster]

"Daniel P. Berrange" <berrange@redhat.com> writes:

> On Thu, Mar 10, 2016 at 09:55:37AM +0100, Markus Armbruster wrote:
>> "Daniel P. Berrange" <berrange@redhat.com> writes:
>> 
>> > When &error_abort is passed in, the error reporting code
>> > will print the current error message and then abort() the
>> > process. Unfortunately at the time it aborts, we've not
>> > yet appended the errno detail. This makes debugging certain
>> > problems significantly harder as the log is incomplete.
>> >
>> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>> 
>> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> 
>> I can take this through my tree, but it's perhaps easier to let it flow
>> along with the rest of your series.
>
> Nah, it isn't critical to the rest of this series. Its just tacked
> on as I noticed it while debugging tests, so you might as well just
> take it through your tree as normal.

Okay, applied to error-next, thanks!