[Qemu-devel] [PATCH v5 0/4] Improve error reporting

[Qemu-devel] [PATCH v5 0/4] Improve error reporting

[Mao Zhongyi]

v5:
* PATCH 01 make the commit message more exact about the actual function.    [Markus Armbruster]
* PATCH 02, 03, 04 still retains the original function, but specific
           content and order of each patch has been adjusted substantially, 
           so that ensure each patch is a completed fix.    [Markus Armbruster]

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: jasowang@redhat.com
Cc: armbru@redhat.com
Cc: berrange@redhat.com
Cc: kraxel@redhat.com
Cc: pbonzini@redhat.com

Mao Zhongyi (4):
  net/socket: Don't treat odd socket type as SOCK_STREAM
  net/socket: Convert error message to Error
  net/net: Convert parse_host_port() to Error
  net/socket: Improve -net socket error reporting

 include/qemu/sockets.h |   3 +-
 net/net.c              |  22 ++++++--
 net/socket.c           | 150 +++++++++++++++++++++++++++++--------------------
 3 files changed, 108 insertions(+), 67 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH v5 1/4] net/socket: Don't treat odd socket type as SOCK_STREAM

[Mao Zhongyi]

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 explicitly
handled.

Cc: jasowang@redhat.com
Cc: armbru@redhat.com
Cc: berrange@redhat.com
Cc: armbru@redhat.com
Suggested-by: Markus Armbruster <armbru@redhat.com>
Suggested-by: Daniel P. Berrange <berrange@redhat.com>
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 net/socket.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index dcae1ae..53765bd 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -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;
 }
-- 
2.9.4

[Qemu-devel] [PATCH v5 2/4] net/socket: Convert error message to Error

[Mao Zhongyi]

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

Now, convert these functions to Error.

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

diff --git a/net/socket.c b/net/socket.c
index 53765bd..e136f87 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -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;
@@ -337,14 +346,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) {
-                fprintf(stderr, "qemu: error: init_dgram: fd=%d unbound, "
-                        "cannot setup multicast dst addr\n", fd);
+                error_setg(errp, "qemu: error: init_dgram: fd=%d unbound, "
+                        "cannot setup multicast dst addr", fd);
                 goto err;
             }
             /* clone dgram socket */
-            newfd = net_socket_mcast_create(&saddr, NULL);
+            newfd = net_socket_mcast_create(&saddr, NULL, errp);
             if (newfd < 0) {
-                /* error already reported by net_socket_mcast_create() */
                 goto err;
             }
             /* clone newfd to fd, close newfd */
@@ -352,8 +360,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,24 +440,26 @@ 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"
+        error_setg(errp, "qemu: error: socket type=%d for fd=%d is not"
                 " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
         closesocket(fd);
     }
@@ -536,6 +546,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 +578,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 +599,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 +612,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 +642,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 +671,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;
     }
 
@@ -696,7 +716,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;
-- 
2.9.4

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

[Mao Zhongyi]

Cc: berrange@redhat.com
Cc: kraxel@redhat.com
Cc: pbonzini@redhat.com
Cc: jasowang@redhat.com
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
 include/qemu/sockets.h |  3 ++-
 net/net.c              | 22 +++++++++++++++++-----
 net/socket.c           | 19 ++++++++++++++-----
 3 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 5c326db..78e2b30 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -53,7 +53,8 @@ void socket_listen_cleanup(int fd, Error **errp);
 int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
 
 /* Old, ipv4 only bits.  Don't use for new code. */
