Improve error reporting

PATCH Superseded Mao Zhongyi 2 participants 10 messages Jun 12, 2017 – Jun 26, 2017

Subsystems (2)

Networking Sockets

lore

Share & tools

lore

Mao Zhongyi

author

2017-06-12 14:16 UTC #0 lore raw

v4 cover Improve error reporting

Daniel's patch(commit 6701e551, Revert "Change net/socket.c to use socket_*() functions" again) has been in upstream. Continue this patchset.

In v2, this series convert the non-blocking connect mechanism to QIOchannel by replace the socket_connect(), and some errors also are converted to Error.

The v2 related discussion was listed on:
https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg04886.html

v4:
* PATCH 01 is redoing previous patch 1, replace the fprintf() with error_report() in the 'default' case of net_socket_fd_init() [Markus Armbruster]

v3:
* PATCH 01 is suggested by Markus and Daniel that removes the dubious 'default' case in the net_socket_fd_init(). Jason agreed.
* PATCH 02 is redoing previous patch 4.
* PATCH 04 is redoing previous patch 2, improves sort of error messages.

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 [email protected]

Mao Zhongyi (4):
net/socket: Drop the odd 'default' case and comment net/net: Convert parse_host_port() to Error net/socket: Convert error report message to Error net/socket: Improve -net socket error reporting

 include/qemu/sockets.h |   2 +-
 net/net.c              |  21 ++++++--
 net/socket.c           | 133 +++++++++++++++++++++++++++++--------------------
 3 files changed, 96 insertions(+), 60 deletions(-)

Signature · 1 line

2.9.3

Mao Zhongyi

author

2017-06-12 14:16 UTC #1 lore raw

PATCH v4 1/4 net/socket: Drop the odd 'default' case and comment

In the net_socket_fd_init(), the 'default' case and comment is odd. If @fd really was a pty, getsockopt() would fail with ENOTSOCK. If @fd was a socket, but neither SOCK_DGRAM nor SOCK_STREAM. It should not be treated as if it was SOCK_STREAM.

If there is a genuine reason to support something like SOCK_RAW, it should be explicitly handled.

So, drop the 'default' case since it is broken already.

Cc [email protected]

Signed-off-by Mao Zhongyi <[email protected]>

---
 net/socket.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

net/socket.c +3 -3

index dcae1ae..53765bd 100644
@@ -449,9 +449,9 @@ static NetSocketState *net_socket_fd_init(NetClientState *peer,
     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);
-        return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
+        error_report("qemu: error: socket type=%d for fd=%d is not"
+                " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
+        closesocket(fd);
     }
     return NULL;
 }

Signature · 1 line

2.9.3

Mao Zhongyi

author

2017-06-12 14:16 UTC #9 lore raw

PATCH v4 4/4 net/socket: Improve -net socket error reporting

In reply to 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 superfluous second error message. After the patch, the effect 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

At the same time, add many explicit error handling message when it fails.

Cc [email protected]

Signed-off-by Mao Zhongyi <[email protected]>

---
 net/socket.c | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

net/socket.c +22 ~~-15~~

