Improve error reporting

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]

Cc [email protected]

Cc [email protected]

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

Reviewed-by Daniel P. Berrange <[email protected]>

---
 net/socket.c | 81 +++++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 50 insertions(+), 31 deletions(-)

net/socket.c +3 -1

index 7d05e70..44fb504 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,16 +223,16 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
 #endif
 
     if (!IN_MULTICAST(ntohl(mcastaddr->sin_addr.s_addr))) {

net/socket.c +2 -1

             goto fail;
         }
     }

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

net/socket.c +1 -2

                 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;
             }
             /* clone newfd to fd, close newfd */

net/socket.c +1 -1

             goto err;
         }
     }

@@ -432,20 +439,22 @@ 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,

net/socket.c +19 -8

         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:

@@ -536,6 +545,7 @@ static int net_socket_connect_init(NetClientState *peer,
     NetSocketState *s;
     int fd, connected, ret;
     struct sockaddr_in saddr;
+    Error *err = NULL;
 
     if (parse_host_port(&saddr, host_str) < 0)
         return -1;

@@ -567,9 +577,11 @@ static int net_socket_connect_init(NetClientState *peer,
             break;
         }
     }
-    s = net_socket_fd_init(peer, model, name, fd, connected);
-    if (!s)
+    s = net_socket_fd_init(peer, model, name, fd, connected, &err);
+    if (!s) {
+        error_report_err(err);
         return -1;
+    }
     snprintf(s->nc.info_str, sizeof(s->nc.info_str),
              "socket: connect to %s:%d",
              inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));

@@ -586,6 +598,7 @@ static int net_socket_mcast_init(NetClientState *peer,
     int fd;
     struct sockaddr_in saddr;
     struct in_addr localaddr, *param_localaddr;
+    Error *err = NULL;
 
     if (parse_host_port(&saddr, host_str) < 0)
         return -1;

@@ -598,13 +611,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, &err);
+    if (fd < 0) {
+        error_report_err(err);
         return -1;
+    }
 
-    s = net_socket_fd_init(peer, model, name, fd, 0);
-    if (!s)
+    s = net_socket_fd_init(peer, model, name, fd, 0, &err);
+    if (!s) {
+        error_report_err(err);
         return -1;
+    }
 
     s->dgram_dst = saddr;
 

@@ -624,6 +641,7 @@ static int net_socket_udp_init(NetClientState *peer,
     NetSocketState *s;
     int fd, ret;
     struct sockaddr_in laddr, raddr;
+    Error *err = NULL;
 
     if (parse_host_port(&laddr, lhost) < 0) {
         return -1;

@@ -652,8 +670,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, &err);
     if (!s) {
+        error_report_err(err);
         return -1;
     }
 

Quoted 33 lines · 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]

Cc [email protected]

Cc [email protected]

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

Reviewed-by Daniel P. Berrange <[email protected]>

---
 net/socket.c | 81 +++++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 50 insertions(+), 31 deletions(-)

net/socket.c +3 -1

index 7d05e70..44fb504 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,16 +223,16 @@ static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr
 #endif
 

net/socket.c static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr · line 231

Quoted 11 lines

net/socket.c +4 -4

     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",
-                inet_ntoa(mcastaddr->sin_addr),
-                (int)ntohl(mcastaddr->sin_addr.s_addr));
+        error_setg(errp, "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;
 

You could drop the empty line here, and ...

net/socket.c static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr · line 232

Quoted 2 lines

net/socket.c +0 -0

     }

... maybe insert one here.

net/socket.c static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr · line 235

Quoted 4 lines

net/socket.c +1 -1

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

I suggested "can't create datagram socket" in my review of v5. I prefer "can't <do something>" over "failed to <do something>" myself, but grep shows more strings starting "failed to" than with "can't". I guess reporting "failed to <do something>" is more common. Okay.

net/socket.c static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr · line 248

Quoted 10 lines

net/socket.c +2 -1

         return -1;
     }
 

@@ -242,13 +244,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 'SO_REUSEADDR'"
+                         " attribute failed");

Adapting my review of v5 to the next instance: I'm not a native speaker, but "set FOO failed" doesn't feel right to me. Where's the subject? "Create" is a verb. "Setting FOO failed" has a subject, but doesn't feel right. What about "can't set socket option SO_REUSEADDR"?

Also, please avoid breaking the line in the middle of an argument when you could just as well break it between arguments, like this:

           error_setg_errno(errp, errno,
                            "can't set socket attribute SO_REUSEADDR");

net/socket.c static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr · line 255

Quoted 8 lines

net/socket.c +2 -1

         goto fail;
     }
 
     ret = bind(fd, (struct sockaddr *)mcastaddr, sizeof(*mcastaddr));
     if (ret < 0) {
-        perror("bind");
+        error_setg_errno(errp, errno, "bind ip=%s to socket failed",
+                         inet_ntoa(mcastaddr->sin_addr));

Likewise, "can't bind ip=%s to socket".

net/socket.c static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr · line 271

Quoted 10 lines

net/socket.c +2 -1

         goto fail;
     }
 

