[Qemu-devel] [PATCH 1/2] net/socket: convert non-blocking connect to use QIOChannel

[Qemu-devel] [PATCH 1/2] net/socket: convert non-blocking connect to use QIOChannel

[Mao Zhongyi]

The non-blocking connect mechanism doesn't work well in
net connection, it still blocks on DNS lookups. So it is
obsolete. Supersede it with QIOchannel.

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 | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index fe3547b..1886f98 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,21 @@ 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 +549,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 +573,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,10 +584,12 @@ static int net_socket_connect_init(NetClientState *peer,
         goto err;
     }
 
-    fd = socket_connect(c->saddr, &local_error, net_socket_connected, c);
-    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;
 
-- 
2.9.3

[Qemu-devel] [PATCH 2/2] 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.

Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
 net/socket.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 1886f98..85ad9cd 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -691,7 +691,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 +699,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;
     }
 
@@ -752,7 +751,7 @@ 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) ==
-- 
2.9.3

Re: [Qemu-devel] [PATCH 2/2] 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.
>
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>

Thanks for trying to tackle this.

> ---
>  net/socket.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/net/socket.c b/net/socket.c
> index 1886f98..85ad9cd 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -691,7 +691,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 +699,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;
>      }
>  
       if (sock->has_fd) {
           int fd;

           fd = monitor_fd_param(cur_mon, sock->fd, &err);
           if (fd == -1) {
               error_report_err(err);

You missed this one.

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

       if (sock->has_listen) {
           if (net_socket_listen_init(peer, "socket", name, sock->listen) == -1) {

net_socket_listen_init() calls error_report_err().  You need to convert
it to Error.

               return -1;
           }
           return 0;
       }

       if (sock->has_connect) {
           if (net_socket_connect_init(peer, "socket", name, sock->connect) ==
               -1) {

Likewise.

You also need to convert net_socket_connected().

You should review everything called by this function for use of
error_report() or similar.

               return -1;
           }
           return 0;
       }

       if (sock->has_mcast) {
           /* 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) {

Different kind of problem here: this function can fail without reporting
an error.  You need to fix it to set an error when it fails.

               return -1;
           }
           return 0;
       }
> @@ -752,7 +751,7 @@ 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) ==