index 9825422..a8f2d7f 100644
@@ -510,7 +510,8 @@ static int net_socket_listen_init(NetClientState *peer,
 
     fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {
-        perror("socket");
+        error_setg_errno(errp, errno, "Create socket(PF_INET,"
+                         " SOCK_STREAM) failed");
         return -1;
     }
     qemu_set_nonblock(fd);

@@ -519,13 +520,14 @@ static int net_socket_listen_init(NetClientState *peer,
 
     ret = bind(fd, (struct sockaddr *)&saddr, sizeof(saddr));
     if (ret < 0) {
-        perror("bind");
+        error_setg_errno(errp, errno, "Bind ip=%s to fd=%d failed",
+                         inet_ntoa(saddr.sin_addr), fd);
         closesocket(fd);
         return -1;
     }
     ret = listen(fd, 0);
     if (ret < 0) {
-        perror("listen");
+        error_setg_errno(errp, errno, "Listen socket fd=%d failed", fd);
         closesocket(fd);
         return -1;
     }

@@ -557,7 +559,8 @@ static int net_socket_connect_init(NetClientState *peer,
 
     fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {
-        perror("socket");
+        error_setg_errno(errp, errno, "Create socket(PF_INET,"
+                         " SOCK_STREAM) failed");
         return -1;
     }
     qemu_set_nonblock(fd);

@@ -573,7 +576,7 @@ static int net_socket_connect_init(NetClientState *peer,
                        errno == EINVAL) {
                 break;
             } else {
-                perror("connect");
+                error_setg_errno(errp, errno, "Connection failed");
                 closesocket(fd);
                 return -1;
             }

@@ -607,8 +610,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, "localaddr '%s' is not a valid IPv4 address",
+                       localaddr_str);
             return -1;
+        }
         param_localaddr = &localaddr;
     } else {
         param_localaddr = NULL;

@@ -652,18 +658,22 @@ 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, "Create socket(PF_INET,"
+                         " SOCK_DGRAM) failed");
         return -1;
     }
 
     ret = socket_set_fast_reuse(fd);
     if (ret < 0) {
+        error_setg_errno(errp, errno, "Set the socket fd=%d attribute"
+                   " SO_REUSEADDR failed", fd);
         closesocket(fd);
         return -1;
     }
     ret = bind(fd, (struct sockaddr *)&laddr, sizeof(laddr));
     if (ret < 0) {
-        perror("bind");
+        error_setg_errno(errp, errno, "Bind ip=%s to fd=%d failed",
+                         inet_ntoa(laddr.sin_addr), fd);
         closesocket(fd);
         return -1;
     }

@@ -685,8 +695,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;
 
     assert(netdev->type == NET_CLIENT_DRIVER_SOCKET);

@@ -694,22 +702,21 @@ 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);
+        fd = monitor_fd_param(cur_mon, sock->fd, errp);
         if (fd == -1) {
-            error_report_err(err);
             return -1;
         }
         qemu_set_nonblock(fd);

@@ -747,7 +754,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,

Signature · 1 line

2.9.3

Mao Zhongyi

author

2017-06-12 14:16 UTC #8 lore raw

PATCH v4 3/4 net/socket: Convert error report message to Error

In reply to 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 [email protected]

Signed-off-by Mao Zhongyi <[email protected]>

---
 net/socket.c | 57 ++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 34 insertions(+), 23 deletions(-)

net/socket.c +34 ~~-23~~

index 66016c8..9825422 100644
@@ -209,7 +209,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;

@@ -221,8 +223,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;

@@ -230,7 +232,8 @@ 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, "Create socket(PF_INET, SOCK_DGRAM)"
+                         " failed");
         return -1;
     }

@@ -242,13 +245,15 @@ 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, "Set the socket fd=%d attribute"
+                         " SO_REUSEADDR failed", fd);
         goto fail;
     }
 
     ret = bind(fd, (struct sockaddr *)mcastaddr, sizeof(*mcastaddr));
     if (ret < 0) {
-        perror("bind");
+        error_setg_errno(errp, errno, "Bind ip=%s to fd=%d failed",
+                         inet_ntoa(mcastaddr->sin_addr), fd);
         goto fail;
     }

@@ -263,7 +268,8 @@ 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, "Add socket fd=%d to multicast group %s"
+                " failed", fd, inet_ntoa(imr.imr_multiaddr));
         goto fail;
     }

@@ -272,7 +278,8 @@ 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, "Force multicast message to loopback"
+                         " failed");
         goto fail;
     }

@@ -281,7 +288,8 @@ 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, "Set the default network send "
+                             "interfaec failed");
             goto fail;
         }
     }

@@ -320,7 +328,8 @@ 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;

@@ -342,7 +351,7 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
                 goto err;
             }
             /* clone dgram socket */