@@ -263,7 +267,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 to multicast group %s"
+                         " failed", inet_ntoa(imr.imr_multiaddr));

Likewise.

net/socket.c static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr · line 281

Quoted 10 lines

net/socket.c +2 -1

         goto fail;
     }
 

@@ -272,7 +277,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");

Likwise.

net/socket.c static int net_socket_mcast_create(struct sockaddr_in *mcastaddr, struct in_addr · line 291

Quoted 10 lines

net/socket.c +2 -1

         goto fail;
     }
 

@@ -281,7 +287,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 "
+                             "interface failed");

Likewise.

Quoted 17 lines

net/socket.c +2 -1

             goto fail;
         }
     }

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

@@ -337,14 +345,13 @@ 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) {

net/socket.c static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer, · line 349

Quoted 4 lines

net/socket.c +2 -2

-                fprintf(stderr, "qemu: error: init_dgram: fd=%d unbound, "
-                        "cannot setup multicast dst addr\n", fd);
+                error_setg(errp, "init_dgram: fd=%d unbound, "
+                           "cannot setup multicast dst addr", fd);

Yet another awful error message: "init_dgram" and the value of @fd are useful to a developer, but not to the user, and "dst addr" aren't even words. What about "can't setup multicast destination address"?

Aside: I wonder whether this is actually a programming error. How can it happen? Mao, I don't expect you to answer this :)

Quoted 11 lines

net/socket.c +1 -2

                 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;
             }
             /* clone newfd to fd, close newfd */

net/socket.c static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer, · line 364

Quoted 9 lines

net/socket.c +3 -3

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

Similarly awful error message. What about "can't get socket name"?

error_setg_errno() instead of strerror(), please.

Quoted 13 lines

net/socket.c +2 -1

             goto err;
         }
     }

@@ -432,20 +439,22 @@ 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);
 

net/socket.c static NetSocketState *net_socket_fd_init_stream(NetClientState *peer, · line 450

Quoted 6 lines

net/socket.c +2 -2

     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",
-                fd);
+        error_setg(errp, "getsockopt(SO_TYPE) for fd=%d failed",
+                   fd);

Another awful one. What about "can't get socket option SO_TYPE"?

Quoted 83 lines

net/socket.c +19 -8

         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:

@@ -536,6 +545,7 @@ static int net_socket_connect_init(NetClientState *peer,
     NetSocketState *s;
     int fd, connected, ret;
     struct sockaddr_in saddr;
+    Error *err = NULL;
 
     if (parse_host_port(&saddr, host_str) < 0)
         return -1;

@@ -567,9 +577,11 @@ static int net_socket_connect_init(NetClientState *peer,
             break;
         }
     }
-    s = net_socket_fd_init(peer, model, name, fd, connected);
-    if (!s)
+    s = net_socket_fd_init(peer, model, name, fd, connected, &err);
+    if (!s) {
+        error_report_err(err);
         return -1;
+    }
     snprintf(s->nc.info_str, sizeof(s->nc.info_str),
              "socket: connect to %s:%d",
              inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));

@@ -586,6 +598,7 @@ static int net_socket_mcast_init(NetClientState *peer,
     int fd;
     struct sockaddr_in saddr;
     struct in_addr localaddr, *param_localaddr;
+    Error *err = NULL;
 
     if (parse_host_port(&saddr, host_str) < 0)
         return -1;

@@ -598,13 +611,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, &err);
+    if (fd < 0) {
+        error_report_err(err);
         return -1;
+    }
 
-    s = net_socket_fd_init(peer, model, name, fd, 0);
-    if (!s)
+    s = net_socket_fd_init(peer, model, name, fd, 0, &err);
+    if (!s) {
+        error_report_err(err);
         return -1;
+    }
 
     s->dgram_dst = saddr;
 

@@ -624,6 +641,7 @@ static int net_socket_udp_init(NetClientState *peer,
     NetSocketState *s;
     int fd, ret;
     struct sockaddr_in laddr, raddr;
+    Error *err = NULL;
 
     if (parse_host_port(&laddr, lhost) < 0) {
         return -1;

@@ -652,8 +670,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, &err);
     if (!s) {
+        error_report_err(err);
         return -1;
     }
 

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

Quoted 9 lines

net/socket.c +1 -1

@@ -696,7 +715,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;

Looks good to me otherwise.

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

Cc [email protected]

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

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_socket_*_init() 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]

Cc [email protected]

Cc [email protected]

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

---
 net/socket.c | 94 +++++++++++++++++++++++++++++-------------------------------
 1 file changed, 45 insertions(+), 49 deletions(-)

net/socket.c +4 -5

index bd80b3c..a891c3a 100644
@@ -494,22 +494,21 @@ 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;
-    Error *err = NULL;
 
