[Qemu-devel] [PATCH v2 0/4] Convert non-blocking connect and fix its error reporting

[Qemu-devel] [PATCH v2 0/4] Convert non-blocking connect and fix its error reporting

[Mao Zhongyi]

v2:
* PATCH 02 reworking of patch 2 following Markus's suggestion that convert error_report()
  in the function called by net_socket_*_init() to Error. Also add many error handling
  information.
* PATCH 03 net_socket_mcast_create(), net_socket_fd_init_dgram() and net_socket_fd_init() 
  use the function such as fprintf, perror to report an error message. Convert it to Error.
* PATCH 04 parse_host_port() may fail without reporting an error. Now, fix it to set an
  error when it fails.

CC: berrange@redhat.com, kraxel@redhat.com, pbonzini@redhat.com, jasowang@redhat.com, armbru@redhat.com

Mao Zhongyi (4):
  net/socket: Convert the non-blocking connection mechanism to
    QIOchannel
  net/socket: Improve -net socket error reporting
  net/socket: Convert error report message to Error
  net/net: Convert parse_host_port() to Error

 include/qemu/sockets.h |   4 +-
 net/net.c              |  21 +++++--
 net/socket.c           | 159 ++++++++++++++++++++++++++++++-------------------
 util/qemu-sockets.c    |   2 +-
 4 files changed, 117 insertions(+), 69 deletions(-)

-- 
2.9.3

[Qemu-devel] [PATCH v2 1/4] net/socket: Convert the non-blocking connection mechanism to QIOchannel

[Mao Zhongyi]

Currently, socket connection in net is realized by an old
mechanism which is non-blocking.

That old mechanism may cause net blocks on DNS lookups and
QEmu has already replaced it with QIOchannel in many features,
such as migration.

Convert it to QIOchannel for net as well.

CC: berrange@redhat.com, pbonzini@redhat.com, jasowang@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
The test steps like this:

    $ qemu-system-x86_64 -net nic -net socket,listen=:1234 ~/img/test.img
    $ qemu-system-x86_64 -net nic -net socket,connect=127.0.0.1:1234 ~/img/test.img

No exception.

 net/socket.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index b8c931e..52f9dce 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -33,6 +33,7 @@
 #include "qemu/sockets.h"
 #include "qemu/iov.h"
 #include "qemu/main-loop.h"