-            newfd = net_socket_mcast_create(&saddr, NULL);
+            newfd = net_socket_mcast_create(&saddr, NULL, errp);
             if (newfd < 0) {
                 /* error already reported by net_socket_mcast_create() */
                 goto err;

@@ -352,8 +361,8 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
             close(newfd);
 
         } else {
-            fprintf(stderr,
-                    "qemu: error: init_dgram: fd=%d failed getsockname(): %s\n",
+            error_setg(errp,
+                    "qemu: error: init_dgram: fd=%d failed getsockname(): %s",
                     fd, strerror(errno));
             goto err;
         }

@@ -432,25 +441,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:
-        error_report("qemu: error: socket type=%d for fd=%d is not"
-                " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
+        error_setg(errp, "qemu: error: socket type=%d for fd=%d is not"
+              " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
         closesocket(fd);
     }
     return NULL;

@@ -571,7 +582,7 @@ static int net_socket_connect_init(NetClientState *peer,
             break;
         }
     }
-    s = net_socket_fd_init(peer, model, name, fd, connected);
+    s = net_socket_fd_init(peer, model, name, fd, connected, errp);
     if (!s)
         return -1;
     snprintf(s->nc.info_str, sizeof(s->nc.info_str),

@@ -603,11 +614,11 @@ static int net_socket_mcast_init(NetClientState *peer,
         param_localaddr = NULL;
     }
 
-    fd = net_socket_mcast_create(&saddr, param_localaddr);
+    fd = net_socket_mcast_create(&saddr, param_localaddr, errp);
     if (fd < 0)
         return -1;
 
-    s = net_socket_fd_init(peer, model, name, fd, 0);
+    s = net_socket_fd_init(peer, model, name, fd, 0, errp);
     if (!s)
         return -1;

@@ -658,7 +669,7 @@ 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, errp);
     if (!s) {
         return -1;
     }

@@ -702,7 +713,7 @@ 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, errp)) {
             return -1;
         }
         return 0;

Signature · 1 line

2.9.3

Mao Zhongyi

author

2017-06-12 14:16 UTC #4 lore raw

PATCH v4 2/4 net/net: Convert parse_host_port() to Error

In reply to Mao Zhongyi

Cc [email protected]

Signed-off-by Mao Zhongyi <[email protected]>

---
 include/qemu/sockets.h |  2 +-
 net/net.c              | 21 ++++++++++++++++-----
 net/socket.c           | 37 ++++++++++++++++++++++---------------
 3 files changed, 39 insertions(+), 21 deletions(-)

include/qemu/sockets.h +1 -1