-    if (parse_host_port(&saddr, host_str, &err) < 0) {
-        error_report_err(err);
+    if (parse_host_port(&saddr, host_str, errp) < 0) {
         return -1;
     }
 
     fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {
-        perror("socket");
+        error_setg_errno(errp, errno, "failed to create stream socket");
         return -1;
     }
     qemu_set_nonblock(fd);

net/socket.c +4 -5

         closesocket(fd);
         return -1;
     }

@@ -543,21 +543,20 @@ 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;
-    Error *err = NULL;
 
-    if (parse_host_port(&saddr, host_str, &err) < 0) {
-        error_report_err(err);
+    if (parse_host_port(&saddr, host_str, errp) < 0) {
         return -1;
     }
 
     fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {
-        perror("socket");
+        error_setg_errno(errp, errno, "failed to create stream socket");
         return -1;
     }
     qemu_set_nonblock(fd);

net/socket.c +13 -15

                 closesocket(fd);
                 return -1;
             }

@@ -582,9 +581,8 @@ static int net_socket_connect_init(NetClientState *peer,
             break;
         }
     }
-    s = net_socket_fd_init(peer, model, name, fd, connected, &err);
+    s = net_socket_fd_init(peer, model, name, fd, connected, errp);
     if (!s) {
-        error_report_err(err);
         return -1;
     }
     snprintf(s->nc.info_str, sizeof(s->nc.info_str),

@@ -597,36 +595,36 @@ 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;
-    Error *err = NULL;
 
-    if (parse_host_port(&saddr, host_str, &err) < 0) {
-        error_report_err(err);
+    if (parse_host_port(&saddr, host_str, errp) < 0) {
         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;
     }
 
-    fd = net_socket_mcast_create(&saddr, param_localaddr, &err);
+    fd = net_socket_mcast_create(&saddr, param_localaddr, errp);
     if (fd < 0) {
-        error_report_err(err);
         return -1;
     }
 
-    s = net_socket_fd_init(peer, model, name, fd, 0, &err);
+    s = net_socket_fd_init(peer, model, name, fd, 0, errp);
     if (!s) {
-        error_report_err(err);
         return -1;
     }
 

@@ -643,45 +641,45 @@ 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;
-    Error *err = NULL;
 
-    if (parse_host_port(&laddr, lhost, &err) < 0) {
-        error_report_err(err);
+    if (parse_host_port(&laddr, lhost, errp) < 0) {
         return -1;
     }
 

net/socket.c +5 -9

         closesocket(fd);
         return -1;
     }
     qemu_set_nonblock(fd);
 
-    s = net_socket_fd_init(peer, model, name, fd, 0, &err);
+    s = net_socket_fd_init(peer, model, name, fd, 0, errp);
     if (!s) {
-        error_report_err(err);
         return -1;
     }
 

@@ -696,8 +694,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);

@@ -705,22 +701,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="
-                     " is required");
+        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);

net/socket.c +0 -0

             return -1;
         }
         return 0;

@@ -757,11 +753,11 @@ int net_init_socket(const Netdev *netdev, const char *name,
 

net/socket.c +0 -0

         return -1;
     }
     return 0;

Quoted 56 lines · 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_socket_*_init() 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]

Cc [email protected]

Cc [email protected]

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

---
 net/socket.c | 94 +++++++++++++++++++++++++++++-------------------------------
 1 file changed, 45 insertions(+), 49 deletions(-)

net/socket.c +4 -5

index bd80b3c..a891c3a 100644
@@ -494,22 +494,21 @@ 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;
-    Error *err = NULL;
 
-    if (parse_host_port(&saddr, host_str, &err) < 0) {
-        error_report_err(err);
+    if (parse_host_port(&saddr, host_str, errp) < 0) {
         return -1;
     }
 
     fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {
-        perror("socket");
+        error_setg_errno(errp, errno, "failed to create stream socket");
         return -1;
     }
     qemu_set_nonblock(fd);

@@ -518,13 +517,14 @@ static int net_socket_listen_init(NetClientState *peer,
 

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

Quoted 5 lines

net/socket.c +2 -1

     ret = bind(fd, (struct sockaddr *)&saddr, sizeof(saddr));
     if (ret < 0) {
-        perror("bind");
+        error_setg_errno(errp, errno, "bind ip=%s to socket failed",
+                         inet_ntoa(saddr.sin_addr));

Comment on the same error message in PATCH 2 applies.

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

Quoted 8 lines

net/socket.c +1 -1

         closesocket(fd);
         return -1;
     }
     ret = listen(fd, 0);
     if (ret < 0) {
-        perror("listen");
+        error_setg_errno(errp, errno, "listen socket failed");

Suggest "can't listen on socket".

Quoted 1 line

net/socket.c +0 -1

-    fd = net_socket_mcast_create(&saddr, param_localaddr, &err);

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

Quoted 4 lines

     if (sock->has_listen) {

net/socket.c +2 -1

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

Please avoid breaking the line in the middle of an operator's operand when you could just as well break it at the operator, like this:

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

Since you're touching this line anyway, I suggest to replace == -1 by the more idiomatic < 0.