-int parse_host_port(struct sockaddr_in *saddr, const char *str);
+int parse_host_port(struct sockaddr_in *saddr, const char *str,
+                    Error **errp);
 int socket_init(void);
 
 /**
diff --git a/net/net.c b/net/net.c
index 6235aab..884e3ac 100644
--- a/net/net.c
+++ b/net/net.c
@@ -100,7 +100,8 @@ 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 +109,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;
 }
diff --git a/net/socket.c b/net/socket.c
index e136f87..a875205 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -501,9 +501,12 @@ static int net_socket_listen_init(NetClientState *peer,
     NetSocketState *s;
     struct sockaddr_in saddr;
     int fd, ret;
+    Error *err = NULL;
 
-    if (parse_host_port(&saddr, host_str) < 0)
+    if (parse_host_port(&saddr, host_str, &err) < 0) {
+        error_report_err(err);
         return -1;
+    }
 
     fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {
@@ -548,8 +551,10 @@ static int net_socket_connect_init(NetClientState *peer,
     struct sockaddr_in saddr;
     Error *err = NULL;
 
-    if (parse_host_port(&saddr, host_str) < 0)
+    if (parse_host_port(&saddr, host_str, &err) < 0) {
+        error_report_err(err);
         return -1;
+    }
 
     fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {
@@ -601,8 +606,10 @@ static int net_socket_mcast_init(NetClientState *peer,
     struct in_addr localaddr, *param_localaddr;
     Error *err = NULL;
 
-    if (parse_host_port(&saddr, host_str) < 0)
+    if (parse_host_port(&saddr, host_str, &err) < 0) {
+        error_report_err(err);
         return -1;
+    }
 
     if (localaddr_str != NULL) {
         if (inet_aton(localaddr_str, &localaddr) == 0)
@@ -644,11 +651,13 @@ static int net_socket_udp_init(NetClientState *peer,
     struct sockaddr_in laddr, raddr;
     Error *err = NULL;
 
-    if (parse_host_port(&laddr, lhost) < 0) {
+    if (parse_host_port(&laddr, lhost, &err) < 0) {
+        error_report_err(err);
         return -1;
     }
 
-    if (parse_host_port(&raddr, rhost) < 0) {
+    if (parse_host_port(&raddr, rhost, &err) < 0) {
+        error_report_err(err);
         return -1;
     }
 
-- 
2.9.4

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

[Mao Zhongyi]

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

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

Convert net_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: jasowang@redhat.com
Cc: armbru@redhat.com
Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
---
 net/socket.c | 95 ++++++++++++++++++++++++++++++------------------------------
 1 file changed, 47 insertions(+), 48 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index a875205..8be37d1 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -495,22 +495,22 @@ 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, "Create socket(PF_INET,"
+                         " SOCK_STREAM) failed");
         return -1;
     }
     qemu_set_nonblock(fd);
@@ -519,13 +519,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;
     }
@@ -544,21 +545,21 @@ 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, "Create socket(PF_INET,"
+                         " SOCK_STREAM) failed");
         return -1;
     }
     qemu_set_nonblock(fd);
@@ -574,7 +575,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;
             }
@@ -583,9 +584,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),
@@ -598,36 +598,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;
     }
 
@@ -644,45 +644,46 @@ 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;
     }
 
-    if (parse_host_port(&raddr, rhost, &err) < 0) {
-        error_report_err(err);
+    if (parse_host_port(&raddr, rhost, errp) < 0) {
         return -1;
     }
 
     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;
     }
     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;
     }
 
@@ -697,8 +698,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);
@@ -706,22 +705,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);
@@ -732,15 +730,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;
@@ -749,8 +748,8 @@ int net_init_socket(const Netdev *netdev, const char *name,
     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) {
+        if (net_socket_mcast_init(peer, "socket", name,
+            sock->mcast, sock->localaddr, errp) == -1) {
             return -1;
         }
         return 0;
@@ -758,11 +757,11 @@ int net_init_socket(const Netdev *netdev, const char *name,
 
     assert(sock->has_udp);
     if (!sock->has_localaddr) {
-        error_report("localaddr= is mandatory with udp=");
+        error_setg(errp, "localaddr= is mandatory with udp=");
         return -1;
     }
-    if (net_socket_udp_init(peer, "socket", name, sock->udp, sock->localaddr) ==
-        -1) {
+    if (net_socket_udp_init(peer, "socket", name,
+        sock->udp, sock->localaddr, errp) == -1) {
         return -1;
     }
     return 0;
-- 
2.9.4

Re: [Qemu-devel] [PATCH v5 2/4] net/socket: Convert error message to Error

[Markus Armbruster]

Suggest "net/socket: Convert several helper functions to Error".

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

> Currently, net_socket_mcast_create(), net_socket_fd_init_dgram() and
> net_socket_fd_init() use the function such as fprintf(), perror() to
> report an error message.
>
> Now, convert these functions to Error.
>
> Cc: jasowang@redhat.com
> Cc: armbru@redhat.com
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
>  net/socket.c | 76 ++++++++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 48 insertions(+), 28 deletions(-)
>
> diff --git a/net/socket.c b/net/socket.c
> index 53765bd..e136f87 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -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",

Please drop the "qemu: error: " prefix.  More of the same below.

>                  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");

I'm not a native speaker, but "Create FOO failed" doesn't feel right to
me.  Where's the subject?  "Create" is a verb.  "Creation of FOO failed"
has a subject, but doesn't feel right.  What about "can't create
datagram socket"?

Same for many of the other new error messages.

>          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);

Why would the user want to know the value of @fd here?

Same for many of the other new error messages.

>          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");

s/interfaec/interface/

>              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;
> @@ -337,14 +346,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) {
> -                fprintf(stderr, "qemu: error: init_dgram: fd=%d unbound, "
> -                        "cannot setup multicast dst addr\n", fd);
> +                error_setg(errp, "qemu: error: init_dgram: fd=%d unbound, "
> +                        "cannot setup multicast dst addr", fd);
>                  goto err;
>              }
>              /* clone dgram socket */
> -            newfd = net_socket_mcast_create(&saddr, NULL);
> +            newfd = net_socket_mcast_create(&saddr, NULL, errp);
>              if (newfd < 0) {
> -                /* error already reported by net_socket_mcast_create() */
>                  goto err;
>              }
>              /* clone newfd to fd, close newfd */
> @@ -352,8 +360,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,24 +440,26 @@ 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"
> +        error_setg(errp, "qemu: error: socket type=%d for fd=%d is not"
>                  " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
>          closesocket(fd);
>      }
> @@ -536,6 +546,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 +578,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 +599,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 +612,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 +642,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 +671,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;
>      }
>  
> @@ -696,7 +716,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.

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