index 5c326db..7abffc4 100644
@@ -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);
 
 /**

net/net.c +16 -5

index 6235aab..e55869a 100644
@@ -100,7 +100,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;

@@ -108,24 +108,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 address should contain ':', for example: "
+                   "xxxx=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, "Host address '%s' is not a valid "
+                           "IPv4 address", buf);
                 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;
 }

net/socket.c +22 ~~-15~~

index 53765bd..66016c8 100644
@@ -485,15 +485,17 @@ 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;
     struct sockaddr_in saddr;
     int fd, ret;
 
-    if (parse_host_port(&saddr, host_str) < 0)
+    if (parse_host_port(&saddr, host_str, errp) < 0) {
         return -1;
+    }
 
     fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {

@@ -531,14 +533,16 @@ static int net_socket_listen_init(NetClientState *peer,
 static int net_socket_connect_init(NetClientState *peer,
                                    const char *model,
                                    const char *name,
-                                   const char *host_str)
+                                   const char *host_str,
+                                   Error **errp)
 {
     NetSocketState *s;
     int fd, connected, ret;
     struct sockaddr_in saddr;
 
-    if (parse_host_port(&saddr, host_str) < 0)
+    if (parse_host_port(&saddr, host_str, errp) < 0) {
         return -1;
+    }
 
     fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {

@@ -580,14 +584,15 @@ 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;
     struct sockaddr_in saddr;
     struct in_addr localaddr, *param_localaddr;
 
-    if (parse_host_port(&saddr, host_str) < 0)
+    if (parse_host_port(&saddr, host_str, errp) < 0)
         return -1;
 
     if (localaddr_str != NULL) {

@@ -619,17 +624,18 @@ 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;
     struct sockaddr_in laddr, raddr;
 
-    if (parse_host_port(&laddr, lhost) < 0) {
+    if (parse_host_port(&laddr, lhost, errp) < 0) {
         return -1;
     }
 
-    if (parse_host_port(&raddr, rhost) < 0) {
+    if (parse_host_port(&raddr, rhost, errp) < 0) {
         return -1;
     }

@@ -703,15 +709,16 @@ 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,
+            errp) == -1) {
             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,
+            errp) == -1) {
             return -1;
         }
         return 0;

@@ -721,7 +728,7 @@ 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, errp) == -1) {
             return -1;
         }
         return 0;

@@ -732,8 +739,8 @@ int net_init_socket(const Netdev *netdev, const char *name,
         error_report("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,
+        errp) == -1) {
         return -1;
     }
     return 0;

Signature · 1 line

2.9.3

Markus Armbruster

Reviewed-by on v4 1/4

2017-06-23 13:36 UTC #2 lore raw

Re: [Qemu-devel] [PATCH v4 1/4] net/socket: Drop the odd 'default' case and comment

In reply to Mao Zhongyi

Quoted 28 lines · Mao Zhongyi

In the net_socket_fd_init(), the 'default' case and comment is odd. If @fd really was a pty, getsockopt() would fail with ENOTSOCK. If @fd was a socket, but neither SOCK_DGRAM nor SOCK_STREAM. It should not be treated as if it was SOCK_STREAM.
If there is a genuine reason to support something like SOCK_RAW, it should be explicitly handled.
So, drop the 'default' case since it is broken already.
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Suggested-by Markus Armbruster <[email protected]>
Suggested-by Daniel P. Berrange <[email protected]>
Signed-off-by Mao Zhongyi <[email protected]>
---
 net/socket.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
net/socket.c +0 -0
index dcae1ae..53765bd 100644
@@ -449,9 +449,9 @@ static NetSocketState *net_socket_fd_init(NetClientState *peer,
     case SOCK_STREAM:
         return net_socket_fd_init_stream(peer, model, name, fd, is_connected);

net/socket.c static NetSocketState *net_socket_fd_init(NetClientState *peer, · line 457

Quoted 10 lines

net/socket.c +3 -3

     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);
-        return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
+        error_report("qemu: error: socket type=%d for fd=%d is not"
+                " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
+        closesocket(fd);
     }
     return NULL;
 }

You don't actually "drop the 'default' case", as your commit message claims. Perhaps:

net/socket: Don't treat odd socket type as SOCK_STREAM

In net_socket_fd_init(), the 'default' case is odd: it warns, then continues as if the socket type was SOCK_STREAM. The comment explains "this could be a eg. a pty", but that makes no sense. If @fd really was a pty, getsockopt() would fail with ENOTSOCK. If @fd was a socket, but neither SOCK_DGRAM nor SOCK_STREAM, it should not be treated as if it was SOCK_STREAM.

Turn this case into an error. If there is a genuine reason to support something like SOCK_RAW, it should be handled explicitly.

With something like that:

Reviewed-by Markus Armbruster <[email protected]>

Markus Armbruster

2017-06-23 14:09 UTC #5 lore raw

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

In reply to Mao Zhongyi

Quoted 83 lines · Mao Zhongyi

Cc [email protected]

Signed-off-by Mao Zhongyi <[email protected]>

---
 include/qemu/sockets.h |  2 +-
 net/net.c              | 21 ++++++++++++++++-----
 net/socket.c           | 37 ++++++++++++++++++++++---------------
 3 files changed, 39 insertions(+), 21 deletions(-)

include/qemu/sockets.h +1 -1

index 5c326db..7abffc4 100644
@@ -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);
 
 /**

net/net.c +16 -5

index 6235aab..e55869a 100644
@@ -100,7 +100,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;

@@ -108,24 +108,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 address should contain ':', for example: "
+                   "xxxx=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, "Host address '%s' is not a valid "
+                           "IPv4 address", buf);
                 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;
 }

net/socket.c +0 -0

index 53765bd..66016c8 100644

net/socket.c static void net_socket_accept(void *opaque) · line 489

Quoted 6 lines

net/socket.c +2 -1

@@ -485,15 +485,17 @@ 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)

You convert net_socket_listen_init() to Error. This means you have to make it set an error on failure. You do ...

net/socket.c static void net_socket_accept(void *opaque) · line 498

Quoted 10 lines

net/socket.c +2 -1

 {
     NetClientState *nc;
     NetSocketState *s;
     struct sockaddr_in saddr;
     int fd, ret;
 
-    if (parse_host_port(&saddr, host_str) < 0)
+    if (parse_host_port(&saddr, host_str, errp) < 0) {
         return -1;
+    }

... here, ...

net/socket.c static void net_socket_accept(void *opaque) · line 500

Quoted 3 lines

net/socket.c +0 -0

     fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {

           perror("socket");
           return -1;
       }

... but not here. Two more error returns follow.

Preexisting bug: net_socket_listen_init() reports to stderr for three out of four failures. Functions should either report on no failure or on all. Conversion to Error should fix that, but yours isn't complete. When it is, the fix should be mentioned in the commit message.

Quoted 9 lines

net/socket.c +2 -1

@@ -531,14 +533,16 @@ static int net_socket_listen_init(NetClientState *peer,
 static int net_socket_connect_init(NetClientState *peer,
                                    const char *model,
                                    const char *name,
-                                   const char *host_str)
+                                   const char *host_str,
+                                   Error **errp)
 {
     NetSocketState *s;

net/socket.c static int net_socket_listen_init(NetClientState *peer, · line 548

Quoted 10 lines

net/socket.c +2 -1

     int fd, connected, ret;
     struct sockaddr_in saddr;
 
-    if (parse_host_port(&saddr, host_str) < 0)
+    if (parse_host_port(&saddr, host_str, errp) < 0) {
         return -1;
+    }
 
     fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {

Again, you have to make the function set an error on all failures, not just this one.

Additionally:

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

net_socket_fd_init() still prints to stderr. Please convert it to Error.

Quoted 8 lines

net/socket.c +2 -1

@@ -580,14 +584,15 @@ 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)
 {

net/socket.c static int net_socket_mcast_init(NetClientState *peer, · line 598

Quoted 10 lines

net/socket.c +1 -1

     NetSocketState *s;
     int fd;
     struct sockaddr_in saddr;
     struct in_addr localaddr, *param_localaddr;
 
-    if (parse_host_port(&saddr, host_str) < 0)
+    if (parse_host_port(&saddr, host_str, errp) < 0)
         return -1;
 
     if (localaddr_str != NULL) {

Again, you have to make the function set an error on all failures.

Additionally, convert net_socket_mcast_create() to Error.

Quoted 12 lines

net/socket.c +2 -1

@@ -619,17 +624,18 @@ 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;
     struct sockaddr_in laddr, raddr;

net/socket.c static int net_socket_udp_init(NetClientState *peer, · line 641

Quoted 10 lines

net/socket.c +2 -2

-    if (parse_host_port(&laddr, lhost) < 0) {
+    if (parse_host_port(&laddr, lhost, errp) < 0) {
         return -1;
     }
 
-    if (parse_host_port(&raddr, rhost) < 0) {
+    if (parse_host_port(&raddr, rhost, errp) < 0) {
         return -1;
     }

Again, you have make the function set an error on all failures.

Quoted 30 lines

net/socket.c +5 -4

@@ -703,15 +709,16 @@ 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,
+            errp) == -1) {
             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,
+            errp) == -1) {
             return -1;
         }
         return 0;

@@ -721,7 +728,7 @@ 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, errp) == -1) {
             return -1;
         }
         return 0;

net/socket.c int net_init_socket(const Netdev *netdev, const char *name, · line 746

Quoted 10 lines

net/socket.c +2 -2

@@ -732,8 +739,8 @@ int net_init_socket(const Netdev *netdev, const char *name,
         error_report("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,
+        errp) == -1) {
         return -1;
     }
     return 0;

This is a partial fix for net_init_socket()'s

    /* FIXME error_setg(errp, ...) on failure */