+#include "io/channel-socket.h"
 
 typedef struct NetSocketState {
     NetClientState nc;
@@ -525,16 +526,22 @@ typedef struct {
     char *name;
 } socket_connect_data;
 
-static void socket_connect_data_free(socket_connect_data *c)
+static void socket_connect_data_free(void *opaque)
 {
+    socket_connect_data *c = opaque;
+    if (!c) {
+        return;
+    }
+
     qapi_free_SocketAddress(c->saddr);
     g_free(c->model);
     g_free(c->name);
     g_free(c);
 }
 
-static void net_socket_connected(int fd, Error *err, void *opaque)
+static void net_socket_connected(QIOTask *task, void *opaque)
 {
+    QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(qio_task_get_source(task));
     socket_connect_data *c = opaque;
     NetSocketState *s;
     char *addr_str = NULL;
@@ -543,13 +550,13 @@ static void net_socket_connected(int fd, Error *err, void *opaque)
     addr_str = socket_address_to_string(c->saddr, &local_error);
     if (addr_str == NULL) {
         error_report_err(local_error);
-        closesocket(fd);
+        closesocket(sioc->fd);
         goto end;
     }
 
-    s = net_socket_fd_init(c->peer, c->model, c->name, fd, true);
+    s = net_socket_fd_init(c->peer, c->model, c->name, sioc->fd, true);
     if (!s) {
-        closesocket(fd);
+        closesocket(sioc->fd);
         goto end;
     }
 
@@ -567,7 +574,7 @@ static int net_socket_connect_init(NetClientState *peer,
                                    const char *host_str)
 {
     socket_connect_data *c = g_new0(socket_connect_data, 1);
-    int fd = -1;
+    QIOChannelSocket *sioc;
     Error *local_error = NULL;
 
     c->peer = peer;
@@ -578,11 +585,12 @@ static int net_socket_connect_init(NetClientState *peer,
         goto err;
     }
 
-    fd = socket_connect(c->saddr, net_socket_connected, c, &local_error);
-    if (fd < 0) {
-        goto err;
-    }
-
+    sioc = qio_channel_socket_new();
+    qio_channel_socket_connect_async(sioc,
+                                     c->saddr,
+                                     net_socket_connected,
+                                     c,
+                                     NULL);
     return 0;
 
 err:
-- 
2.9.3

[Qemu-devel] [PATCH v2 2/4] net/socket: Improve -net socket error reporting

[Mao Zhongyi]

When -net socket fails, it first reports a specific error, then
a generic one, like this:

    $ qemu-system-x86_64 -net socket,
    qemu-system-x86_64: -net socket: exactly one of fd=, listen=, connect=, mcast= or udp= is required
    qemu-system-x86_64: -net socket: Device 'socket' could not be initialized

Convert net_init_socket() to Error to get rid of the unwanted second
error message.

CC: berrange@redhat.com, kraxel@redhat.com, pbonzini@redhat.com, jasowang@redhat.com, armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
 After the repair, the effect is as follows:

    $ qemu-system-x86_64 -net socket,
    qemu-system-x86_64: -net socket: exactly one of fd=, listen=, connect=, mcast= or udp= is required

 include/qemu/sockets.h |  2 +-
 net/socket.c           | 57 +++++++++++++++++++++++++++++---------------------
 util/qemu-sockets.c    |  2 +-
 3 files changed, 35 insertions(+), 26 deletions(-)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index af28532..528b802 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -118,7 +118,7 @@ SocketAddress *socket_remote_address(int fd, Error **errp);
  *
  * Returns: the socket address in string format, or NULL on error
  */
-char *socket_address_to_string(struct SocketAddress *addr, Error **errp);
+char *socket_address_to_string(struct SocketAddress *addr);
 
 /**
  * socket_address_crumple:
diff --git a/net/socket.c b/net/socket.c
index 52f9dce..b0decbe 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -486,7 +486,8 @@ static void net_socket_accept(void *opaque)
 static int net_socket_listen_init(NetClientState *peer,
                                   const char *model,
                                   const char *name,
-                                  const char *host_str)
+                                  const char *host_str,
+                                  Error **errp)
 {
     NetClientState *nc;
     NetSocketState *s;
@@ -496,14 +497,14 @@ static int net_socket_listen_init(NetClientState *peer,
 
     saddr = socket_parse(host_str, &local_error);
     if (saddr == NULL) {
-        error_report_err(local_error);
+        error_propagate(errp, local_error);
         return -1;
     }
 
     ret = socket_listen(saddr, &local_error);
     if (ret < 0) {
         qapi_free_SocketAddress(saddr);
-        error_report_err(local_error);
+        error_propagate(errp, local_error);
         return -1;
     }
 
@@ -545,11 +546,9 @@ static void net_socket_connected(QIOTask *task, void *opaque)
     socket_connect_data *c = opaque;
     NetSocketState *s;
     char *addr_str = NULL;
-    Error *local_error = NULL;
 
-    addr_str = socket_address_to_string(c->saddr, &local_error);
+    addr_str = socket_address_to_string(c->saddr);
     if (addr_str == NULL) {
-        error_report_err(local_error);
         closesocket(sioc->fd);
         goto end;
     }
@@ -571,7 +570,8 @@ end:
 static int net_socket_connect_init(NetClientState *peer,
                                    const char *model,
                                    const char *name,
-                                   const char *host_str)
+                                   const char *host_str,
+                                   Error **errp)
 {
     socket_connect_data *c = g_new0(socket_connect_data, 1);
     QIOChannelSocket *sioc;
@@ -594,7 +594,7 @@ static int net_socket_connect_init(NetClientState *peer,
     return 0;
 
 err:
-    error_report_err(local_error);
+    error_propagate(errp, local_error);
     socket_connect_data_free(c);
     return -1;
 }
@@ -603,7 +603,8 @@ static int net_socket_mcast_init(NetClientState *peer,
                                  const char *model,
                                  const char *name,
                                  const char *host_str,
-                                 const char *localaddr_str)
+                                 const char *localaddr_str,
+                                 Error **errp)
 {
     NetSocketState *s;
     int fd;
@@ -614,8 +615,11 @@ static int net_socket_mcast_init(NetClientState *peer,
         return -1;
 
     if (localaddr_str != NULL) {
-        if (inet_aton(localaddr_str, &localaddr) == 0)
+        if (inet_aton(localaddr_str, &localaddr) == 0) {
+            error_setg(errp, "Convert the local address to network"
+                        " byte order failed");
             return -1;
+        }
         param_localaddr = &localaddr;
     } else {
         param_localaddr = NULL;
@@ -642,7 +646,8 @@ static int net_socket_udp_init(NetClientState *peer,
                                  const char *model,
                                  const char *name,
                                  const char *rhost,
-                                 const char *lhost)
+                                 const char *lhost,
+                                 Error **errp)
 {
     NetSocketState *s;
     int fd, ret;
@@ -658,7 +663,7 @@ static int net_socket_udp_init(NetClientState *peer,
 
     fd = qemu_socket(PF_INET, SOCK_DGRAM, 0);
     if (fd < 0) {
-        perror("socket(PF_INET, SOCK_DGRAM)");
+        error_setg_errno(errp, errno, "socket(PF_INET, SOCK_DGRAM)");
         return -1;
     }
 
@@ -669,7 +674,7 @@ static int net_socket_udp_init(NetClientState *peer,
     }
     ret = bind(fd, (struct sockaddr *)&laddr, sizeof(laddr));
     if (ret < 0) {
-        perror("bind");
+        error_setg_errno(errp, errno, "bind");
         closesocket(fd);
         return -1;
     }
@@ -691,7 +696,6 @@ static int net_socket_udp_init(NetClientState *peer,
 int net_init_socket(const Netdev *netdev, const char *name,
                     NetClientState *peer, Error **errp)
 {
-    /* FIXME error_setg(errp, ...) on failure */
     Error *err = NULL;
     const NetdevSocketOptions *sock;
 
@@ -700,13 +704,13 @@ int net_init_socket(const Netdev *netdev, const char *name,
 
     if (sock->has_fd + sock->has_listen + sock->has_connect + sock->has_mcast +
         sock->has_udp != 1) {
-        error_report("exactly one of fd=, listen=, connect=, mcast= or udp="
+        error_setg(errp, "exactly one of fd=, listen=, connect=, mcast= or udp="
                      " is required");
         return -1;
     }
 
     if (sock->has_localaddr && !sock->has_mcast && !sock->has_udp) {
-        error_report("localaddr= is only valid with mcast= or udp=");
+        error_setg(errp, "localaddr= is only valid with mcast= or udp=");
         return -1;
     }
 
@@ -715,7 +719,7 @@ int net_init_socket(const Netdev *netdev, const char *name,
 
         fd = monitor_fd_param(cur_mon, sock->fd, &err);
         if (fd == -1) {
-            error_report_err(err);
+            error_propagate(errp, err);
             return -1;
         }
         qemu_set_nonblock(fd);
@@ -726,15 +730,18 @@ int net_init_socket(const Netdev *netdev, const char *name,
     }
 
     if (sock->has_listen) {
-        if (net_socket_listen_init(peer, "socket", name, sock->listen) == -1) {
+        if (net_socket_listen_init(peer, "socket", name, sock->listen,
+            &err) == -1) {
+            error_propagate(errp, err);
             return -1;
         }
         return 0;
     }
 
     if (sock->has_connect) {
-        if (net_socket_connect_init(peer, "socket", name, sock->connect) ==
-            -1) {
+        if (net_socket_connect_init(peer, "socket", name, sock->connect,
+            &err) == -1) {
+            error_propagate(errp, err);
             return -1;
         }
         return 0;
@@ -744,7 +751,8 @@ int net_init_socket(const Netdev *netdev, const char *name,
         /* if sock->localaddr is missing, it has been initialized to "all bits
          * zero" */
         if (net_socket_mcast_init(peer, "socket", name, sock->mcast,
-            sock->localaddr) == -1) {
+            sock->localaddr, &err) == -1) {
+            error_propagate(errp, err);
             return -1;
         }
         return 0;
@@ -752,11 +760,12 @@ int net_init_socket(const Netdev *netdev, const char *name,
 
     assert(sock->has_udp);
     if (!sock->has_localaddr) {
-        error_report("localaddr= is mandatory with udp=");
+        error_setg(errp, "localaddr= is mandatory with udp=");
         return -1;
     }
-    if (net_socket_udp_init(peer, "socket", name, sock->udp, sock->localaddr) ==
-        -1) {
+    if (net_socket_udp_init(peer, "socket", name, sock->udp, sock->localaddr,
+        &err) == -1) {
+        error_propagate(errp, err);
         return -1;
     }
     return 0;
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 8188d9a..0da33d7 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1312,7 +1312,7 @@ SocketAddress *socket_remote_address(int fd, Error **errp)
     return socket_sockaddr_to_address(&ss, sslen, errp);
 }
 
-char *socket_address_to_string(struct SocketAddress *addr, Error **errp)
+char *socket_address_to_string(struct SocketAddress *addr)
 {
     char *buf;
     InetSocketAddress *inet;
-- 
2.9.3

[Qemu-devel] [PATCH v2 3/4] net/socket: Convert error report message to Error

[Mao Zhongyi]

Currently, net_socket_mcast_create(), net_socket_fd_init_dgram() and
net_socket_fd_init() use the function such as fprintf(), perror() to
report an error message.

Now, convert these functions to Error.

CC: jasowang@redhat.com, armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
 net/socket.c | 66 +++++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 41 insertions(+), 25 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index b0decbe..559e09a 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -210,7 +210,9 @@ static void net_socket_send_dgram(void *opaque)
     }
 }
 
-static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr *localaddr)
+static int net_socket_mcast_create(struct sockaddr_in *mcastaddr,
+                                   struct in_addr *localaddr,
+                                   Error **errp)
 {
     struct ip_mreq imr;
     int fd;
@@ -222,8 +224,8 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
 #endif
 
     if (!IN_MULTICAST(ntohl(mcastaddr->sin_addr.s_addr))) {
-        fprintf(stderr, "qemu: error: specified mcastaddr \"%s\" (0x%08x) "
-                "does not contain a multicast address\n",
+        error_setg(errp, "qemu: error: specified mcastaddr %s (0x%08x) "
+                "does not contain a multicast address",
                 inet_ntoa(mcastaddr->sin_addr),
                 (int)ntohl(mcastaddr->sin_addr.s_addr));
         return -1;
@@ -231,7 +233,7 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
     }
     fd = qemu_socket(PF_INET, SOCK_DGRAM, 0);
     if (fd < 0) {
-        perror("socket(PF_INET, SOCK_DGRAM)");
+        error_setg_errno(errp, errno, "socket(PF_INET, SOCK_DGRAM)");
         return -1;
     }
 
@@ -243,13 +245,13 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
     val = 1;
     ret = qemu_setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val));
     if (ret < 0) {
-        perror("setsockopt(SOL_SOCKET, SO_REUSEADDR)");
+        error_setg_errno(errp, errno, "setsockopt(SOL_SOCKET, SO_REUSEADDR)");
         goto fail;
     }
 
     ret = bind(fd, (struct sockaddr *)mcastaddr, sizeof(*mcastaddr));
     if (ret < 0) {
-        perror("bind");
+        error_setg_errno(errp, errno, "bind");
         goto fail;
     }
 
@@ -264,7 +266,7 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
     ret = qemu_setsockopt(fd, IPPROTO_IP, IP_ADD_MEMBERSHIP,
                           &imr, sizeof(struct ip_mreq));
     if (ret < 0) {
-        perror("setsockopt(IP_ADD_MEMBERSHIP)");
+        error_setg_errno(errp, errno, "setsockopt(IP_ADD_MEMBERSHIP)");
         goto fail;
     }
 
@@ -273,7 +275,7 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
     ret = qemu_setsockopt(fd, IPPROTO_IP, IP_MULTICAST_LOOP,
                           &loop, sizeof(loop));
     if (ret < 0) {
-        perror("setsockopt(SOL_IP, IP_MULTICAST_LOOP)");
+        error_setg_errno(errp, errno, "setsockopt(SOL_IP, IP_MULTICAST_LOOP)");
         goto fail;
     }
 
@@ -282,7 +284,7 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
         ret = qemu_setsockopt(fd, IPPROTO_IP, IP_MULTICAST_IF,
                               localaddr, sizeof(*localaddr));
         if (ret < 0) {
-            perror("setsockopt(IP_MULTICAST_IF)");
+            error_setg_errno(errp, errno, "setsockopt(IP_MULTICAST_IF)");
             goto fail;
         }
     }
@@ -321,13 +323,15 @@ static NetClientInfo net_dgram_socket_info = {
 static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
                                                 const char *model,
                                                 const char *name,
-                                                int fd, int is_connected)
+                                                int fd, int is_connected,
+                                                Error **errp)
 {
     struct sockaddr_in saddr;
     int newfd;
     socklen_t saddr_len = sizeof(saddr);
     NetClientState *nc;
     NetSocketState *s;
+    Error *local_error = NULL;
 
     /* fd passed: multicast: "learn" dgram_dst address from bound address and save it
      * Because this may be "shared" socket from a "master" process, datagrams would be recv()
@@ -338,14 +342,14 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
         if (getsockname(fd, (struct sockaddr *) &saddr, &saddr_len) == 0) {
             /* must be bound */
             if (saddr.sin_addr.s_addr == 0) {
-                fprintf(stderr, "qemu: error: init_dgram: fd=%d unbound, "
-                        "cannot setup multicast dst addr\n", fd);
+                error_setg(errp, "qemu: error: init_dgram: fd=%d unbound, "
+                        "cannot setup multicast dst addr", fd);
                 goto err;
             }
             /* clone dgram socket */
-            newfd = net_socket_mcast_create(&saddr, NULL);
+            newfd = net_socket_mcast_create(&saddr, NULL, &local_error);
             if (newfd < 0) {
-                /* error already reported by net_socket_mcast_create() */
+                error_propagate(errp, local_error);
                 goto err;
             }
             /* clone newfd to fd, close newfd */
@@ -433,25 +437,27 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
 
 static NetSocketState *net_socket_fd_init(NetClientState *peer,
                                           const char *model, const char *name,
-                                          int fd, int is_connected)
+                                          int fd, int is_connected,
+                                          Error **errp)
 {
     int so_type = -1, optlen=sizeof(so_type);
 
     if(getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
         (socklen_t *)&optlen)< 0) {
-        fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n",
+        error_setg(errp, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed",
                 fd);
         closesocket(fd);
         return NULL;
     }
     switch(so_type) {
     case SOCK_DGRAM:
-        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected);
+        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected, errp);
     case SOCK_STREAM:
         return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
     default:
         /* who knows ... this could be a eg. a pty, do warn and continue as stream */
-        fprintf(stderr, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM or SOCK_STREAM\n", so_type, fd);
+        error_setg(errp, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM"
+                   " or SOCK_STREAM", so_type, fd);
         return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
     }
     return NULL;
@@ -546,6 +552,7 @@ static void net_socket_connected(QIOTask *task, void *opaque)
     socket_connect_data *c = opaque;
     NetSocketState *s;
     char *addr_str = NULL;
+    Error *local_error = NULL;
 
     addr_str = socket_address_to_string(c->saddr);
     if (addr_str == NULL) {
@@ -553,9 +560,10 @@ static void net_socket_connected(QIOTask *task, void *opaque)
         goto end;
     }
 
-    s = net_socket_fd_init(c->peer, c->model, c->name, sioc->fd, true);
+    s = net_socket_fd_init(c->peer, c->model, c->name, sioc->fd, true, &local_error);
     if (!s) {
         closesocket(sioc->fd);
+        error_report_err(local_error);
         goto end;
     }
 
@@ -610,6 +618,7 @@ static int net_socket_mcast_init(NetClientState *peer,
     int fd;
     struct sockaddr_in saddr;
     struct in_addr localaddr, *param_localaddr;
+    Error *local_error = NULL;
 
     if (parse_host_port(&saddr, host_str) < 0)
         return -1;
@@ -625,13 +634,17 @@ static int net_socket_mcast_init(NetClientState *peer,
         param_localaddr = NULL;
     }
 
-    fd = net_socket_mcast_create(&saddr, param_localaddr);
-    if (fd < 0)
+    fd = net_socket_mcast_create(&saddr, param_localaddr, &local_error);
+    if (fd < 0) {
+        error_propagate(errp, local_error);
         return -1;
+    }
 
-    s = net_socket_fd_init(peer, model, name, fd, 0);
-    if (!s)
+    s = net_socket_fd_init(peer, model, name, fd, 0, &local_error);
+    if (!s) {
+        error_propagate(errp, local_error);
         return -1;
+    }
 
     s->dgram_dst = saddr;
 
@@ -652,6 +665,7 @@ static int net_socket_udp_init(NetClientState *peer,
     NetSocketState *s;
     int fd, ret;
     struct sockaddr_in laddr, raddr;
+    Error *local_error = NULL;
 
     if (parse_host_port(&laddr, lhost) < 0) {
         return -1;
@@ -680,8 +694,9 @@ static int net_socket_udp_init(NetClientState *peer,
     }
     qemu_set_nonblock(fd);
 
-    s = net_socket_fd_init(peer, model, name, fd, 0);
+    s = net_socket_fd_init(peer, model, name, fd, 0, &local_error);
     if (!s) {
+        error_propagate(errp, local_error);
         return -1;
     }
 
@@ -723,7 +738,8 @@ int net_init_socket(const Netdev *netdev, const char *name,
             return -1;
         }
         qemu_set_nonblock(fd);
-        if (!net_socket_fd_init(peer, "socket", name, fd, 1)) {
+        if (!net_socket_fd_init(peer, "socket", name, fd, 1, &err)) {
+            error_propagate(errp, err);
             return -1;
         }
         return 0;
-- 
2.9.3

[Qemu-devel] [PATCH v2 4/4] net/net: Convert parse_host_port() to Error

[Mao Zhongyi]

CC: berrange@redhat.com, kraxel@redhat.com, pbonzini@redhat.com, jasowang@redhat.com, armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
 include/qemu/sockets.h |  2 +-
 net/net.c              | 21 ++++++++++++++++-----
 net/socket.c           | 10 +++++++---
 3 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 528b802..adb8d4f 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -53,7 +53,7 @@ void socket_listen_cleanup(int fd, Error **errp);
 int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
 
 /* Old, ipv4 only bits.  Don't use for new code. */
-int parse_host_port(struct sockaddr_in *saddr, const char *str);
+int parse_host_port(struct sockaddr_in *saddr, const char *str, Error **errp);
 int socket_init(void);
 
 /**
diff --git a/net/net.c b/net/net.c
index 0ac3b9e..5054035 100644
--- a/net/net.c
+++ b/net/net.c
@@ -99,7 +99,7 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
     return 0;
 }
 
-int parse_host_port(struct sockaddr_in *saddr, const char *str)
+int parse_host_port(struct sockaddr_in *saddr, const char *str, Error **errp)
 {
     char buf[512];
     struct hostent *he;
@@ -107,24 +107,35 @@ int parse_host_port(struct sockaddr_in *saddr, const char *str)
     int port;
 
     p = str;
-    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0)
+    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
+        error_setg(errp, "The mcast address should contain ':', for example: "
+                   "mcast=230.0.0.1:1234");
         return -1;
+    }
     saddr->sin_family = AF_INET;
     if (buf[0] == '\0') {
         saddr->sin_addr.s_addr = 0;
     } else {
         if (qemu_isdigit(buf[0])) {
-            if (!inet_aton(buf, &saddr->sin_addr))
+            if (!inet_aton(buf, &saddr->sin_addr)) {
+                error_setg(errp, "Convert the mcast address to network "
+                           "byte order failed.");
                 return -1;
+            }
         } else {
-            if ((he = gethostbyname(buf)) == NULL)
+            he = gethostbyname(buf);
+            if (he == NULL) {
+                error_setg(errp, "Specified hostname is error.");
                 return - 1;
+            }
             saddr->sin_addr = *(struct in_addr *)he->h_addr;
         }
     }
     port = strtol(p, (char **)&r, 0);
-    if (r == p)
+    if (r == p) {
+        error_setg(errp, "The port number is illegal");
         return -1;
+    }
     saddr->sin_port = htons(port);
     return 0;
 }
diff --git a/net/socket.c b/net/socket.c
index 559e09a..4373d9d 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -620,8 +620,10 @@ static int net_socket_mcast_init(NetClientState *peer,
     struct in_addr localaddr, *param_localaddr;
     Error *local_error = NULL;
 
-    if (parse_host_port(&saddr, host_str) < 0)
+    if (parse_host_port(&saddr, host_str, &local_error) < 0) {
+        error_propagate(errp, local_error);
         return -1;
+    }
 
     if (localaddr_str != NULL) {
         if (inet_aton(localaddr_str, &localaddr) == 0) {
@@ -667,11 +669,13 @@ static int net_socket_udp_init(NetClientState *peer,
     struct sockaddr_in laddr, raddr;
     Error *local_error = NULL;
 
-    if (parse_host_port(&laddr, lhost) < 0) {
+    if (parse_host_port(&laddr, lhost, &local_error) < 0) {
+        error_propagate(errp, local_error);
         return -1;
     }
 
-    if (parse_host_port(&raddr, rhost) < 0) {
+    if (parse_host_port(&raddr, rhost, &local_error) < 0) {
+        error_propagate(errp, local_error);
         return -1;
     }
 
-- 
2.9.3

Re: [Qemu-devel] [PATCH v2 1/4] net/socket: Convert the non-blocking connection mechanism to QIOchannel

[Markus Armbruster]

Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:

> Currently, socket connection in net is realized by an old
> mechanism which is non-blocking.
>
> That old mechanism may cause net blocks on DNS lookups and
> QEmu has already replaced it with QIOchannel in many features,
> such as migration.
>
> Convert it to QIOchannel for net as well.
>
> CC: berrange@redhat.com, pbonzini@redhat.com, jasowang@redhat.com
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
> The test steps like this:
>
>     $ qemu-system-x86_64 -net nic -net socket,listen=:1234 ~/img/test.img
>     $ qemu-system-x86_64 -net nic -net socket,connect=127.0.0.1:1234 ~/img/test.img
>
> No exception.
>
>  net/socket.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/net/socket.c b/net/socket.c
> index b8c931e..52f9dce 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -33,6 +33,7 @@
>  #include "qemu/sockets.h"
>  #include "qemu/iov.h"
>  #include "qemu/main-loop.h"
> +#include "io/channel-socket.h"
>  
>  typedef struct NetSocketState {
>      NetClientState nc;
> @@ -525,16 +526,22 @@ typedef struct {
>      char *name;
>  } socket_connect_data;
>  
> -static void socket_connect_data_free(socket_connect_data *c)
> +static void socket_connect_data_free(void *opaque)
>  {
> +    socket_connect_data *c = opaque;

Blank line between declarations and statements, please.

> +    if (!c) {
> +        return;
> +    }
> +
>      qapi_free_SocketAddress(c->saddr);
>      g_free(c->model);
>      g_free(c->name);
>      g_free(c);
>  }
>  
> -static void net_socket_connected(int fd, Error *err, void *opaque)
> +static void net_socket_connected(QIOTask *task, void *opaque)
>  {
> +    QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(qio_task_get_source(task));
>      socket_connect_data *c = opaque;
>      NetSocketState *s;
>      char *addr_str = NULL;
> @@ -543,13 +550,13 @@ static void net_socket_connected(int fd, Error *err, void *opaque)
>      addr_str = socket_address_to_string(c->saddr, &local_error);
>      if (addr_str == NULL) {
>          error_report_err(local_error);
> -        closesocket(fd);
> +        closesocket(sioc->fd);
>          goto end;
>      }
>  
> -    s = net_socket_fd_init(c->peer, c->model, c->name, fd, true);
> +    s = net_socket_fd_init(c->peer, c->model, c->name, sioc->fd, true);
>      if (!s) {
> -        closesocket(fd);
> +        closesocket(sioc->fd);
>          goto end;
>      }
>  
> @@ -567,7 +574,7 @@ static int net_socket_connect_init(NetClientState *peer,
>                                     const char *host_str)
>  {
>      socket_connect_data *c = g_new0(socket_connect_data, 1);
> -    int fd = -1;
> +    QIOChannelSocket *sioc;
>      Error *local_error = NULL;
>  
>      c->peer = peer;
> @@ -578,11 +585,12 @@ static int net_socket_connect_init(NetClientState *peer,
       c->model = g_strdup(model);
       c->name = g_strdup(name);
       c->saddr = socket_parse(host_str, &local_error);
       if (c->saddr == NULL) {
>          goto err;
>      }
>  
> -    fd = socket_connect(c->saddr, net_socket_connected, c, &local_error);
> -    if (fd < 0) {
> -        goto err;
> -    }
> -
> +    sioc = qio_channel_socket_new();
> +    qio_channel_socket_connect_async(sioc,
> +                                     c->saddr,
> +                                     net_socket_connected,
> +                                     c,
> +                                     NULL);
>      return 0;
>  
>  err:
       error_report_err(local_error);
       socket_connect_data_free(c);
       return -1;
   }

Ignorant question: how does this change the reporting of errors?

Before the patch, errors from socket_connect() are reported with
error_report_err(), just like errors from socket_parse().

After the patch?

The next patch converts this function to Error.  Errors from
socket_parse() are then propagated to the caller.  What about errors
from socket_connect()?

Re: [Qemu-devel] [PATCH v2 2/4] net/socket: Improve -net socket error reporting

[Markus Armbruster]

Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:

> When -net socket fails, it first reports a specific error, then
> a generic one, like this:
>
>     $ qemu-system-x86_64 -net socket,
>     qemu-system-x86_64: -net socket: exactly one of fd=, listen=, connect=, mcast= or udp= is required
>     qemu-system-x86_64: -net socket: Device 'socket' could not be initialized
>
> Convert net_init_socket() to Error to get rid of the unwanted second
> error message.
>
> CC: berrange@redhat.com, kraxel@redhat.com, pbonzini@redhat.com, jasowang@redhat.com, armbru@redhat.com
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
>  After the repair, the effect is as follows:
>
>     $ qemu-system-x86_64 -net socket,
>     qemu-system-x86_64: -net socket: exactly one of fd=, listen=, connect=, mcast= or udp= is required

Please move this into the commit message proper.

>
>  include/qemu/sockets.h |  2 +-
>  net/socket.c           | 57 +++++++++++++++++++++++++++++---------------------
>  util/qemu-sockets.c    |  2 +-
>  3 files changed, 35 insertions(+), 26 deletions(-)
>
> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> index af28532..528b802 100644
> --- a/include/qemu/sockets.h
> +++ b/include/qemu/sockets.h
> @@ -118,7 +118,7 @@ SocketAddress *socket_remote_address(int fd, Error **errp);
>   *
>   * Returns: the socket address in string format, or NULL on error
>   */
> -char *socket_address_to_string(struct SocketAddress *addr, Error **errp);
> +char *socket_address_to_string(struct SocketAddress *addr);
>  
>  /**
>   * socket_address_crumple:

I'd make this a separate patch.  Matter of taste.

> diff --git a/net/socket.c b/net/socket.c
> index 52f9dce..b0decbe 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -486,7 +486,8 @@ static void net_socket_accept(void *opaque)
>  static int net_socket_listen_init(NetClientState *peer,
>                                    const char *model,
>                                    const char *name,
> -                                  const char *host_str)
> +                                  const char *host_str,
> +                                  Error **errp)
>  {
>      NetClientState *nc;
>      NetSocketState *s;
> @@ -496,14 +497,14 @@ static int net_socket_listen_init(NetClientState *peer,
>  
>      saddr = socket_parse(host_str, &local_error);
>      if (saddr == NULL) {
> -        error_report_err(local_error);
> +        error_propagate(errp, local_error);
>          return -1;
>      }

Simpler:

  -    saddr = socket_parse(host_str, &local_error);
  +    saddr = socket_parse(host_str, errp);
       if (saddr == NULL) {
  -        error_report_err(local_error);
           return -1;
       }

>  
>      ret = socket_listen(saddr, &local_error);
>      if (ret < 0) {
>          qapi_free_SocketAddress(saddr);
> -        error_report_err(local_error);
> +        error_propagate(errp, local_error);
>          return -1;
>      }

Likewise.

>  
> @@ -545,11 +546,9 @@ static void net_socket_connected(QIOTask *task, void *opaque)
>      socket_connect_data *c = opaque;
>      NetSocketState *s;
>      char *addr_str = NULL;
> -    Error *local_error = NULL;
>  
> -    addr_str = socket_address_to_string(c->saddr, &local_error);
> +    addr_str = socket_address_to_string(c->saddr);
>      if (addr_str == NULL) {
> -        error_report_err(local_error);
>          closesocket(sioc->fd);
>          goto end;
>      }

On first glance, this looks like you're sweeping an error under the rug.
Not true, as socket_address_to_string() can't actually fail.  Please
drop the dead error handling entirely.

> @@ -571,7 +570,8 @@ end:
>  static int net_socket_connect_init(NetClientState *peer,
>                                     const char *model,
>                                     const char *name,
> -                                   const char *host_str)
> +                                   const char *host_str,
> +                                   Error **errp)
>  {
>      socket_connect_data *c = g_new0(socket_connect_data, 1);
>      QIOChannelSocket *sioc;
> @@ -594,7 +594,7 @@ static int net_socket_connect_init(NetClientState *peer,
       Error *local_error = NULL;

       c->peer = peer;
       c->model = g_strdup(model);
       c->name = g_strdup(name);
       c->saddr = socket_parse(host_str, &local_error);
       if (c->saddr == NULL) {
           goto err;
       }

       sioc = qio_channel_socket_new();
       qio_channel_socket_connect_async(sioc,
                                        c->saddr,
                                        net_socket_connected,
                                        c,
                                        NULL);
>      return 0;
>  
>  err:
> -    error_report_err(local_error);
> +    error_propagate(errp, local_error);
>      socket_connect_data_free(c);
>      return -1;
>  }

Since all we do with local_error is propagate it, we can eliminate it:
pass errp directly to socket_parse(), drop error_propagate().

> @@ -603,7 +603,8 @@ static int net_socket_mcast_init(NetClientState *peer,
>                                   const char *model,
>                                   const char *name,
>                                   const char *host_str,
> -                                 const char *localaddr_str)
> +                                 const char *localaddr_str,
> +                                 Error **errp)
>  {
>      NetSocketState *s;
>      int fd;
> @@ -614,8 +615,11 @@ static int net_socket_mcast_init(NetClientState *peer,
       struct sockaddr_in saddr;
       struct in_addr localaddr, *param_localaddr;

       if (parse_host_port(&saddr, host_str) < 0)
>          return -1;

Fails without setting an error.  Fixed in PATCH 4.  Recommend to do
PATCH 4 before this one.

>  
>      if (localaddr_str != NULL) {
> -        if (inet_aton(localaddr_str, &localaddr) == 0)
> +        if (inet_aton(localaddr_str, &localaddr) == 0) {
> +            error_setg(errp, "Convert the local address to network"
> +                        " byte order failed");

inet_aton() fails only when its first argument is not a valid IPv4
address.  Suggest:

    error_setg(errp, "localaddr '%s' is not a valid IPv4 address",
               localaddr_str);

This error message includes both the exact parameter name "localaddr"
and the parameter value.

>              return -1;
> +        }
>          param_localaddr = &localaddr;
>      } else {
>          param_localaddr = NULL;
       }

       fd = net_socket_mcast_create(&saddr, param_localaddr);
       if (fd < 0)
           return -1;

Fails without setting an error.  Fixed in PATCH 3.  Recommend to do
PATCH 3 before this one.

       s = net_socket_fd_init(peer, model, name, fd, 0);
       if (!s)
           return -1;

Likewise.

> @@ -642,7 +646,8 @@ static int net_socket_udp_init(NetClientState *peer,
>                                   const char *model,
>                                   const char *name,
>                                   const char *rhost,
> -                                 const char *lhost)
> +                                 const char *lhost,
> +                                 Error **errp)
>  {
>      NetSocketState *s;
>      int fd, ret;
> @@ -658,7 +663,7 @@ static int net_socket_udp_init(NetClientState *peer,
       struct sockaddr_in laddr, raddr;

       if (parse_host_port(&laddr, lhost) < 0) {
           return -1;

Fails without setting an error.  Fixed in PATCH 4.  Recommend to do
PATCH 4 before this one.

       }

       if (parse_host_port(&raddr, rhost) < 0) {
           return -1;

Likewise.

       }

>  
>      fd = qemu_socket(PF_INET, SOCK_DGRAM, 0);
>      if (fd < 0) {
> -        perror("socket(PF_INET, SOCK_DGRAM)");
> +        error_setg_errno(errp, errno, "socket(PF_INET, SOCK_DGRAM)");

Bad error message, but no worse than before.

>          return -1;
>      }
>  
> @@ -669,7 +674,7 @@ static int net_socket_udp_init(NetClientState *peer,
       ret = socket_set_fast_reuse(fd);
       if (ret < 0) {
           closesocket(fd);
           return -1;

Fails without setting an error.

>      }
>      ret = bind(fd, (struct sockaddr *)&laddr, sizeof(laddr));
>      if (ret < 0) {
> -        perror("bind");
> +        error_setg_errno(errp, errno, "bind");

Bad error message, but no worse than before.

>          closesocket(fd);
>          return -1;
>      }
> @@ -691,7 +696,6 @@ static int net_socket_udp_init(NetClientState *peer,
>  int net_init_socket(const Netdev *netdev, const char *name,
>                      NetClientState *peer, Error **errp)
>  {
> -    /* FIXME error_setg(errp, ...) on failure */
>      Error *err = NULL;
>      const NetdevSocketOptions *sock;
>  
> @@ -700,13 +704,13 @@ int net_init_socket(const Netdev *netdev, const char *name,
>  
>      if (sock->has_fd + sock->has_listen + sock->has_connect + sock->has_mcast +
>          sock->has_udp != 1) {
> -        error_report("exactly one of fd=, listen=, connect=, mcast= or udp="
> +        error_setg(errp, "exactly one of fd=, listen=, connect=, mcast= or udp="
>                       " is required");
>          return -1;
>      }
>  
>      if (sock->has_localaddr && !sock->has_mcast && !sock->has_udp) {
> -        error_report("localaddr= is only valid with mcast= or udp=");
> +        error_setg(errp, "localaddr= is only valid with mcast= or udp=");
>          return -1;
>      }
>  
> @@ -715,7 +719,7 @@ int net_init_socket(const Netdev *netdev, const char *name,
>  
>          fd = monitor_fd_param(cur_mon, sock->fd, &err);
>          if (fd == -1) {
> -            error_report_err(err);
> +            error_propagate(errp, err);
>              return -1;
>          }

Again, pass errp instead of &err, and drop error_propagate().

>          qemu_set_nonblock(fd);
           if (!net_socket_fd_init(peer, "socket", name, fd, 1)) {
               return -1;

Fails without setting an error.  Fixed in PATCH 3.  Recommend to do
PATCH 3 before this one.

           }
           return 0;
       }

> @@ -726,15 +730,18 @@ int net_init_socket(const Netdev *netdev, const char *name,
>      }
>  
>      if (sock->has_listen) {
> -        if (net_socket_listen_init(peer, "socket", name, sock->listen) == -1) {
> +        if (net_socket_listen_init(peer, "socket", name, sock->listen,
> +            &err) == -1) {
> +            error_propagate(errp, err);
>              return -1;
>          }
>          return 0;
>      }
>  
>      if (sock->has_connect) {
> -        if (net_socket_connect_init(peer, "socket", name, sock->connect) ==
> -            -1) {
> +        if (net_socket_connect_init(peer, "socket", name, sock->connect,
> +            &err) == -1) {
> +            error_propagate(errp, err);
>              return -1;
>          }
>          return 0;
> @@ -744,7 +751,8 @@ int net_init_socket(const Netdev *netdev, const char *name,
>          /* if sock->localaddr is missing, it has been initialized to "all bits
>           * zero" */
>          if (net_socket_mcast_init(peer, "socket", name, sock->mcast,
> -            sock->localaddr) == -1) {
> +            sock->localaddr, &err) == -1) {
> +            error_propagate(errp, err);
>              return -1;
>          }
>          return 0;
> @@ -752,11 +760,12 @@ int net_init_socket(const Netdev *netdev, const char *name,
>  
>      assert(sock->has_udp);
>      if (!sock->has_localaddr) {
> -        error_report("localaddr= is mandatory with udp=");
> +        error_setg(errp, "localaddr= is mandatory with udp=");
>          return -1;
>      }
> -    if (net_socket_udp_init(peer, "socket", name, sock->udp, sock->localaddr) ==
> -        -1) {
> +    if (net_socket_udp_init(peer, "socket", name, sock->udp, sock->localaddr,
> +        &err) == -1) {
> +        error_propagate(errp, err);
>          return -1;
>      }
>      return 0;
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 8188d9a..0da33d7 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1312,7 +1312,7 @@ SocketAddress *socket_remote_address(int fd, Error **errp)
>      return socket_sockaddr_to_address(&ss, sslen, errp);
>  }
>  
> -char *socket_address_to_string(struct SocketAddress *addr, Error **errp)
> +char *socket_address_to_string(struct SocketAddress *addr)
>  {
>      char *buf;
>      InetSocketAddress *inet;

Re: [Qemu-devel] [PATCH v2 1/4] net/socket: Convert the non-blocking connection mechanism to QIOchannel

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>
>> Currently, socket connection in net is realized by an old
>> mechanism which is non-blocking.
>>
>> That old mechanism may cause net blocks on DNS lookups and
>> QEmu has already replaced it with QIOchannel in many features,
>> such as migration.
>>
>> Convert it to QIOchannel for net as well.
>>
>> CC: berrange@redhat.com, pbonzini@redhat.com, jasowang@redhat.com
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> ---
>> The test steps like this:
>>
>>     $ qemu-system-x86_64 -net nic -net socket,listen=:1234 ~/img/test.img
>>     $ qemu-system-x86_64 -net nic -net socket,connect=127.0.0.1:1234 ~/img/test.img
>>
>> No exception.
>>
>>  net/socket.c | 30 +++++++++++++++++++-----------
>>  1 file changed, 19 insertions(+), 11 deletions(-)
>>
>> diff --git a/net/socket.c b/net/socket.c
>> index b8c931e..52f9dce 100644
>> --- a/net/socket.c
>> +++ b/net/socket.c
>> @@ -33,6 +33,7 @@
>>  #include "qemu/sockets.h"
>>  #include "qemu/iov.h"
>>  #include "qemu/main-loop.h"
>> +#include "io/channel-socket.h"
>>  
>>  typedef struct NetSocketState {
>>      NetClientState nc;
>> @@ -525,16 +526,22 @@ typedef struct {
>>      char *name;
>>  } socket_connect_data;
>>  
>> -static void socket_connect_data_free(socket_connect_data *c)
>> +static void socket_connect_data_free(void *opaque)
>>  {
>> +    socket_connect_data *c = opaque;
>
> Blank line between declarations and statements, please.
>
>> +    if (!c) {
>> +        return;
>> +    }
>> +
>>      qapi_free_SocketAddress(c->saddr);
>>      g_free(c->model);
>>      g_free(c->name);
>>      g_free(c);
>>  }
>>  
>> -static void net_socket_connected(int fd, Error *err, void *opaque)
>> +static void net_socket_connected(QIOTask *task, void *opaque)
>>  {
>> +    QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(qio_task_get_source(task));
>>      socket_connect_data *c = opaque;
>>      NetSocketState *s;
>>      char *addr_str = NULL;
>> @@ -543,13 +550,13 @@ static void net_socket_connected(int fd, Error *err, void *opaque)
>>      addr_str = socket_address_to_string(c->saddr, &local_error);
>>      if (addr_str == NULL) {
>>          error_report_err(local_error);
>> -        closesocket(fd);
>> +        closesocket(sioc->fd);
>>          goto end;
>>      }
>>  
>> -    s = net_socket_fd_init(c->peer, c->model, c->name, fd, true);
>> +    s = net_socket_fd_init(c->peer, c->model, c->name, sioc->fd, true);
>>      if (!s) {
>> -        closesocket(fd);
>> +        closesocket(sioc->fd);

Actually, net_socket_fd_init() closes sioc->fd when it fails.  Closing
it again in the caller is wrong.  None of the other callers does it.
Please drop this line in a separate patch, before this one.

>>          goto end;
>>      }
>>  
[...]

Re: [Qemu-devel] [PATCH v2 1/4] net/socket: Convert the non-blocking connection mechanism to QIOchannel

[Daniel P. Berrange]

On Wed, Apr 26, 2017 at 04:04:15PM +0800, Mao Zhongyi wrote:
> Currently, socket connection in net is realized by an old
> mechanism which is non-blocking.
> 
> That old mechanism may cause net blocks on DNS lookups and
> QEmu has already replaced it with QIOchannel in many features,
> such as migration.
> 
> Convert it to QIOchannel for net as well.
> 
> CC: berrange@redhat.com, pbonzini@redhat.com, jasowang@redhat.com
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
> The test steps like this:
> 
>     $ qemu-system-x86_64 -net nic -net socket,listen=:1234 ~/img/test.img
>     $ qemu-system-x86_64 -net nic -net socket,connect=127.0.0.1:1234 ~/img/test.img
> 
> No exception.
> 
>  net/socket.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/net/socket.c b/net/socket.c
> index b8c931e..52f9dce 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -33,6 +33,7 @@
>  #include "qemu/sockets.h"
>  #include "qemu/iov.h"
>  #include "qemu/main-loop.h"
> +#include "io/channel-socket.h"
>  
>  typedef struct NetSocketState {
>      NetClientState nc;
> @@ -525,16 +526,22 @@ typedef struct {
>      char *name;
>  } socket_connect_data;
>  
> -static void socket_connect_data_free(socket_connect_data *c)
> +static void socket_connect_data_free(void *opaque)
>  {
> +    socket_connect_data *c = opaque;
> +    if (!c) {
> +        return;
> +    }
> +
>      qapi_free_SocketAddress(c->saddr);
>      g_free(c->model);
>      g_free(c->name);
>      g_free(c);
>  }
>  
> -static void net_socket_connected(int fd, Error *err, void *opaque)
> +static void net_socket_connected(QIOTask *task, void *opaque)
>  {
> +    QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(qio_task_get_source(task));
>      socket_connect_data *c = opaque;
>      NetSocketState *s;
>      char *addr_str = NULL;
> @@ -543,13 +550,13 @@ static void net_socket_connected(int fd, Error *err, void *opaque)
>      addr_str = socket_address_to_string(c->saddr, &local_error);
>      if (addr_str == NULL) {
>          error_report_err(local_error);
> -        closesocket(fd);
> +        closesocket(sioc->fd);
>          goto end;
>      }
>  
> -    s = net_socket_fd_init(c->peer, c->model, c->name, fd, true);
> +    s = net_socket_fd_init(c->peer, c->model, c->name, sioc->fd, true);
>      if (!s) {
> -        closesocket(fd);
> +        closesocket(sioc->fd);
>          goto end;
>      }
>  
> @@ -567,7 +574,7 @@ static int net_socket_connect_init(NetClientState *peer,
>                                     const char *host_str)
>  {
>      socket_connect_data *c = g_new0(socket_connect_data, 1);
> -    int fd = -1;
> +    QIOChannelSocket *sioc;
>      Error *local_error = NULL;
>  
>      c->peer = peer;
> @@ -578,11 +585,12 @@ static int net_socket_connect_init(NetClientState *peer,
>          goto err;
>      }
>  
> -    fd = socket_connect(c->saddr, net_socket_connected, c, &local_error);
> -    if (fd < 0) {
> -        goto err;
> -    }
> -
> +    sioc = qio_channel_socket_new();
> +    qio_channel_socket_connect_async(sioc,
> +                                     c->saddr,
> +                                     net_socket_connected,
> +                                     c,
> +                                     NULL);
>      return 0;

You've not saved a copy of the QIOChannelSocket poiinter here, and
the net_socket_connected() method doesn't release the reference
either. So htere is a memory leak. Of course if you relesae the
reference in net_socket_connected(), then you need to dup() the
file descriptor you're borrowing.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 1/4] net/socket: Convert the non-blocking connection mechanism to QIOchannel

[Daniel P. Berrange]

On Thu, Apr 27, 2017 at 06:19:50PM +0200, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
> 
> > Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
> >
> >> Currently, socket connection in net is realized by an old
> >> mechanism which is non-blocking.
> >>
> >> That old mechanism may cause net blocks on DNS lookups and
> >> QEmu has already replaced it with QIOchannel in many features,
> >> such as migration.
> >>
> >> Convert it to QIOchannel for net as well.
> >>
> >> CC: berrange@redhat.com, pbonzini@redhat.com, jasowang@redhat.com
> >> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> >> ---
> >> The test steps like this:
> >>
> >>     $ qemu-system-x86_64 -net nic -net socket,listen=:1234 ~/img/test.img
> >>     $ qemu-system-x86_64 -net nic -net socket,connect=127.0.0.1:1234 ~/img/test.img
> >>
> >> No exception.
> >>
> >>  net/socket.c | 30 +++++++++++++++++++-----------
> >>  1 file changed, 19 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/net/socket.c b/net/socket.c
> >> index b8c931e..52f9dce 100644
> >> --- a/net/socket.c
> >> +++ b/net/socket.c
> >> @@ -33,6 +33,7 @@
> >>  #include "qemu/sockets.h"
> >>  #include "qemu/iov.h"
> >>  #include "qemu/main-loop.h"
> >> +#include "io/channel-socket.h"
> >>  
> >>  typedef struct NetSocketState {
> >>      NetClientState nc;
> >> @@ -525,16 +526,22 @@ typedef struct {
> >>      char *name;
> >>  } socket_connect_data;
> >>  
> >> -static void socket_connect_data_free(socket_connect_data *c)
> >> +static void socket_connect_data_free(void *opaque)
> >>  {
> >> +    socket_connect_data *c = opaque;
> >
> > Blank line between declarations and statements, please.
> >
> >> +    if (!c) {
> >> +        return;
> >> +    }
> >> +
> >>      qapi_free_SocketAddress(c->saddr);
> >>      g_free(c->model);
> >>      g_free(c->name);
> >>      g_free(c);
> >>  }
> >>  
> >> -static void net_socket_connected(int fd, Error *err, void *opaque)
> >> +static void net_socket_connected(QIOTask *task, void *opaque)
> >>  {
> >> +    QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(qio_task_get_source(task));
> >>      socket_connect_data *c = opaque;
> >>      NetSocketState *s;
> >>      char *addr_str = NULL;
> >> @@ -543,13 +550,13 @@ static void net_socket_connected(int fd, Error *err, void *opaque)
> >>      addr_str = socket_address_to_string(c->saddr, &local_error);
> >>      if (addr_str == NULL) {
> >>          error_report_err(local_error);
> >> -        closesocket(fd);
> >> +        closesocket(sioc->fd);
> >>          goto end;
> >>      }
> >>  
> >> -    s = net_socket_fd_init(c->peer, c->model, c->name, fd, true);
> >> +    s = net_socket_fd_init(c->peer, c->model, c->name, sioc->fd, true);
> >>      if (!s) {
> >> -        closesocket(fd);
> >> +        closesocket(sioc->fd);
> 
> Actually, net_socket_fd_init() closes sioc->fd when it fails.  Closing
> it again in the caller is wrong.  None of the other callers does it.
> Please drop this line in a separate patch, before this one.

NB, technically the 'fd' is still owned by the 'sioc' object, so nothing
should close it. If we weren't already leaking the 'sioc' object we would
have a double close here....

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 3/4] net/socket: Convert error report message to Error

[Markus Armbruster]

No review, just an observation.

Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:

> Currently, net_socket_mcast_create(), net_socket_fd_init_dgram() and
> net_socket_fd_init() use the function such as fprintf(), perror() to
> report an error message.
>
> Now, convert these functions to Error.
>
> CC: jasowang@redhat.com, armbru@redhat.com
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
>  net/socket.c | 66 +++++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 41 insertions(+), 25 deletions(-)
>
> diff --git a/net/socket.c b/net/socket.c
> index b0decbe..559e09a 100644
> --- a/net/socket.c
> +++ b/net/socket.c
[...]
> @@ -433,25 +437,27 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
>  
>  static NetSocketState *net_socket_fd_init(NetClientState *peer,
>                                            const char *model, const char *name,
> -                                          int fd, int is_connected)
> +                                          int fd, int is_connected,
> +                                          Error **errp)
>  {
>      int so_type = -1, optlen=sizeof(so_type);
>  
>      if(getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
>          (socklen_t *)&optlen)< 0) {
> -        fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n",
> +        error_setg(errp, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed",
>                  fd);
>          closesocket(fd);
>          return NULL;
>      }
>      switch(so_type) {
>      case SOCK_DGRAM:
> -        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected);
> +        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected, errp);
>      case SOCK_STREAM:
>          return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
>      default:
>          /* who knows ... this could be a eg. a pty, do warn and continue as stream */
> -        fprintf(stderr, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM or SOCK_STREAM\n", so_type, fd);
> +        error_setg(errp, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM"
> +                   " or SOCK_STREAM", so_type, fd);

Not this patches problem: this case is odd, and the comment is bogus.
If @fd really was a pty, getsockopt() would fail with ENOTSOCK, wouldn't
it?  If @fd is a socket, but neither SOCK_DGRAM nor SOCK_STREAM (say
SOCK_RAW), why is it safe to continue as if it was SOCK_STREAM?  Jason?

>          return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
>      }
>      return NULL;

Not reachable.  Ugly, but not your patch's concern.

[...]

Re: [Qemu-devel] [PATCH v2 0/4] Convert non-blocking connect and fix its error reporting

[Markus Armbruster]

Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:

> v2:
> * PATCH 02 reworking of patch 2 following Markus's suggestion that convert error_report()
>   in the function called by net_socket_*_init() to Error. Also add many error handling
>   information.
> * PATCH 03 net_socket_mcast_create(), net_socket_fd_init_dgram() and net_socket_fd_init() 
>   use the function such as fprintf, perror to report an error message. Convert it to Error.
> * PATCH 04 parse_host_port() may fail without reporting an error. Now, fix it to set an
>   error when it fails.

Needs a bit more work.  I'll continue review with v3.

Re: [Qemu-devel] [PATCH v2 3/4] net/socket: Convert error report message to Error

[Daniel P. Berrange]

On Thu, Apr 27, 2017 at 06:24:17PM +0200, Markus Armbruster wrote:
> No review, just an observation.
> 
> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
> 
> > Currently, net_socket_mcast_create(), net_socket_fd_init_dgram() and
> > net_socket_fd_init() use the function such as fprintf(), perror() to
> > report an error message.
> >
> > Now, convert these functions to Error.
> >
> > CC: jasowang@redhat.com, armbru@redhat.com
> > Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> > ---
> >  net/socket.c | 66 +++++++++++++++++++++++++++++++++++++-----------------------
> >  1 file changed, 41 insertions(+), 25 deletions(-)
> >
> > diff --git a/net/socket.c b/net/socket.c
> > index b0decbe..559e09a 100644
> > --- a/net/socket.c
> > +++ b/net/socket.c
> [...]
> > @@ -433,25 +437,27 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
> >  
> >  static NetSocketState *net_socket_fd_init(NetClientState *peer,
> >                                            const char *model, const char *name,
> > -                                          int fd, int is_connected)
> > +                                          int fd, int is_connected,
> > +                                          Error **errp)
> >  {
> >      int so_type = -1, optlen=sizeof(so_type);
> >  
> >      if(getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
> >          (socklen_t *)&optlen)< 0) {
> > -        fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n",
> > +        error_setg(errp, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed",
> >                  fd);
> >          closesocket(fd);
> >          return NULL;
> >      }
> >      switch(so_type) {
> >      case SOCK_DGRAM:
> > -        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected);
> > +        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected, errp);
> >      case SOCK_STREAM:
> >          return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
> >      default:
> >          /* who knows ... this could be a eg. a pty, do warn and continue as stream */
> > -        fprintf(stderr, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM or SOCK_STREAM\n", so_type, fd);
> > +        error_setg(errp, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM"
> > +                   " or SOCK_STREAM", so_type, fd);
> 
> Not this patches problem: this case is odd, and the comment is bogus.
> If @fd really was a pty, getsockopt() would fail with ENOTSOCK, wouldn't
> it?  If @fd is a socket, but neither SOCK_DGRAM nor SOCK_STREAM (say
> SOCK_RAW), why is it safe to continue as if it was SOCK_STREAM?  Jason?

IMHO it is a problem with this patch. Previously we merely printed
a warning & carried on, which is conceptually ok in general, though
dubious here for the reason you say.

Now we are filling an Error **errp object, and carrying on - this is
conceptually broken anywhere. If an Error ** is filled, we must return.
If we want to carry on, we shouldn't fill Error **.

> 
> >          return net_socket_fd_init_stream(peer, model, name, fd, is_connected);

IMHO, we just kill this and put return NULL here. If there is a genuine
reason to support something like SOCK_RAW, it should be explicitly
handled, because this default: case will certainly break SOCK_SEQPACKET
and SOCK_RDM which can't be treated as streams.

> >      }
> >      return NULL;
> 
> Not reachable.  Ugly, but not your patch's concern.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 3/4] net/socket: Convert error report message to Error

[Markus Armbruster]

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

> On Thu, Apr 27, 2017 at 06:24:17PM +0200, Markus Armbruster wrote:
>> No review, just an observation.
>> 
>> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>> 
>> > Currently, net_socket_mcast_create(), net_socket_fd_init_dgram() and
>> > net_socket_fd_init() use the function such as fprintf(), perror() to
>> > report an error message.
>> >
>> > Now, convert these functions to Error.
>> >
>> > CC: jasowang@redhat.com, armbru@redhat.com
>> > Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> > ---
>> >  net/socket.c | 66 +++++++++++++++++++++++++++++++++++++-----------------------
>> >  1 file changed, 41 insertions(+), 25 deletions(-)
>> >
>> > diff --git a/net/socket.c b/net/socket.c
>> > index b0decbe..559e09a 100644
>> > --- a/net/socket.c
>> > +++ b/net/socket.c
>> [...]
>> > @@ -433,25 +437,27 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
>> >  
>> >  static NetSocketState *net_socket_fd_init(NetClientState *peer,
>> >                                            const char *model, const char *name,
>> > -                                          int fd, int is_connected)
>> > +                                          int fd, int is_connected,
>> > +                                          Error **errp)
>> >  {
>> >      int so_type = -1, optlen=sizeof(so_type);
>> >  
>> >      if(getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
>> >          (socklen_t *)&optlen)< 0) {
>> > -        fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n",
>> > +        error_setg(errp, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed",
>> >                  fd);
>> >          closesocket(fd);
>> >          return NULL;
>> >      }
>> >      switch(so_type) {
>> >      case SOCK_DGRAM:
>> > -        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected);
>> > +        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected, errp);
>> >      case SOCK_STREAM:
>> >          return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
>> >      default:
>> >          /* who knows ... this could be a eg. a pty, do warn and continue as stream */
>> > -        fprintf(stderr, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM or SOCK_STREAM\n", so_type, fd);
>> > +        error_setg(errp, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM"
>> > +                   " or SOCK_STREAM", so_type, fd);
>> 
>> Not this patches problem: this case is odd, and the comment is bogus.
>> If @fd really was a pty, getsockopt() would fail with ENOTSOCK, wouldn't
>> it?  If @fd is a socket, but neither SOCK_DGRAM nor SOCK_STREAM (say
>> SOCK_RAW), why is it safe to continue as if it was SOCK_STREAM?  Jason?
>
> IMHO it is a problem with this patch. Previously we merely printed
> a warning & carried on, which is conceptually ok in general, though
> dubious here for the reason you say.
>
> Now we are filling an Error **errp object, and carrying on - this is
> conceptually broken anywhere. If an Error ** is filled, we must return.
> If we want to carry on, we shouldn't fill Error **.

You're right.

>> >          return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
>
> IMHO, we just kill this and put return NULL here. If there is a genuine
> reason to support something like SOCK_RAW, it should be explicitly
> handled, because this default: case will certainly break SOCK_SEQPACKET
> and SOCK_RDM which can't be treated as streams.

It's either magic or misguided defensive programming.  Probably the
latter, but I'd like to hear Jason's opinion.

If it's *necessary* magic, we can't use error_setg().  Else, we should
drop the default, and insert a closesocket(fd) before the final return
NULL.

>> >      }
>> >      return NULL;
>> 
>> Not reachable.  Ugly, but not your patch's concern.
>
>
> Regards,
> Daniel

Re: [Qemu-devel] [PATCH v2 1/4] net/socket: Convert the non-blocking connection mechanism to QIOchannel

[Mao Zhongyi]

Hi Markus,

Thanks for your reply.

On 04/27/2017 11:46 PM, Markus Armbruster wrote:
> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>
>> Currently, socket connection in net is realized by an old
>> mechanism which is non-blocking.
>>
>> That old mechanism may cause net blocks on DNS lookups and
>> QEmu has already replaced it with QIOchannel in many features,
>> such as migration.
>>
>> Convert it to QIOchannel for net as well.
>>
>> CC: berrange@redhat.com, pbonzini@redhat.com, jasowang@redhat.com
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> ---
>> The test steps like this:
>>
>>     $ qemu-system-x86_64 -net nic -net socket,listen=:1234 ~/img/test.img
>>     $ qemu-system-x86_64 -net nic -net socket,connect=127.0.0.1:1234 ~/img/test.img
>>
>> No exception.
>>
>>  net/socket.c | 30 +++++++++++++++++++-----------
>>  1 file changed, 19 insertions(+), 11 deletions(-)
>>
>> diff --git a/net/socket.c b/net/socket.c
>> index b8c931e..52f9dce 100644
>> --- a/net/socket.c
>> +++ b/net/socket.c
>> @@ -33,6 +33,7 @@
>>  #include "qemu/sockets.h"
>>  #include "qemu/iov.h"
>>  #include "qemu/main-loop.h"
>> +#include "io/channel-socket.h"
>>
>>  typedef struct NetSocketState {
>>      NetClientState nc;
>> @@ -525,16 +526,22 @@ typedef struct {
>>      char *name;
>>  } socket_connect_data;
>>
>> -static void socket_connect_data_free(socket_connect_data *c)
>> +static void socket_connect_data_free(void *opaque)
>>  {
>> +    socket_connect_data *c = opaque;
>
> Blank line between declarations and statements, please.

OK, I see.

>
>> +    if (!c) {
>> +        return;
>> +    }
>> +
>>      qapi_free_SocketAddress(c->saddr);
>>      g_free(c->model);
>>      g_free(c->name);
>>      g_free(c);
>>  }
>>
>> -static void net_socket_connected(int fd, Error *err, void *opaque)
>> +static void net_socket_connected(QIOTask *task, void *opaque)
>>  {
>> +    QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(qio_task_get_source(task));
>>      socket_connect_data *c = opaque;
>>      NetSocketState *s;
>>      char *addr_str = NULL;
>> @@ -543,13 +550,13 @@ static void net_socket_connected(int fd, Error *err, void *opaque)
>>      addr_str = socket_address_to_string(c->saddr, &local_error);
>>      if (addr_str == NULL) {
>>          error_report_err(local_error);
>> -        closesocket(fd);
>> +        closesocket(sioc->fd);
>>          goto end;
>>      }
>>
>> -    s = net_socket_fd_init(c->peer, c->model, c->name, fd, true);
>> +    s = net_socket_fd_init(c->peer, c->model, c->name, sioc->fd, true);
>>      if (!s) {
>> -        closesocket(fd);
>> +        closesocket(sioc->fd);
>>          goto end;
>>      }
>>
>> @@ -567,7 +574,7 @@ static int net_socket_connect_init(NetClientState *peer,
>>                                     const char *host_str)
>>  {
>>      socket_connect_data *c = g_new0(socket_connect_data, 1);
>> -    int fd = -1;
>> +    QIOChannelSocket *sioc;
>>      Error *local_error = NULL;
>>
>>      c->peer = peer;
>> @@ -578,11 +585,12 @@ static int net_socket_connect_init(NetClientState *peer,
>        c->model = g_strdup(model);
>        c->name = g_strdup(name);
>        c->saddr = socket_parse(host_str, &local_error);
>        if (c->saddr == NULL) {
>>          goto err;
>>      }
>>
>> -    fd = socket_connect(c->saddr, net_socket_connected, c, &local_error);
>> -    if (fd < 0) {
>> -        goto err;
>> -    }
>> -
>> +    sioc = qio_channel_socket_new();
>> +    qio_channel_socket_connect_async(sioc,
>> +                                     c->saddr,
>> +                                     net_socket_connected,
>> +                                     c,
>> +                                     NULL);
>>      return 0;
>>
>>  err:
>        error_report_err(local_error);
>        socket_connect_data_free(c);
>        return -1;
>    }
>
> Ignorant question: how does this change the reporting of errors?
>
> Before the patch, errors from socket_connect() are reported with
> error_report_err(), just like errors from socket_parse().
>
> After the patch?
>
> The next patch converts this function to Error.  Errors from
> socket_parse() are then propagated to the caller.  What about errors
> from socket_connect()?

I'm really careless. I intend to report errors from socket_connect()
in the net_socket_connected(), but it doesn't continue to propagate to
the caller as socket_parse(). Because if not, plenty of QIOChannel
codes should be modified. It seems not a proper way. So I will report
it in the net_socket_connected() whit v3.

Thanks
Mao
>
>
>
>
>

Re: [Qemu-devel] [PATCH v2 1/4] net/socket: Convert the non-blocking connection mechanism to QIOchannel

[Mao Zhongyi]

Hi, Daniel

On 04/28/2017 12:20 AM, Daniel P. Berrange wrote:
> On Wed, Apr 26, 2017 at 04:04:15PM +0800, Mao Zhongyi wrote:
>> Currently, socket connection in net is realized by an old
>> mechanism which is non-blocking.

[...]

             NULL);
>>      return 0;
>
> You've not saved a copy of the QIOChannelSocket poiinter here, and
> the net_socket_connected() method doesn't release the reference
> either. So htere is a memory leak. Of course if you relesae the
> reference in net_socket_connected(), then you need to dup() the
> file descriptor you're borrowing.
>
> Regards,
> Daniel


Sorry for delay, thanks for your detailed reply.
I see, will fix it in the next version.

Thanks
Mao
>

Re: [Qemu-devel] [PATCH v2 2/4] net/socket: Improve -net socket error reporting

[Mao Zhongyi]

Hi, Markus

On 04/28/2017 12:10 AM, Markus Armbruster wrote:
> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>
>> When -net socket fails, it first reports a specific error, then
>> a generic one, like this:
>>
>>     $ qemu-system-x86_64 -net socket,
>>     qemu-system-x86_64: -net socket: exactly one of fd=, listen=, connect=, mcast= or udp= is required
>>     qemu-system-x86_64: -net socket: Device 'socket' could not be initialized
>>
>> Convert net_init_socket() to Error to get rid of the unwanted second
>> error message.
>>
>> CC: berrange@redhat.com, kraxel@redhat.com, pbonzini@redhat.com, jasowang@redhat.com, armbru@redhat.com
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> ---
>>  After the repair, the effect is as follows:
>>
>>     $ qemu-system-x86_64 -net socket,
>>     qemu-system-x86_64: -net socket: exactly one of fd=, listen=, connect=, mcast= or udp= is required
>
> Please move this into the commit message proper.

OK, I see.
Thanks.

>
>>
>>  include/qemu/sockets.h |  2 +-
>>  net/socket.c           | 57 +++++++++++++++++++++++++++++---------------------
>>  util/qemu-sockets.c    |  2 +-
>>  3 files changed, 35 insertions(+), 26 deletions(-)
>>
>> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
>> index af28532..528b802 100644
>> --- a/include/qemu/sockets.h
>> +++ b/include/qemu/sockets.h
>> @@ -118,7 +118,7 @@ SocketAddress *socket_remote_address(int fd, Error **errp);
>>   *
>>   * Returns: the socket address in string format, or NULL on error
>>   */
>> -char *socket_address_to_string(struct SocketAddress *addr, Error **errp);
>> +char *socket_address_to_string(struct SocketAddress *addr);
>>
>>  /**
>>   * socket_address_crumple:
>
> I'd make this a separate patch.  Matter of taste.

OK, will separate from here.
Thanks.

>
>> diff --git a/net/socket.c b/net/socket.c
>> index 52f9dce..b0decbe 100644
>> --- a/net/socket.c
>> +++ b/net/socket.c
>> @@ -486,7 +486,8 @@ static void net_socket_accept(void *opaque)
>>  static int net_socket_listen_init(NetClientState *peer,
>>                                    const char *model,
>>                                    const char *name,
>> -                                  const char *host_str)
>> +                                  const char *host_str,
>> +                                  Error **errp)
>>  {
>>      NetClientState *nc;
>>      NetSocketState *s;
>> @@ -496,14 +497,14 @@ static int net_socket_listen_init(NetClientState *peer,
>>
>>      saddr = socket_parse(host_str, &local_error);
>>      if (saddr == NULL) {
>> -        error_report_err(local_error);
>> +        error_propagate(errp, local_error);
>>          return -1;
>>      }
>
> Simpler:
>
>   -    saddr = socket_parse(host_str, &local_error);
>   +    saddr = socket_parse(host_str, errp);
>        if (saddr == NULL) {
>   -        error_report_err(local_error);
>            return -1;
>        }
>

OK, I see.
Thanks.

>>
>>      ret = socket_listen(saddr, &local_error);
>>      if (ret < 0) {
>>          qapi_free_SocketAddress(saddr);
>> -        error_report_err(local_error);
>> +        error_propagate(errp, local_error);
>>          return -1;
>>      }
>
> Likewise.
>
>>
>> @@ -545,11 +546,9 @@ static void net_socket_connected(QIOTask *task, void *opaque)
>>      socket_connect_data *c = opaque;
>>      NetSocketState *s;
>>      char *addr_str = NULL;
>> -    Error *local_error = NULL;
>>
>> -    addr_str = socket_address_to_string(c->saddr, &local_error);
>> +    addr_str = socket_address_to_string(c->saddr);
>>      if (addr_str == NULL) {
>> -        error_report_err(local_error);
>>          closesocket(sioc->fd);
>>          goto end;
>>      }
>
> On first glance, this looks like you're sweeping an error under the rug.
> Not true, as socket_address_to_string() can't actually fail.  Please
> drop the dead error handling entirely.
>

OK, I see.
Thanks.

>> @@ -571,7 +570,8 @@ end:
>>  static int net_socket_connect_init(NetClientState *peer,
>>                                     const char *model,
>>                                     const char *name,
>> -                                   const char *host_str)
>> +                                   const char *host_str,
>> +                                   Error **errp)
>>  {
>>      socket_connect_data *c = g_new0(socket_connect_data, 1);
>>      QIOChannelSocket *sioc;
>> @@ -594,7 +594,7 @@ static int net_socket_connect_init(NetClientState *peer,
>        Error *local_error = NULL;
>
>        c->peer = peer;
>        c->model = g_strdup(model);
>        c->name = g_strdup(name);
>        c->saddr = socket_parse(host_str, &local_error);
>        if (c->saddr == NULL) {
>            goto err;
>        }
>
>        sioc = qio_channel_socket_new();
>        qio_channel_socket_connect_async(sioc,
>                                         c->saddr,
>                                         net_socket_connected,
>                                         c,
>                                         NULL);
>>      return 0;
>>
>>  err:
>> -    error_report_err(local_error);
>> +    error_propagate(errp, local_error);
>>      socket_connect_data_free(c);
>>      return -1;
>>  }
>
> Since all we do with local_error is propagate it, we can eliminate it:
> pass errp directly to socket_parse(), drop error_propagate().

OK, I see.
Thanks.

>
>> @@ -603,7 +603,8 @@ static int net_socket_mcast_init(NetClientState *peer,
>>                                   const char *model,
>>                                   const char *name,
>>                                   const char *host_str,
>> -                                 const char *localaddr_str)
>> +                                 const char *localaddr_str,
>> +                                 Error **errp)
>>  {
>>      NetSocketState *s;
>>      int fd;
>> @@ -614,8 +615,11 @@ static int net_socket_mcast_init(NetClientState *peer,
>        struct sockaddr_in saddr;
>        struct in_addr localaddr, *param_localaddr;
>
>        if (parse_host_port(&saddr, host_str) < 0)
>>          return -1;
>
> Fails without setting an error.  Fixed in PATCH 4.  Recommend to do
> PATCH 4 before this one.

Thanks, will adjust the order of patch properly.

>
>>
>>      if (localaddr_str != NULL) {
>> -        if (inet_aton(localaddr_str, &localaddr) == 0)
>> +        if (inet_aton(localaddr_str, &localaddr) == 0) {
>> +            error_setg(errp, "Convert the local address to network"
>> +                        " byte order failed");
>
> inet_aton() fails only when its first argument is not a valid IPv4
> address.  Suggest:
>
>     error_setg(errp, "localaddr '%s' is not a valid IPv4 address",
>                localaddr_str);
>
> This error message includes both the exact parameter name "localaddr"
> and the parameter value.

Thanks, will fix it and similar.

>
>>              return -1;
>> +        }
>>          param_localaddr = &localaddr;
>>      } else {
>>          param_localaddr = NULL;
>        }
>
>        fd = net_socket_mcast_create(&saddr, param_localaddr);
>        if (fd < 0)
>            return -1;
>
> Fails without setting an error.  Fixed in PATCH 3.  Recommend to do
> PATCH 3 before this one.

Thanks, will adjust the order of patch properly.


>
>        s = net_socket_fd_init(peer, model, name, fd, 0);
>        if (!s)
>            return -1;
>
> Likewise.
>
>> @@ -642,7 +646,8 @@ static int net_socket_udp_init(NetClientState *peer,
>>                                   const char *model,
>>                                   const char *name,
>>                                   const char *rhost,
>> -                                 const char *lhost)
>> +                                 const char *lhost,
>> +                                 Error **errp)
>>  {
>>      NetSocketState *s;
>>      int fd, ret;
>> @@ -658,7 +663,7 @@ static int net_socket_udp_init(NetClientState *peer,
>        struct sockaddr_in laddr, raddr;
>
>        if (parse_host_port(&laddr, lhost) < 0) {
>            return -1;
>
> Fails without setting an error.  Fixed in PATCH 4.  Recommend to do
> PATCH 4 before this one.

Thanks, will adjust the order of patch properly.

>
>        }
>
>        if (parse_host_port(&raddr, rhost) < 0) {
>            return -1;
>
> Likewise.
>
>        }
>
>>
>>      fd = qemu_socket(PF_INET, SOCK_DGRAM, 0);
>>      if (fd < 0) {
>> -        perror("socket(PF_INET, SOCK_DGRAM)");
>> +        error_setg_errno(errp, errno, "socket(PF_INET, SOCK_DGRAM)");
>
> Bad error message, but no worse than before.
>
>>          return -1;
>>      }
>>
>> @@ -669,7 +674,7 @@ static int net_socket_udp_init(NetClientState *peer,
>        ret = socket_set_fast_reuse(fd);
>        if (ret < 0) {
>            closesocket(fd);
>            return -1;
>
> Fails without setting an error.

Thanks, will add error message.

>
>>      }
>>      ret = bind(fd, (struct sockaddr *)&laddr, sizeof(laddr));
>>      if (ret < 0) {
>> -        perror("bind");
>> +        error_setg_errno(errp, errno, "bind");
>
> Bad error message, but no worse than before.
>
>>          closesocket(fd);
>>          return -1;
>>      }
>> @@ -691,7 +696,6 @@ static int net_socket_udp_init(NetClientState *peer,
>>  int net_init_socket(const Netdev *netdev, const char *name,
>>                      NetClientState *peer, Error **errp)
>>  {
>> -    /* FIXME error_setg(errp, ...) on failure */
>>      Error *err = NULL;
>>      const NetdevSocketOptions *sock;
>>
>> @@ -700,13 +704,13 @@ int net_init_socket(const Netdev *netdev, const char *name,
>>
>>      if (sock->has_fd + sock->has_listen + sock->has_connect + sock->has_mcast +
>>          sock->has_udp != 1) {
>> -        error_report("exactly one of fd=, listen=, connect=, mcast= or udp="
>> +        error_setg(errp, "exactly one of fd=, listen=, connect=, mcast= or udp="
>>                       " is required");
>>          return -1;
>>      }
>>
>>      if (sock->has_localaddr && !sock->has_mcast && !sock->has_udp) {
>> -        error_report("localaddr= is only valid with mcast= or udp=");
>> +        error_setg(errp, "localaddr= is only valid with mcast= or udp=");
>>          return -1;
>>      }
>>
>> @@ -715,7 +719,7 @@ int net_init_socket(const Netdev *netdev, const char *name,
>>
>>          fd = monitor_fd_param(cur_mon, sock->fd, &err);
>>          if (fd == -1) {
>> -            error_report_err(err);
>> +            error_propagate(errp, err);
>>              return -1;
>>          }
>
> Again, pass errp instead of &err, and drop error_propagate().

OK, I see.
Thanks.

>
>>          qemu_set_nonblock(fd);
>            if (!net_socket_fd_init(peer, "socket", name, fd, 1)) {
>                return -1;
>
> Fails without setting an error.  Fixed in PATCH 3.  Recommend to do
> PATCH 3 before this one.

Thanks, will adjust the order of patch properly.

>
>            }
>            return 0;
>        }
>
>> @@ -726,15 +730,18 @@ int net_init_socket(const Netdev *netdev, const char *name,
>>      }
>>
>>      if (sock->has_listen) {
>> -        if (net_socket_listen_init(peer, "socket", name, sock->listen) == -1) {
>> +        if (net_socket_listen_init(peer, "socket", name, sock->listen,
>> +            &err) == -1) {
>> +            error_propagate(errp, err);
>>              return -1;
>>          }
>>          return 0;
>>      }
>>
>>      if (sock->has_connect) {
>> -        if (net_socket_connect_init(peer, "socket", name, sock->connect) ==
>> -            -1) {
>> +        if (net_socket_connect_init(peer, "socket", name, sock->connect,
>> +            &err) == -1) {
>> +            error_propagate(errp, err);
>>              return -1;
>>          }
>>          return 0;
>> @@ -744,7 +751,8 @@ int net_init_socket(const Netdev *netdev, const char *name,
>>          /* if sock->localaddr is missing, it has been initialized to "all bits
>>           * zero" */
>>          if (net_socket_mcast_init(peer, "socket", name, sock->mcast,
>> -            sock->localaddr) == -1) {
>> +            sock->localaddr, &err) == -1) {
>> +            error_propagate(errp, err);
>>              return -1;
>>          }
>>          return 0;
>> @@ -752,11 +760,12 @@ int net_init_socket(const Netdev *netdev, const char *name,
>>
>>      assert(sock->has_udp);
>>      if (!sock->has_localaddr) {
>> -        error_report("localaddr= is mandatory with udp=");
>> +        error_setg(errp, "localaddr= is mandatory with udp=");
>>          return -1;
>>      }
>> -    if (net_socket_udp_init(peer, "socket", name, sock->udp, sock->localaddr) ==
>> -        -1) {
>> +    if (net_socket_udp_init(peer, "socket", name, sock->udp, sock->localaddr,
>> +        &err) == -1) {
>> +        error_propagate(errp, err);
>>          return -1;
>>      }
>>      return 0;
>> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
>> index 8188d9a..0da33d7 100644
>> --- a/util/qemu-sockets.c
>> +++ b/util/qemu-sockets.c
>> @@ -1312,7 +1312,7 @@ SocketAddress *socket_remote_address(int fd, Error **errp)
>>      return socket_sockaddr_to_address(&ss, sslen, errp);
>>  }
>>
>> -char *socket_address_to_string(struct SocketAddress *addr, Error **errp)
>> +char *socket_address_to_string(struct SocketAddress *addr)
>>  {
>>      char *buf;
>>      InetSocketAddress *inet;
>
>
>
>
>

Re: [Qemu-devel] [PATCH v2 3/4] net/socket: Convert error report message to Error

[Mao Zhongyi]

Hi, Markus,Daniel

On 04/28/2017 04:02 PM, Markus Armbruster wrote:
> "Daniel P. Berrange" <berrange@redhat.com> writes:
>
>> On Thu, Apr 27, 2017 at 06:24:17PM +0200, Markus Armbruster wrote:
>>> No review, just an observation.
>>>
>>> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>>>
>>>> Currently, net_socket_mcast_create(), net_socket_fd_init_dgram() and
>>>> net_socket_fd_init() use the function such as fprintf(), perror() to
>>>> report an error message.
>>>>
>>>> Now, convert these functions to Error.
>>>>
>>>> CC: jasowang@redhat.com, armbru@redhat.com
>>>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>>>> ---
>>>>  net/socket.c | 66 +++++++++++++++++++++++++++++++++++++-----------------------
>>>>  1 file changed, 41 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/net/socket.c b/net/socket.c
>>>> index b0decbe..559e09a 100644
>>>> --- a/net/socket.c
>>>> +++ b/net/socket.c
>>> [...]
>>>> @@ -433,25 +437,27 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
>>>>
>>>>  static NetSocketState *net_socket_fd_init(NetClientState *peer,
>>>>                                            const char *model, const char *name,
>>>> -                                          int fd, int is_connected)
>>>> +                                          int fd, int is_connected,
>>>> +                                          Error **errp)
>>>>  {
>>>>      int so_type = -1, optlen=sizeof(so_type);
>>>>
>>>>      if(getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
>>>>          (socklen_t *)&optlen)< 0) {
>>>> -        fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n",
>>>> +        error_setg(errp, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed",
>>>>                  fd);
>>>>          closesocket(fd);
>>>>          return NULL;
>>>>      }
>>>>      switch(so_type) {
>>>>      case SOCK_DGRAM:
>>>> -        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected);
>>>> +        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected, errp);
>>>>      case SOCK_STREAM:
>>>>          return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
>>>>      default:
>>>>          /* who knows ... this could be a eg. a pty, do warn and continue as stream */
>>>> -        fprintf(stderr, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM or SOCK_STREAM\n", so_type, fd);
>>>> +        error_setg(errp, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM"
>>>> +                   " or SOCK_STREAM", so_type, fd);
>>>
>>> Not this patches problem: this case is odd, and the comment is bogus.
>>> If @fd really was a pty, getsockopt() would fail with ENOTSOCK, wouldn't
>>> it?  If @fd is a socket, but neither SOCK_DGRAM nor SOCK_STREAM (say
>>> SOCK_RAW), why is it safe to continue as if it was SOCK_STREAM?  Jason?
>>
>> IMHO it is a problem with this patch. Previously we merely printed
>> a warning & carried on, which is conceptually ok in general, though
>> dubious here for the reason you say.
>>
>> Now we are filling an Error **errp object, and carrying on - this is
>> conceptually broken anywhere. If an Error ** is filled, we must return.
>> If we want to carry on, we shouldn't fill Error **.
>
> You're right.
>
>>>>          return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
>>
>> IMHO, we just kill this and put return NULL here. If there is a genuine
>> reason to support something like SOCK_RAW, it should be explicitly
>> handled, because this default: case will certainly break SOCK_SEQPACKET
>> and SOCK_RDM which can't be treated as streams.
>
> It's either magic or misguided defensive programming.  Probably the
> latter, but I'd like to hear Jason's opinion.
>
> If it's *necessary* magic, we can't use error_setg().  Else, we should
> drop the default, and insert a closesocket(fd) before the final return
> NULL.

As Daniel said, although the previous printed warning message is
dubious, but it is conceptually ok in general. Simply filling it to
Error ** is problematic. Of course, as you said drop the default case,
there will be no problem. But really to do that?

Thanks
Mao

>
>>>>      }
>>>>      return NULL;
>>>
>>> Not reachable.  Ugly, but not your patch's concern.
>>
>>
>> Regards,
>> Daniel
>
>
>
>
>

Re: [Qemu-devel] [PATCH v2 0/4] Convert non-blocking connect and fix its error reporting

[Mao Zhongyi]

On 04/28/2017 12:25 AM, Markus Armbruster wrote:
> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>
>> v2:
>> * PATCH 02 reworking of patch 2 following Markus's suggestion that convert error_report()
>>   in the function called by net_socket_*_init() to Error. Also add many error handling
>>   information.
>> * PATCH 03 net_socket_mcast_create(), net_socket_fd_init_dgram() and net_socket_fd_init()
>>   use the function such as fprintf, perror to report an error message. Convert it to Error.
>> * PATCH 04 parse_host_port() may fail without reporting an error. Now, fix it to set an
>>   error when it fails.
>
> Needs a bit more work.  I'll continue review with v3.

Hi, Markus,

Thanks for your kind reply very much.
I will fix it in the v3 as soon as.

Thanks
Mao
>
>
>
>
>

Re: [Qemu-devel] [PATCH v2 3/4] net/socket: Convert error report message to Error

[Daniel P. Berrange]

On Wed, May 03, 2017 at 03:09:57PM +0800, Mao Zhongyi wrote:
> Hi, Markus,Daniel
> 
> On 04/28/2017 04:02 PM, Markus Armbruster wrote:
> > "Daniel P. Berrange" <berrange@redhat.com> writes:
> > 
> > > On Thu, Apr 27, 2017 at 06:24:17PM +0200, Markus Armbruster wrote:
> > > > No review, just an observation.
> > > > 
> > > > Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
> > > > 
> > > > > Currently, net_socket_mcast_create(), net_socket_fd_init_dgram() and
> > > > > net_socket_fd_init() use the function such as fprintf(), perror() to
> > > > > report an error message.
> > > > > 
> > > > > Now, convert these functions to Error.
> > > > > 
> > > > > CC: jasowang@redhat.com, armbru@redhat.com
> > > > > Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> > > > > ---
> > > > >  net/socket.c | 66 +++++++++++++++++++++++++++++++++++++-----------------------
> > > > >  1 file changed, 41 insertions(+), 25 deletions(-)
> > > > > 
> > > > > diff --git a/net/socket.c b/net/socket.c
> > > > > index b0decbe..559e09a 100644
> > > > > --- a/net/socket.c
> > > > > +++ b/net/socket.c
> > > > [...]
> > > > > @@ -433,25 +437,27 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
> > > > > 
> > > > >  static NetSocketState *net_socket_fd_init(NetClientState *peer,
> > > > >                                            const char *model, const char *name,
> > > > > -                                          int fd, int is_connected)
> > > > > +                                          int fd, int is_connected,
> > > > > +                                          Error **errp)
> > > > >  {
> > > > >      int so_type = -1, optlen=sizeof(so_type);
> > > > > 
> > > > >      if(getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
> > > > >          (socklen_t *)&optlen)< 0) {
> > > > > -        fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n",
> > > > > +        error_setg(errp, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed",
> > > > >                  fd);
> > > > >          closesocket(fd);
> > > > >          return NULL;
> > > > >      }
> > > > >      switch(so_type) {
> > > > >      case SOCK_DGRAM:
> > > > > -        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected);
> > > > > +        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected, errp);
> > > > >      case SOCK_STREAM:
> > > > >          return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
> > > > >      default:
> > > > >          /* who knows ... this could be a eg. a pty, do warn and continue as stream */
> > > > > -        fprintf(stderr, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM or SOCK_STREAM\n", so_type, fd);
> > > > > +        error_setg(errp, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM"
> > > > > +                   " or SOCK_STREAM", so_type, fd);
> > > > 
> > > > Not this patches problem: this case is odd, and the comment is bogus.
> > > > If @fd really was a pty, getsockopt() would fail with ENOTSOCK, wouldn't
> > > > it?  If @fd is a socket, but neither SOCK_DGRAM nor SOCK_STREAM (say
> > > > SOCK_RAW), why is it safe to continue as if it was SOCK_STREAM?  Jason?
> > > 
> > > IMHO it is a problem with this patch. Previously we merely printed
> > > a warning & carried on, which is conceptually ok in general, though
> > > dubious here for the reason you say.
> > > 
> > > Now we are filling an Error **errp object, and carrying on - this is
> > > conceptually broken anywhere. If an Error ** is filled, we must return.
> > > If we want to carry on, we shouldn't fill Error **.
> > 
> > You're right.
> > 
> > > > >          return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
> > > 
> > > IMHO, we just kill this and put return NULL here. If there is a genuine
> > > reason to support something like SOCK_RAW, it should be explicitly
> > > handled, because this default: case will certainly break SOCK_SEQPACKET
> > > and SOCK_RDM which can't be treated as streams.
> > 
> > It's either magic or misguided defensive programming.  Probably the
> > latter, but I'd like to hear Jason's opinion.
> > 
> > If it's *necessary* magic, we can't use error_setg().  Else, we should
> > drop the default, and insert a closesocket(fd) before the final return
> > NULL.
> 
> As Daniel said, although the previous printed warning message is
> dubious, but it is conceptually ok in general. Simply filling it to
> Error ** is problematic. Of course, as you said drop the default case,
> there will be no problem. But really to do that?

Yes, please drop that 'default' case since it is broken already.

BTW, drop the default case in a separate patch at the start of
your series, before changing the error code, so the functional
change is clear in git history.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 3/4] net/socket: Convert error report message to Error

[Mao Zhongyi]

On 05/03/2017 04:37 PM, Daniel P. Berrange wrote:
> On Wed, May 03, 2017 at 03:09:57PM +0800, Mao Zhongyi wrote:
>> Hi, Markus,Daniel
>>
>> On 04/28/2017 04:02 PM, Markus Armbruster wrote:
>>> "Daniel P. Berrange" <berrange@redhat.com> writes:
>>>
>>>> On Thu, Apr 27, 2017 at 06:24:17PM +0200, Markus Armbruster wrote:
>>>>> No review, just an observation.
>>>>>
>>>>> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>>>>>
>>>>>> Currently, net_socket_mcast_create(), net_socket_fd_init_dgram() and
>>>>>> net_socket_fd_init() use the function such as fprintf(), perror() to
>>>>>> report an error message.
>>>>>>
>>>>>> Now, convert these functions to Error.
>>>>>>
>>>>>> CC: jasowang@redhat.com, armbru@redhat.com
>>>>>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>>>>>> ---
>>>>>>  net/socket.c | 66 +++++++++++++++++++++++++++++++++++++-----------------------
>>>>>>  1 file changed, 41 insertions(+), 25 deletions(-)
>>>>>>
>>>>>> diff --git a/net/socket.c b/net/socket.c
>>>>>> index b0decbe..559e09a 100644
>>>>>> --- a/net/socket.c
>>>>>> +++ b/net/socket.c
>>>>> [...]
>>>>>> @@ -433,25 +437,27 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
>>>>>>
>>>>>>  static NetSocketState *net_socket_fd_init(NetClientState *peer,
>>>>>>                                            const char *model, const char *name,
>>>>>> -                                          int fd, int is_connected)
>>>>>> +                                          int fd, int is_connected,
>>>>>> +                                          Error **errp)
>>>>>>  {
>>>>>>      int so_type = -1, optlen=sizeof(so_type);
>>>>>>
>>>>>>      if(getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
>>>>>>          (socklen_t *)&optlen)< 0) {
>>>>>> -        fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n",
>>>>>> +        error_setg(errp, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed",
>>>>>>                  fd);
>>>>>>          closesocket(fd);
>>>>>>          return NULL;
>>>>>>      }
>>>>>>      switch(so_type) {
>>>>>>      case SOCK_DGRAM:
>>>>>> -        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected);
>>>>>> +        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected, errp);
>>>>>>      case SOCK_STREAM:
>>>>>>          return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
>>>>>>      default:
>>>>>>          /* who knows ... this could be a eg. a pty, do warn and continue as stream */
>>>>>> -        fprintf(stderr, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM or SOCK_STREAM\n", so_type, fd);
>>>>>> +        error_setg(errp, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM"
>>>>>> +                   " or SOCK_STREAM", so_type, fd);
>>>>>
>>>>> Not this patches problem: this case is odd, and the comment is bogus.
>>>>> If @fd really was a pty, getsockopt() would fail with ENOTSOCK, wouldn't
>>>>> it?  If @fd is a socket, but neither SOCK_DGRAM nor SOCK_STREAM (say
>>>>> SOCK_RAW), why is it safe to continue as if it was SOCK_STREAM?  Jason?
>>>>
>>>> IMHO it is a problem with this patch. Previously we merely printed
>>>> a warning & carried on, which is conceptually ok in general, though
>>>> dubious here for the reason you say.
>>>>
>>>> Now we are filling an Error **errp object, and carrying on - this is
>>>> conceptually broken anywhere. If an Error ** is filled, we must return.
>>>> If we want to carry on, we shouldn't fill Error **.
>>>
>>> You're right.
>>>
>>>>>>          return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
>>>>
>>>> IMHO, we just kill this and put return NULL here. If there is a genuine
>>>> reason to support something like SOCK_RAW, it should be explicitly
>>>> handled, because this default: case will certainly break SOCK_SEQPACKET
>>>> and SOCK_RDM which can't be treated as streams.
>>>
>>> It's either magic or misguided defensive programming.  Probably the
>>> latter, but I'd like to hear Jason's opinion.
>>>
>>> If it's *necessary* magic, we can't use error_setg().  Else, we should
>>> drop the default, and insert a closesocket(fd) before the final return
>>> NULL.
>>
>> As Daniel said, although the previous printed warning message is
>> dubious, but it is conceptually ok in general. Simply filling it to
>> Error ** is problematic. Of course, as you said drop the default case,
>> there will be no problem. But really to do that?
>
> Yes, please drop that 'default' case since it is broken already.
>
> BTW, drop the default case in a separate patch at the start of
> your series, before changing the error code, so the functional
> change is clear in git history.
>
>
> Regards,
> Daniel
>

OK，I see.
Thank you very much.

Mao

Re: [Qemu-devel] [PATCH v2 3/4] net/socket: Convert error report message to Error

[Markus Armbruster]

Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:

> Hi, Markus,Daniel
>
> On 04/28/2017 04:02 PM, Markus Armbruster wrote:
>> "Daniel P. Berrange" <berrange@redhat.com> writes:
>>
>>> On Thu, Apr 27, 2017 at 06:24:17PM +0200, Markus Armbruster wrote:
>>>> No review, just an observation.
>>>>
>>>> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>>>>
>>>>> Currently, net_socket_mcast_create(), net_socket_fd_init_dgram() and
>>>>> net_socket_fd_init() use the function such as fprintf(), perror() to
>>>>> report an error message.
>>>>>
>>>>> Now, convert these functions to Error.
>>>>>
>>>>> CC: jasowang@redhat.com, armbru@redhat.com
>>>>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>>>>> ---
>>>>>  net/socket.c | 66 +++++++++++++++++++++++++++++++++++++-----------------------
>>>>>  1 file changed, 41 insertions(+), 25 deletions(-)
>>>>>
>>>>> diff --git a/net/socket.c b/net/socket.c
>>>>> index b0decbe..559e09a 100644
>>>>> --- a/net/socket.c
>>>>> +++ b/net/socket.c
>>>> [...]
>>>>> @@ -433,25 +437,27 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
>>>>>
>>>>>  static NetSocketState *net_socket_fd_init(NetClientState *peer,
>>>>>                                            const char *model, const char *name,
>>>>> -                                          int fd, int is_connected)
>>>>> +                                          int fd, int is_connected,
>>>>> +                                          Error **errp)
>>>>>  {
>>>>>      int so_type = -1, optlen=sizeof(so_type);
>>>>>
>>>>>      if(getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
>>>>>          (socklen_t *)&optlen)< 0) {
>>>>> -        fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n",
>>>>> +        error_setg(errp, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed",
>>>>>                  fd);
>>>>>          closesocket(fd);
>>>>>          return NULL;
>>>>>      }
>>>>>      switch(so_type) {
>>>>>      case SOCK_DGRAM:
>>>>> -        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected);
>>>>> +        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected, errp);
>>>>>      case SOCK_STREAM:
>>>>>          return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
>>>>>      default:
>>>>>          /* who knows ... this could be a eg. a pty, do warn and continue as stream */
>>>>> -        fprintf(stderr, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM or SOCK_STREAM\n", so_type, fd);
>>>>> +        error_setg(errp, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM"
>>>>> +                   " or SOCK_STREAM", so_type, fd);
>>>>
>>>> Not this patches problem: this case is odd, and the comment is bogus.
>>>> If @fd really was a pty, getsockopt() would fail with ENOTSOCK, wouldn't
>>>> it?  If @fd is a socket, but neither SOCK_DGRAM nor SOCK_STREAM (say
>>>> SOCK_RAW), why is it safe to continue as if it was SOCK_STREAM?  Jason?
>>>
>>> IMHO it is a problem with this patch. Previously we merely printed
>>> a warning & carried on, which is conceptually ok in general, though
>>> dubious here for the reason you say.
>>>
>>> Now we are filling an Error **errp object, and carrying on - this is
>>> conceptually broken anywhere. If an Error ** is filled, we must return.
>>> If we want to carry on, we shouldn't fill Error **.
>>
>> You're right.
>>
>>>>>          return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
>>>
>>> IMHO, we just kill this and put return NULL here. If there is a genuine
>>> reason to support something like SOCK_RAW, it should be explicitly
>>> handled, because this default: case will certainly break SOCK_SEQPACKET
>>> and SOCK_RDM which can't be treated as streams.
>>
>> It's either magic or misguided defensive programming.  Probably the
>> latter, but I'd like to hear Jason's opinion.
>>
>> If it's *necessary* magic, we can't use error_setg().  Else, we should
>> drop the default, and insert a closesocket(fd) before the final return
>> NULL.
>
> As Daniel said, although the previous printed warning message is
> dubious, but it is conceptually ok in general. Simply filling it to
> Error ** is problematic. Of course, as you said drop the default case,
> there will be no problem. But really to do that?

Since Jason hasn't spoken up, I suggest you follow Dan's recommendation
and insert a patch to drop the default case before your error rework.

Re: [Qemu-devel] [PATCH v2 3/4] net/socket: Convert error report message to Error

[Mao Zhongyi]

On 05/03/2017 04:54 PM, Markus Armbruster wrote:
> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>
>> Hi, Markus,Daniel
>>
>> On 04/28/2017 04:02 PM, Markus Armbruster wrote:
>>> "Daniel P. Berrange" <berrange@redhat.com> writes:
>>>
>>>> On Thu, Apr 27, 2017 at 06:24:17PM +0200, Markus Armbruster wrote:
>>>>> No review, just an observation.
>>>>>
>>>>> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>>>>>
>>>>>> Currently, net_socket_mcast_create(), net_socket_fd_init_dgram() and
>>>>>> net_socket_fd_init() use the function such as fprintf(), perror() to
>>>>>> report an error message.
>>>>>>
>>>>>> Now, convert these functions to Error.
>>>>>>
>>>>>> CC: jasowang@redhat.com, armbru@redhat.com
>>>>>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>>>>>> ---
>>>>>>  net/socket.c | 66 +++++++++++++++++++++++++++++++++++++-----------------------
>>>>>>  1 file changed, 41 insertions(+), 25 deletions(-)
>>>>>>
>>>>>> diff --git a/net/socket.c b/net/socket.c
>>>>>> index b0decbe..559e09a 100644
>>>>>> --- a/net/socket.c
>>>>>> +++ b/net/socket.c
>>>>> [...]
>>>>>> @@ -433,25 +437,27 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
>>>>>>
>>>>>>  static NetSocketState *net_socket_fd_init(NetClientState *peer,
>>>>>>                                            const char *model, const char *name,
>>>>>> -                                          int fd, int is_connected)
>>>>>> +                                          int fd, int is_connected,
>>>>>> +                                          Error **errp)
>>>>>>  {
>>>>>>      int so_type = -1, optlen=sizeof(so_type);
>>>>>>
>>>>>>      if(getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
>>>>>>          (socklen_t *)&optlen)< 0) {
>>>>>> -        fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n",
>>>>>> +        error_setg(errp, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed",
>>>>>>                  fd);
>>>>>>          closesocket(fd);
>>>>>>          return NULL;
>>>>>>      }
>>>>>>      switch(so_type) {
>>>>>>      case SOCK_DGRAM:
>>>>>> -        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected);
>>>>>> +        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected, errp);
>>>>>>      case SOCK_STREAM:
>>>>>>          return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
>>>>>>      default:
>>>>>>          /* who knows ... this could be a eg. a pty, do warn and continue as stream */
>>>>>> -        fprintf(stderr, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM or SOCK_STREAM\n", so_type, fd);
>>>>>> +        error_setg(errp, "qemu: warning: socket type=%d for fd=%d is not SOCK_DGRAM"
>>>>>> +                   " or SOCK_STREAM", so_type, fd);
>>>>>
>>>>> Not this patches problem: this case is odd, and the comment is bogus.
>>>>> If @fd really was a pty, getsockopt() would fail with ENOTSOCK, wouldn't
>>>>> it?  If @fd is a socket, but neither SOCK_DGRAM nor SOCK_STREAM (say
>>>>> SOCK_RAW), why is it safe to continue as if it was SOCK_STREAM?  Jason?
>>>>
>>>> IMHO it is a problem with this patch. Previously we merely printed
>>>> a warning & carried on, which is conceptually ok in general, though
>>>> dubious here for the reason you say.
>>>>
>>>> Now we are filling an Error **errp object, and carrying on - this is
>>>> conceptually broken anywhere. If an Error ** is filled, we must return.
>>>> If we want to carry on, we shouldn't fill Error **.
>>>
>>> You're right.
>>>
>>>>>>          return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
>>>>
>>>> IMHO, we just kill this and put return NULL here. If there is a genuine
>>>> reason to support something like SOCK_RAW, it should be explicitly
>>>> handled, because this default: case will certainly break SOCK_SEQPACKET
>>>> and SOCK_RDM which can't be treated as streams.
>>>
>>> It's either magic or misguided defensive programming.  Probably the
>>> latter, but I'd like to hear Jason's opinion.
>>>
>>> If it's *necessary* magic, we can't use error_setg().  Else, we should
>>> drop the default, and insert a closesocket(fd) before the final return
>>> NULL.
>>
>> As Daniel said, although the previous printed warning message is
>> dubious, but it is conceptually ok in general. Simply filling it to
>> Error ** is problematic. Of course, as you said drop the default case,
>> there will be no problem. But really to do that?
>
> Since Jason hasn't spoken up, I suggest you follow Dan's recommendation
> and insert a patch to drop the default case before your error rework.
>
>
>

I see, will drop the 'default' case in a separated patch.

Thanks.
Mao

Re: [Qemu-devel] [PATCH v2 3/4] net/socket: Convert error report message to Error

[Jason Wang]

On 2017年05月03日 16:59, Mao Zhongyi wrote:
>
>
> On 05/03/2017 04:54 PM, Markus Armbruster wrote:
>> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>>
>>> Hi, Markus,Daniel
>>>
>>> On 04/28/2017 04:02 PM, Markus Armbruster wrote:
>>>> "Daniel P. Berrange" <berrange@redhat.com> writes:
>>>>
>>>>> On Thu, Apr 27, 2017 at 06:24:17PM +0200, Markus Armbruster wrote:
>>>>>> No review, just an observation.
>>>>>>
>>>>>> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>>>>>>
>>>>>>> Currently, net_socket_mcast_create(), net_socket_fd_init_dgram() 
>>>>>>> and
>>>>>>> net_socket_fd_init() use the function such as fprintf(), 
>>>>>>> perror() to
>>>>>>> report an error message.
>>>>>>>
>>>>>>> Now, convert these functions to Error.
>>>>>>>
>>>>>>> CC: jasowang@redhat.com, armbru@redhat.com
>>>>>>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>>>>>>> ---
>>>>>>>  net/socket.c | 66 
>>>>>>> +++++++++++++++++++++++++++++++++++++-----------------------
>>>>>>>  1 file changed, 41 insertions(+), 25 deletions(-)
>>>>>>>
>>>>>>> diff --git a/net/socket.c b/net/socket.c
>>>>>>> index b0decbe..559e09a 100644
>>>>>>> --- a/net/socket.c
>>>>>>> +++ b/net/socket.c
>>>>>> [...]
>>>>>>> @@ -433,25 +437,27 @@ static NetSocketState 
>>>>>>> *net_socket_fd_init_stream(NetClientState *peer,
>>>>>>>
>>>>>>>  static NetSocketState *net_socket_fd_init(NetClientState *peer,
>>>>>>>                                            const char *model, 
>>>>>>> const char *name,
>>>>>>> -                                          int fd, int 
>>>>>>> is_connected)
>>>>>>> +                                          int fd, int 
>>>>>>> is_connected,
>>>>>>> +                                          Error **errp)
>>>>>>>  {
>>>>>>>      int so_type = -1, optlen=sizeof(so_type);
>>>>>>>
>>>>>>>      if(getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
>>>>>>>          (socklen_t *)&optlen)< 0) {
>>>>>>> -        fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for 
>>>>>>> fd=%d failed\n",
>>>>>>> +        error_setg(errp, "qemu: error: getsockopt(SO_TYPE) for 
>>>>>>> fd=%d failed",
>>>>>>>                  fd);
>>>>>>>          closesocket(fd);
>>>>>>>          return NULL;
>>>>>>>      }
>>>>>>>      switch(so_type) {
>>>>>>>      case SOCK_DGRAM:
>>>>>>> -        return net_socket_fd_init_dgram(peer, model, name, fd, 
>>>>>>> is_connected);
>>>>>>> +        return net_socket_fd_init_dgram(peer, model, name, fd, 
>>>>>>> is_connected, errp);
>>>>>>>      case SOCK_STREAM:
>>>>>>>          return net_socket_fd_init_stream(peer, model, name, fd, 
>>>>>>> is_connected);
>>>>>>>      default:
>>>>>>>          /* who knows ... this could be a eg. a pty, do warn and 
>>>>>>> continue as stream */
>>>>>>> -        fprintf(stderr, "qemu: warning: socket type=%d for 
>>>>>>> fd=%d is not SOCK_DGRAM or SOCK_STREAM\n", so_type, fd);
>>>>>>> +        error_setg(errp, "qemu: warning: socket type=%d for 
>>>>>>> fd=%d is not SOCK_DGRAM"
>>>>>>> +                   " or SOCK_STREAM", so_type, fd);
>>>>>>
>>>>>> Not this patches problem: this case is odd, and the comment is 
>>>>>> bogus.
>>>>>> If @fd really was a pty, getsockopt() would fail with ENOTSOCK, 
>>>>>> wouldn't
>>>>>> it?  If @fd is a socket, but neither SOCK_DGRAM nor SOCK_STREAM (say
>>>>>> SOCK_RAW), why is it safe to continue as if it was SOCK_STREAM?  
>>>>>> Jason?
>>>>>
>>>>> IMHO it is a problem with this patch. Previously we merely printed
>>>>> a warning & carried on, which is conceptually ok in general, though
>>>>> dubious here for the reason you say.
>>>>>
>>>>> Now we are filling an Error **errp object, and carrying on - this is
>>>>> conceptually broken anywhere. If an Error ** is filled, we must 
>>>>> return.
>>>>> If we want to carry on, we shouldn't fill Error **.
>>>>
>>>> You're right.
>>>>
>>>>>>>          return net_socket_fd_init_stream(peer, model, name, fd, 
>>>>>>> is_connected);
>>>>>
>>>>> IMHO, we just kill this and put return NULL here. If there is a 
>>>>> genuine
>>>>> reason to support something like SOCK_RAW, it should be explicitly
>>>>> handled, because this default: case will certainly break 
>>>>> SOCK_SEQPACKET
>>>>> and SOCK_RDM which can't be treated as streams.
>>>>
>>>> It's either magic or misguided defensive programming. Probably the
>>>> latter, but I'd like to hear Jason's opinion.
>>>>
>>>> If it's *necessary* magic, we can't use error_setg().  Else, we should
>>>> drop the default, and insert a closesocket(fd) before the final return
>>>> NULL.
>>>
>>> As Daniel said, although the previous printed warning message is
>>> dubious, but it is conceptually ok in general. Simply filling it to
>>> Error ** is problematic. Of course, as you said drop the default case,
>>> there will be no problem. But really to do that?
>>
>> Since Jason hasn't spoken up, I suggest you follow Dan's recommendation
>> and insert a patch to drop the default case before your error rework.
>>
>>
>>
>
> I see, will drop the 'default' case in a separated patch.
>
> Thanks.

Sorry for the late. I agree to drop the default. Even SOCK_RAW could not 
work as stream and any type should be supported explicitly.

Thanks

Re: [Qemu-devel] [PATCH v2 0/4] Convert non-blocking connect and fix its error reporting

[Daniel P. Berrange]

On Wed, Apr 26, 2017 at 04:04:14PM +0800, Mao Zhongyi wrote:
> v2:
> * PATCH 02 reworking of patch 2 following Markus's suggestion that convert error_report()
>   in the function called by net_socket_*_init() to Error. Also add many error handling
>   information.
> * PATCH 03 net_socket_mcast_create(), net_socket_fd_init_dgram() and net_socket_fd_init() 
>   use the function such as fprintf, perror to report an error message. Convert it to Error.
> * PATCH 04 parse_host_port() may fail without reporting an error. Now, fix it to set an
>   error when it fails.

FYI, I discovered that previous change

  commit 883e4f7624e10b98d16d9adaffb8b1795664d899
  Author: Marc-André Lureau <marcandre.lureau@redhat.com>
  Date:   Sat Jun 18 13:24:02 2016 +0530

    Change net/socket.c to use socket_*() functions

has seriously broken the current code because net_socket_fd_init()
was not called from the right place. Fixing the current code is
somewhat painful, so I've sent a revert of that broken patch.

To demo the problem first run:

  $ ./x86_64-softmmu/qemu-system-x86_64  \
      -device e1000,id=e0,netdev=user.0,mac=DE:AD:BE:EF:AF:04 \
      -netdev socket,id=user.0,listen=:1234 

and then run:

   $ ./x86_64-softmmu/qemu-system-x86_64 \
           -device e1000,id=e0,netdev=hn0,mac=DE:AD:BE:EF:AF:05 \
           -netdev socket,id=hn0,connect=localhost:1234

currently the second command fails with

  qemu-system-x86_64: -device e1000,id=e0,netdev=hn0,mac=DE:AD:BE:EF:AF:05: Property 'e1000.netdev' can't find value 'hn0'

and my revert fixes that. Just something for you to test with your
new patch series...

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v2 0/4] Convert non-blocking connect and fix its error reporting

[Mao Zhongyi]

On 05/06/2017 12:39 AM, Daniel P. Berrange wrote:
> On Wed, Apr 26, 2017 at 04:04:14PM +0800, Mao Zhongyi wrote:
>> v2:
>> * PATCH 02 reworking of patch 2 following Markus's suggestion that convert error_report()
>>   in the function called by net_socket_*_init() to Error. Also add many error handling
>>   information.
>> * PATCH 03 net_socket_mcast_create(), net_socket_fd_init_dgram() and net_socket_fd_init()
>>   use the function such as fprintf, perror to report an error message. Convert it to Error.
>> * PATCH 04 parse_host_port() may fail without reporting an error. Now, fix it to set an
>>   error when it fails.
>
> FYI, I discovered that previous change
>
>   commit 883e4f7624e10b98d16d9adaffb8b1795664d899
>   Author: Marc-André Lureau <marcandre.lureau@redhat.com>
>   Date:   Sat Jun 18 13:24:02 2016 +0530
>
>     Change net/socket.c to use socket_*() functions
>
> has seriously broken the current code because net_socket_fd_init()
> was not called from the right place. Fixing the current code is
> somewhat painful, so I've sent a revert of that broken patch.
>
> To demo the problem first run:
>
>   $ ./x86_64-softmmu/qemu-system-x86_64  \
>       -device e1000,id=e0,netdev=user.0,mac=DE:AD:BE:EF:AF:04 \
>       -netdev socket,id=user.0,listen=:1234
>
> and then run:
>
>    $ ./x86_64-softmmu/qemu-system-x86_64 \
>            -device e1000,id=e0,netdev=hn0,mac=DE:AD:BE:EF:AF:05 \
>            -netdev socket,id=hn0,connect=localhost:1234
>
> currently the second command fails with
>
>   qemu-system-x86_64: -device e1000,id=e0,netdev=hn0,mac=DE:AD:BE:EF:AF:05: Property 'e1000.netdev' can't find value 'hn0'
>
> and my revert fixes that. Just something for you to test with your
> new patch series...
>
> Regards,
> Daniel

Hi, Daniel

According to your latest advice, run these two commands in my new
patch, the second command reported the same error.

Now, you sent a revert of that broken patch to avoid this problem.
It also means that my first patch based on socket_*() is disabled.
Currently, there is no good way to convert it use QIOchannel. So I
have an idea to use the reverted patch instead of the first of my
series, and then based on the first to fix the rest. Latter I will
convert it to QIOchannel in a separated patch. Do you think that's
ok?

Looking forward to your opinion.

Thanks
Mao