[Markus Armbruster]

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

> Cc: berrange@redhat.com
> Cc: kraxel@redhat.com
> Cc: pbonzini@redhat.com
> Cc: jasowang@redhat.com
> Cc: armbru@redhat.com
> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
> ---
>  include/qemu/sockets.h |  3 ++-
>  net/net.c              | 22 +++++++++++++++++-----
>  net/socket.c           | 19 ++++++++++++++-----
>  3 files changed, 33 insertions(+), 11 deletions(-)
>
> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> index 5c326db..78e2b30 100644
> --- a/include/qemu/sockets.h
> +++ b/include/qemu/sockets.h
> @@ -53,7 +53,8 @@ void socket_listen_cleanup(int fd, Error **errp);
>  int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
>  
>  /* Old, ipv4 only bits.  Don't use for new code. */
> -int parse_host_port(struct sockaddr_in *saddr, const char *str);
> +int parse_host_port(struct sockaddr_in *saddr, const char *str,
> +                    Error **errp);
>  int socket_init(void);
>  
>  /**
> diff --git a/net/net.c b/net/net.c
> index 6235aab..884e3ac 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -100,7 +100,8 @@ 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 +109,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");

Suggest "Host address '%s' should ..." like you do in the next error message.

The xxxx= is confusing.  Do we need an example here?  The other error
messages in this function apparently don't.

What about "host address '%s' doesn't contain ':' separating host from
port" or "can't find ':' separating host from port in host address
'%s'"?


>          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.");

No.  Suggest "can't resolve host address '%s'.  This error message still
lacks detail, but I'm not sure hstrerror() is sufficiently portable.

Outside this patch's scope: gethostbyname() is obsolete.  Applications
should use getaddrinfo() instead.  Comes with gai_strerror().

>                  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");

Suggest "Port number '%s' is invalid".

>          return -1;
> +    }
>      saddr->sin_port = htons(port);
>      return 0;
>  }
> diff --git a/net/socket.c b/net/socket.c
> index e136f87..a875205 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -501,9 +501,12 @@ static int net_socket_listen_init(NetClientState *peer,
>      NetSocketState *s;
>      struct sockaddr_in saddr;
>      int fd, ret;
> +    Error *err = NULL;
>  
> -    if (parse_host_port(&saddr, host_str) < 0)
> +    if (parse_host_port(&saddr, host_str, &err) < 0) {
> +        error_report_err(err);
>          return -1;
> +    }
>  
>      fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
>      if (fd < 0) {
> @@ -548,8 +551,10 @@ static int net_socket_connect_init(NetClientState *peer,
>      struct sockaddr_in saddr;
>      Error *err = NULL;
>  
> -    if (parse_host_port(&saddr, host_str) < 0)
> +    if (parse_host_port(&saddr, host_str, &err) < 0) {
> +        error_report_err(err);
>          return -1;
> +    }
>  
>      fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
>      if (fd < 0) {
> @@ -601,8 +606,10 @@ static int net_socket_mcast_init(NetClientState *peer,
>      struct in_addr localaddr, *param_localaddr;
>      Error *err = NULL;
>  
> -    if (parse_host_port(&saddr, host_str) < 0)
> +    if (parse_host_port(&saddr, host_str, &err) < 0) {
> +        error_report_err(err);
>          return -1;
> +    }
>  
>      if (localaddr_str != NULL) {
>          if (inet_aton(localaddr_str, &localaddr) == 0)
> @@ -644,11 +651,13 @@ static int net_socket_udp_init(NetClientState *peer,
>      struct sockaddr_in laddr, raddr;
>      Error *err = NULL;
>  
> -    if (parse_host_port(&laddr, lhost) < 0) {
> +    if (parse_host_port(&laddr, lhost, &err) < 0) {
> +        error_report_err(err);
>          return -1;
>      }
>  
> -    if (parse_host_port(&raddr, rhost) < 0) {
> +    if (parse_host_port(&raddr, rhost, &err) < 0) {
> +        error_report_err(err);
>          return -1;
>      }

Looks good otherwise.

Re: [Qemu-devel] [PATCH v5 2/4] net/socket: Convert error message to Error

[Mao Zhongyi]

Hi, Markus

On 06/27/2017 03:43 PM, Markus Armbruster wrote:
> Suggest "net/socket: Convert several helper functions to Error".

OK, I see.

>
> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>
>> Currently, net_socket_mcast_create(), net_socket_fd_init_dgram() and
>> net_socket_fd_init() use the function such as fprintf(), perror() to
>> report an error message.
>>
>> Now, convert these functions to Error.
>>
>> Cc: jasowang@redhat.com
>> Cc: armbru@redhat.com
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> ---
>>  net/socket.c | 76 ++++++++++++++++++++++++++++++++++++++----------------------
>>  1 file changed, 48 insertions(+), 28 deletions(-)
>>
>> diff --git a/net/socket.c b/net/socket.c
>> index 53765bd..e136f87 100644
>> --- a/net/socket.c
>> +++ b/net/socket.c
>> @@ -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",
>
> Please drop the "qemu: error: " prefix.  More of the same below.

OK, I will


>
>>                  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, ")"
>> +                         " failed");
>
> I'm not a native speaker, but "Create FOO failed" doesn't feel right to
> me.  Where's the subject?  "Create" is a verb.  "Creation of FOO failed"
> has a subject, but doesn't feel right.  What about "can't create
> datagram socket"?
>

Thanks for your sharing.
My English is really poor, I will continue to improve.


> Same for many of the other new error messages.
>
>>          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);
>
> Why would the user want to know the value of @fd here?
>
> Same for many of the other new error messages.

It really doesn't make sense. I will drop it.

>
>>          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");
>
> s/interfaec/interface/

OK, I see.

Thanks,
Mao

>
>>              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;
>> @@ -337,14 +346,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) {
>> -                fprintf(stderr, "qemu: error: init_dgram: fd=%d unbound, "
>> -                        "cannot setup multicast dst addr\n", fd);
>> +                error_setg(errp, "qemu: error: init_dgram: fd=%d unbound, "
>> +                        "cannot setup multicast dst addr", fd);
>>                  goto err;
>>              }
>>              /* clone dgram socket */
>> -            newfd = net_socket_mcast_create(&saddr, NULL);
>> +            newfd = net_socket_mcast_create(&saddr, NULL, errp);
>>              if (newfd < 0) {
>> -                /* error already reported by net_socket_mcast_create() */
>>                  goto err;
>>              }
>>              /* clone newfd to fd, close newfd */
>> @@ -352,8 +360,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,24 +440,26 @@ 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"
>> +        error_setg(errp, "qemu: error: socket type=%d for fd=%d is not"
>>                  " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
>>          closesocket(fd);
>>      }
>> @@ -536,6 +546,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 +578,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 +599,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 +612,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 +642,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 +671,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;
>>      }
>>
>> @@ -696,7 +716,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.
>
>
>

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