Can you fix it completely? Not required to get the patch accepted, as far as I'm concerned.

Your commit message claims "Convert parse_host_port() to Error". You do more than that. Please rephrase the commit message.

Markus Armbruster

2017-06-23 14:21 UTC #6 lore raw

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

Quoted 171 lines · Markus Armbruster

Quoted 83 lines · Mao Zhongyi

Cc [email protected]

Signed-off-by Mao Zhongyi <[email protected]>

---
 include/qemu/sockets.h |  2 +-
 net/net.c              | 21 ++++++++++++++++-----
 net/socket.c           | 37 ++++++++++++++++++++++---------------
 3 files changed, 39 insertions(+), 21 deletions(-)

include/qemu/sockets.h +1 -1

index 5c326db..7abffc4 100644
@@ -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);
 
 /**

net/net.c +16 -5

index 6235aab..e55869a 100644
@@ -100,7 +100,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;

@@ -108,24 +108,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 address should contain ':', for example: "
+                   "xxxx=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, "Host address '%s' is not a valid "
+                           "IPv4 address", buf);
                 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;
 }

net/socket.c +0 -0

index 53765bd..66016c8 100644

net/socket.c static void net_socket_accept(void *opaque) · line 489

Quoted 6 lines

net/socket.c +2 -1

@@ -485,15 +485,17 @@ 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)

You convert net_socket_listen_init() to Error. This means you have to make it set an error on failure. You do ...

net/socket.c static void net_socket_accept(void *opaque) · line 498

Quoted 10 lines

net/socket.c +2 -1

 {
     NetClientState *nc;
     NetSocketState *s;
     struct sockaddr_in saddr;
     int fd, ret;
 
-    if (parse_host_port(&saddr, host_str) < 0)
+    if (parse_host_port(&saddr, host_str, errp) < 0) {
         return -1;
+    }

... here, ...

net/socket.c static void net_socket_accept(void *opaque) · line 500

Quoted 3 lines

net/socket.c +0 -0

     fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {

           perror("socket");
           return -1;
       }

... but not here. Two more error returns follow.

Quoted 9 lines

net/socket.c +2 -1

@@ -531,14 +533,16 @@ static int net_socket_listen_init(NetClientState *peer,
 static int net_socket_connect_init(NetClientState *peer,
                                    const char *model,
                                    const char *name,
-                                   const char *host_str)
+                                   const char *host_str,
+                                   Error **errp)
 {
     NetSocketState *s;

net/socket.c static int net_socket_listen_init(NetClientState *peer, · line 548

Quoted 10 lines

net/socket.c +2 -1

     int fd, connected, ret;
     struct sockaddr_in saddr;
 
-    if (parse_host_port(&saddr, host_str) < 0)
+    if (parse_host_port(&saddr, host_str, errp) < 0) {
         return -1;
+    }
 
     fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {

Again, you have to make the function set an error on all failures, not just this one.

Additionally:

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

net_socket_fd_init() still prints to stderr. Please convert it to Error.

Quoted 17 lines

net/socket.c +3 -2

@@ -580,14 +584,15 @@ 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;
     struct sockaddr_in saddr;
     struct in_addr localaddr, *param_localaddr;
 
-    if (parse_host_port(&saddr, host_str) < 0)
+    if (parse_host_port(&saddr, host_str, errp) < 0)
         return -1;

Quoted 5 lines

net/socket.c static int net_socket_mcast_init(NetClientState *peer, · line 598
Quoted 1 line
net/socket.c +0 -0
     if (localaddr_str != NULL) {
Again, you have to make the function set an error on all failures.
Additionally, convert net_socket_mcast_create() to Error.

Actually, you do in the next patch. So, you first do an incorrect conversion in PATCH 2, then fix it in PATCH 3. Not horrible, but avoiding the broken intermediate state is easy enough: convert net_socket_mcast_create() to Error before PATCH 2, changing its callers from

    fd = net_socket_mcast_create(&saddr, param_localaddr);
    if (fd < 0) {
        // error already reported
        take the error exit
    }

    fd = net_socket_mcast_create(&saddr, param_localaddr, &err);
    if (fd < 0) {
        error_report_err(err);
        take the error exit
    }

The error_report_err() goes away when the function containing it gets converted to Error. Any questions?

Quoted 67 lines

Quoted 12 lines

net/socket.c +2 -1

@@ -619,17 +624,18 @@ 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;
     struct sockaddr_in laddr, raddr;

net/socket.c static int net_socket_udp_init(NetClientState *peer, · line 641

Quoted 10 lines

net/socket.c +2 -2

-    if (parse_host_port(&laddr, lhost) < 0) {
+    if (parse_host_port(&laddr, lhost, errp) < 0) {
         return -1;
     }
 
-    if (parse_host_port(&raddr, rhost) < 0) {
+    if (parse_host_port(&raddr, rhost, errp) < 0) {
         return -1;
     }

Again, you have make the function set an error on all failures.

Quoted 30 lines

net/socket.c +5 -4

@@ -703,15 +709,16 @@ 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,
+            errp) == -1) {
             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,
+            errp) == -1) {
             return -1;
         }
         return 0;

@@ -721,7 +728,7 @@ 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, errp) == -1) {
             return -1;
         }
         return 0;

net/socket.c int net_init_socket(const Netdev *netdev, const char *name, · line 746

Quoted 10 lines

net/socket.c +2 -2

@@ -732,8 +739,8 @@ int net_init_socket(const Netdev *netdev, const char *name,
         error_report("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,
+        errp) == -1) {
         return -1;
     }
     return 0;

This is a partial fix for net_init_socket()'s

Quoted 4 lines

    /* FIXME error_setg(errp, ...) on failure */
Can you fix it completely? Not required to get the patch accepted, as far as I'm concerned.

Ah, you do in PATCH 4. Suggest to explain in your commit message that this is a partial fix, which will be completed in the following commit(s).

Quoted 2 lines

Your commit message claims "Convert parse_host_port() to Error". You do more than that. Please rephrase the commit message.

Mao Zhongyi

author

2017-06-26 06:13 UTC #3 lore raw

Re: [Qemu-devel] [PATCH v4 1/4] net/socket: Drop the odd 'default' case and comment

In reply to Markus Armbruster

Hi, Markus

Quoted 53 lines · Markus Armbruster

Quoted 28 lines · Mao Zhongyi
In the net_socket_fd_init(), the 'default' case and comment is odd. If @fd really was a pty, getsockopt() would fail with ENOTSOCK. If @fd was a socket, but neither SOCK_DGRAM nor SOCK_STREAM. It should not be treated as if it was SOCK_STREAM.
If there is a genuine reason to support something like SOCK_RAW, it should be explicitly handled.
So, drop the 'default' case since it is broken already.
Cc [email protected]
Cc [email protected]
Cc [email protected]
Cc [email protected]
Suggested-by Markus Armbruster <[email protected]>
Suggested-by Daniel P. Berrange <[email protected]>
Signed-off-by Mao Zhongyi <[email protected]>
---
 net/socket.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