[Mao Zhongyi]

Hi, Markus

On 06/27/2017 04:04 PM, Markus Armbruster wrote:
> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>
>> Cc: berrange@redhat.com
>> Cc: kraxel@redhat.com
>> Cc: pbonzini@redhat.com
>> Cc: jasowang@redhat.com
>> Cc: armbru@redhat.com
>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>> ---
>>  include/qemu/sockets.h |  3 ++-
>>  net/net.c              | 22 +++++++++++++++++-----
>>  net/socket.c           | 19 ++++++++++++++-----
>>  3 files changed, 33 insertions(+), 11 deletions(-)
>>
>> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
>> index 5c326db..78e2b30 100644
>> --- a/include/qemu/sockets.h
>> +++ b/include/qemu/sockets.h
>> @@ -53,7 +53,8 @@ void socket_listen_cleanup(int fd, Error **errp);
>>  int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
>>
>>  /* Old, ipv4 only bits.  Don't use for new code. */
>> -int parse_host_port(struct sockaddr_in *saddr, const char *str);
>> +int parse_host_port(struct sockaddr_in *saddr, const char *str,
>> +                    Error **errp);
>>  int socket_init(void);
>>
>>  /**
>> diff --git a/net/net.c b/net/net.c
>> index 6235aab..884e3ac 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -100,7 +100,8 @@ 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 +109,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");
>
> Suggest "Host address '%s' should ..." like you do in the next error message.
>
> The xxxx= is confusing.  Do we need an example here?  The other error
> messages in this function apparently don't.
>
> What about "host address '%s' doesn't contain ':' separating host from
> port" or "can't find ':' separating host from port in host address
> '%s'"?
>

Yes, my description message is really confusing.
Both of your prompt message are well understood.

Thank you very much.

>
>>          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.");
>
> No.  Suggest "can't resolve host address '%s'.  This error message still
> lacks detail, but I'm not sure hstrerror() is sufficiently portable.
>

The gethostbyname() return a null pointer if an error occurs, and the h_errno
variable holds an error number. herror() and hstrerror() can prints the error
message associated with the current value of h_errno, but hstrerror() returns
the string type is good for passing the error message to Error. So I'd prefer
the hstrerror.

As for the portability of hstrerror(), sorry, I'm also not sure, but in this
case I tested, it's OK. so I want to use hstrerror() for a while, if there are
any problem that can be fixed later. Do you think it can be done?


> Outside this patch's scope: gethostbyname() is obsolete.  Applications
> should use getaddrinfo() instead.  Comes with gai_strerror().

Can I try to fix it later?

Thanks,
Mao

>
>>                  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");
>
> Suggest "Port number '%s' is invalid".
>
>>          return -1;
>> +    }
>>      saddr->sin_port = htons(port);
>>      return 0;
>>  }
>> diff --git a/net/socket.c b/net/socket.c
>> index e136f87..a875205 100644
>> --- a/net/socket.c
>> +++ b/net/socket.c
>> @@ -501,9 +501,12 @@ static int net_socket_listen_init(NetClientState *peer,
>>      NetSocketState *s;
>>      struct sockaddr_in saddr;
>>      int fd, ret;
>> +    Error *err = NULL;
>>
>> -    if (parse_host_port(&saddr, host_str) < 0)
>> +    if (parse_host_port(&saddr, host_str, &err) < 0) {
>> +        error_report_err(err);
>>          return -1;
>> +    }
>>
>>      fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
>>      if (fd < 0) {
>> @@ -548,8 +551,10 @@ static int net_socket_connect_init(NetClientState *peer,
>>      struct sockaddr_in saddr;
>>      Error *err = NULL;
>>
>> -    if (parse_host_port(&saddr, host_str) < 0)
>> +    if (parse_host_port(&saddr, host_str, &err) < 0) {
>> +        error_report_err(err);
>>          return -1;
>> +    }
>>
>>      fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
>>      if (fd < 0) {
>> @@ -601,8 +606,10 @@ static int net_socket_mcast_init(NetClientState *peer,
>>      struct in_addr localaddr, *param_localaddr;
>>      Error *err = NULL;
>>
>> -    if (parse_host_port(&saddr, host_str) < 0)
>> +    if (parse_host_port(&saddr, host_str, &err) < 0) {
>> +        error_report_err(err);
>>          return -1;
>> +    }
>>
>>      if (localaddr_str != NULL) {
>>          if (inet_aton(localaddr_str, &localaddr) == 0)
>> @@ -644,11 +651,13 @@ static int net_socket_udp_init(NetClientState *peer,
>>      struct sockaddr_in laddr, raddr;
>>      Error *err = NULL;
>>
>> -    if (parse_host_port(&laddr, lhost) < 0) {
>> +    if (parse_host_port(&laddr, lhost, &err) < 0) {
>> +        error_report_err(err);
>>          return -1;
>>      }
>>
>> -    if (parse_host_port(&raddr, rhost) < 0) {
>> +    if (parse_host_port(&raddr, rhost, &err) < 0) {
>> +        error_report_err(err);
>>          return -1;
>>      }
>
> Looks good otherwise.
>
>
>

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

[Markus Armbruster]

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

> Hi, Markus
>
> On 06/27/2017 04:04 PM, Markus Armbruster wrote:
>> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>>
>>> Cc: berrange@redhat.com
>>> Cc: kraxel@redhat.com
>>> Cc: pbonzini@redhat.com
>>> Cc: jasowang@redhat.com
>>> Cc: armbru@redhat.com
>>> Signed-off-by: Mao Zhongyi <maozy.fnst@cn.fujitsu.com>
>>> ---
>>>  include/qemu/sockets.h |  3 ++-
>>>  net/net.c              | 22 +++++++++++++++++-----
>>>  net/socket.c           | 19 ++++++++++++++-----
>>>  3 files changed, 33 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
>>> index 5c326db..78e2b30 100644
>>> --- a/include/qemu/sockets.h
>>> +++ b/include/qemu/sockets.h
>>> @@ -53,7 +53,8 @@ void socket_listen_cleanup(int fd, Error **errp);
>>>  int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
>>>
>>>  /* Old, ipv4 only bits.  Don't use for new code. */
>>> -int parse_host_port(struct sockaddr_in *saddr, const char *str);
>>> +int parse_host_port(struct sockaddr_in *saddr, const char *str,
>>> +                    Error **errp);
>>>  int socket_init(void);
>>>
>>>  /**
>>> diff --git a/net/net.c b/net/net.c
>>> index 6235aab..884e3ac 100644
>>> --- a/net/net.c
>>> +++ b/net/net.c
>>> @@ -100,7 +100,8 @@ 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 +109,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");
>>
>> Suggest "Host address '%s' should ..." like you do in the next error message.
>>
>> The xxxx= is confusing.  Do we need an example here?  The other error
>> messages in this function apparently don't.
>>
>> What about "host address '%s' doesn't contain ':' separating host from
>> port" or "can't find ':' separating host from port in host address
>> '%s'"?
>>
>
> Yes, my description message is really confusing.
> Both of your prompt message are well understood.
>
> Thank you very much.
>
>>
>>>          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.");
>>
>> No.  Suggest "can't resolve host address '%s'.  This error message still
>> lacks detail, but I'm not sure hstrerror() is sufficiently portable.
>>
>
> The gethostbyname() return a null pointer if an error occurs, and the h_errno
> variable holds an error number. herror() and hstrerror() can prints the error
> message associated with the current value of h_errno, but hstrerror() returns
> the string type is good for passing the error message to Error. So I'd prefer
> the hstrerror.
>
> As for the portability of hstrerror(), sorry, I'm also not sure, but in this
> case I tested, it's OK. so I want to use hstrerror() for a while, if there are
> any problem that can be fixed later. Do you think it can be done?

Standard first portability question: does Windows provide it?

>> Outside this patch's scope: gethostbyname() is obsolete.  Applications
>> should use getaddrinfo() instead.  Comes with gai_strerror().
>
> Can I try to fix it later?

Sure.  By "outside this patch's scope" I mean it's a separate matter
that clearly doesn't have to be addressed to get this patch accepted.

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

[Paolo Bonzini]

On 28/06/2017 07:51, Markus Armbruster wrote:
>> The gethostbyname() return a null pointer if an error occurs, and the h_errno
>> variable holds an error number. herror() and hstrerror() can prints the error
>> message associated with the current value of h_errno, but hstrerror() returns
>> the string type is good for passing the error message to Error. So I'd prefer
>> the hstrerror.
>>
>> As for the portability of hstrerror(), sorry, I'm also not sure, but in this
>> case I tested, it's OK. so I want to use hstrerror() for a while, if there are
>> any problem that can be fixed later. Do you think it can be done?
>
> Standard first portability question: does Windows provide it?

Nope.  But it does have gai_strerror.

Paolo

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

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> On 28/06/2017 07:51, Markus Armbruster wrote:
>>> The gethostbyname() return a null pointer if an error occurs, and the h_errno
>>> variable holds an error number. herror() and hstrerror() can prints the error
>>> message associated with the current value of h_errno, but hstrerror() returns
>>> the string type is good for passing the error message to Error. So I'd prefer
>>> the hstrerror.
>>>
>>> As for the portability of hstrerror(), sorry, I'm also not sure, but in this
>>> case I tested, it's OK. so I want to use hstrerror() for a while, if there are
>>> any problem that can be fixed later. Do you think it can be done?
>>
>> Standard first portability question: does Windows provide it?
>
> Nope.  But it does have gai_strerror.

Let's go with the generic error message I suggested, and leave adding
detail to the patch that converts to getaddrinfo().

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

[Mao Zhongyi]

On 06/28/2017 06:56 PM, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> On 28/06/2017 07:51, Markus Armbruster wrote:
>>>> The gethostbyname() return a null pointer if an error occurs, and the h_errno
>>>> variable holds an error number. herror() and hstrerror() can prints the error
>>>> message associated with the current value of h_errno, but hstrerror() returns
>>>> the string type is good for passing the error message to Error. So I'd prefer
>>>> the hstrerror.
>>>>
>>>> As for the portability of hstrerror(), sorry, I'm also not sure, but in this
>>>> case I tested, it's OK. so I want to use hstrerror() for a while, if there are
>>>> any problem that can be fixed later. Do you think it can be done?
>>>
>>> Standard first portability question: does Windows provide it?
>>
>> Nope.  But it does have gai_strerror.
>
> Let's go with the generic error message I suggested, and leave adding
> detail to the patch that converts to getaddrinfo().

OK, I will fix it right away.

Thanks,
Mao