net/socket.c +0 -0
index dcae1ae..53765bd 100644
@@ -449,9 +449,9 @@ static NetSocketState *net_socket_fd_init(NetClientState *peer,
     case SOCK_STREAM:
         return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
net/socket.c static NetSocketState *net_socket_fd_init(NetClientState *peer, · line 457
Quoted 10 lines
net/socket.c +3 -3
     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);
-        return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
+        error_report("qemu: error: socket type=%d for fd=%d is not"
+                " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
+        closesocket(fd);
     }
     return NULL;
 }
You don't actually "drop the 'default' case", as your commit message claims. Perhaps:
net/socket: Don't treat odd socket type as SOCK_STREAM
In net_socket_fd_init(), the 'default' case is odd: it warns, then continues as if the socket type was SOCK_STREAM. The comment explains "this could be a eg. a pty", but that makes no sense. If @fd really was a pty, getsockopt() would fail with ENOTSOCK. If @fd was a socket, but neither SOCK_DGRAM nor SOCK_STREAM, it should not be treated as if it was SOCK_STREAM.

Quoted 4 lines

Turn this case into an error. If there is a genuine reason to support something like SOCK_RAW, it should be handled explicitly.
With something like that:

Yes, I have noticed that the commit message is really not very match this patch after read your exact description. I will fix it in the next version.

Thank you very much. :)
Mao

Quoted 1 line

Reviewed-by Markus Armbruster <[email protected]>

Mao Zhongyi

author

2017-06-26 06:18 UTC #7 lore raw

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

In reply to Markus Armbruster

Hi, Markus

Quoted 193 lines · Markus Armbruster

Quoted 171 lines · Markus Armbruster

Quoted 83 lines · Mao Zhongyi

Cc [email protected]

Signed-off-by Mao Zhongyi <[email protected]>

---
 include/qemu/sockets.h |  2 +-
 net/net.c              | 21 ++++++++++++++++-----
 net/socket.c           | 37 ++++++++++++++++++++++---------------
 3 files changed, 39 insertions(+), 21 deletions(-)

include/qemu/sockets.h +1 -1

index 5c326db..7abffc4 100644
@@ -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);
 
 /**

net/net.c +16 -5

index 6235aab..e55869a 100644
@@ -100,7 +100,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;

@@ -108,24 +108,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 address should contain ':', for example: "
+                   "xxxx=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, "Host address '%s' is not a valid "
+                           "IPv4 address", buf);
                 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;
 }

net/socket.c +0 -0

index 53765bd..66016c8 100644

net/socket.c static void net_socket_accept(void *opaque) · line 489

Quoted 6 lines

net/socket.c +2 -1

@@ -485,15 +485,17 @@ 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)

You convert net_socket_listen_init() to Error. This means you have to make it set an error on failure. You do ...

net/socket.c static void net_socket_accept(void *opaque) · line 498

Quoted 10 lines

net/socket.c +2 -1

 {
     NetClientState *nc;
     NetSocketState *s;
     struct sockaddr_in saddr;
     int fd, ret;
 
-    if (parse_host_port(&saddr, host_str) < 0)
+    if (parse_host_port(&saddr, host_str, errp) < 0) {
         return -1;
+    }

... here, ...

net/socket.c static void net_socket_accept(void *opaque) · line 500

Quoted 3 lines

net/socket.c +0 -0

     fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {

           perror("socket");
           return -1;
       }

... but not here. Two more error returns follow.

Quoted 9 lines

net/socket.c +2 -1

@@ -531,14 +533,16 @@ static int net_socket_listen_init(NetClientState *peer,
 static int net_socket_connect_init(NetClientState *peer,
                                    const char *model,
                                    const char *name,
-                                   const char *host_str)
+                                   const char *host_str,
+                                   Error **errp)
 {
     NetSocketState *s;

net/socket.c static int net_socket_listen_init(NetClientState *peer, · line 548

Quoted 10 lines

net/socket.c +2 -1

     int fd, connected, ret;
     struct sockaddr_in saddr;
 
-    if (parse_host_port(&saddr, host_str) < 0)
+    if (parse_host_port(&saddr, host_str, errp) < 0) {
         return -1;
+    }
 
     fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {

Again, you have to make the function set an error on all failures, not just this one.

Additionally:

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

net_socket_fd_init() still prints to stderr. Please convert it to Error.

Quoted 17 lines

net/socket.c +3 -2

@@ -580,14 +584,15 @@ 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;
     struct sockaddr_in saddr;
     struct in_addr localaddr, *param_localaddr;
 
-    if (parse_host_port(&saddr, host_str) < 0)
+    if (parse_host_port(&saddr, host_str, errp) < 0)
         return -1;

Quoted 5 lines

net/socket.c static int net_socket_mcast_init(NetClientState *peer, · line 598
Quoted 1 line
net/socket.c +0 -0
     if (localaddr_str != NULL) {
Again, you have to make the function set an error on all failures.
Additionally, convert net_socket_mcast_create() to Error.

    fd = net_socket_mcast_create(&saddr, param_localaddr);
    if (fd < 0) {
        // error already reported
        take the error exit
    }

Quoted 8 lines

    fd = net_socket_mcast_create(&saddr, param_localaddr, &err);
    if (fd < 0) {
        error_report_err(err);
        take the error exit
    }
The error_report_err() goes away when the function containing it gets converted to Error. Any questions?

Thanks for your detailed explanation, it inspired me. :)

This patchset looks a bit confusing because it doesn't have a correct order and error path, I'll rearrange to avoid it.

Thank you very much.
Mao

Quoted 82 lines

Quoted 68 lines

Quoted 13 lines

net/socket.c +2 -1

@@ -619,17 +624,18 @@ 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;
     struct sockaddr_in laddr, raddr;

net/socket.c static int net_socket_udp_init(NetClientState *peer, · line 641

Quoted 10 lines

net/socket.c +2 -2

-    if (parse_host_port(&laddr, lhost) < 0) {
+    if (parse_host_port(&laddr, lhost, errp) < 0) {
         return -1;
     }
 
-    if (parse_host_port(&raddr, rhost) < 0) {
+    if (parse_host_port(&raddr, rhost, errp) < 0) {
         return -1;
     }

Again, you have make the function set an error on all failures.

Quoted 30 lines

net/socket.c +5 -4

@@ -703,15 +709,16 @@ 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,
+            errp) == -1) {
             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,
+            errp) == -1) {
             return -1;
         }
         return 0;

@@ -721,7 +728,7 @@ 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, errp) == -1) {
             return -1;
         }
         return 0;

net/socket.c int net_init_socket(const Netdev *netdev, const char *name, · line 746

Quoted 10 lines

net/socket.c +2 -2

@@ -732,8 +739,8 @@ int net_init_socket(const Netdev *netdev, const char *name,
         error_report("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,
+        errp) == -1) {
         return -1;
     }
     return 0;

This is a partial fix for net_init_socket()'s

Quoted 4 lines

    /* FIXME error_setg(errp, ...) on failure */
Can you fix it completely? Not required to get the patch accepted, as far as I'm concerned.

Ah, you do in PATCH 4. Suggest to explain in your commit message that this is a partial fix, which will be completed in the following commit(s).

Quoted 2 lines

Your commit message claims "Convert parse_host_port() to Error". You do more than that. Please rephrase the commit message.