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

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

[Mao Zhongyi]

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

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

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

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

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

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


Cc: jasowang@redhat.com
Cc: armbru@redhat.com
Cc: berrange@redhat.com
Cc: kraxel@redhat.com
Cc: pbonzini@redhat.com

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

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

-- 
2.9.3

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

[Mao Zhongyi]

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

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

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

Cc: 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>
---
 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.3

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

[Markus Armbruster]

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

> In the net_socket_fd_init(), the 'default' case and comment is odd.
> If @fd really was a pty, getsockopt() would fail with ENOTSOCK. If
> @fd was a socket, but neither SOCK_DGRAM nor SOCK_STREAM. It should
> not be treated as if it was SOCK_STREAM.
>
> If there is a genuine reason to support something like SOCK_RAW, it
> should be explicitly handled.
>
> So, drop the 'default' case since it is broken already.
>
> Cc: 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>
> ---
>  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;
>  }

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

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

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

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

With something like that:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Mao Zhongyi]

Hi, Markus


On 06/23/2017 09:36 PM, Markus Armbruster wrote:
> Mao Zhongyi <maozy.fnst@cn.fujitsu.com> writes:
>
>> In the net_socket_fd_init(), the 'default' case and comment is odd.
>> If @fd really was a pty, getsockopt() would fail with ENOTSOCK. If
>> @fd was a socket, but neither SOCK_DGRAM nor SOCK_STREAM. It should
>> not be treated as if it was SOCK_STREAM.
>>
>> If there is a genuine reason to support something like SOCK_RAW, it
>> should be explicitly handled.
>>
>> So, drop the 'default' case since it is broken already.
>>
>> Cc: 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>
>> ---
>>  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;
>>  }
>
> You don't actually "drop the 'default' case", as your commit message
> claims.  Perhaps:
>
>     net/socket: Don't treat odd socket type as SOCK_STREAM
>
>     In net_socket_fd_init(), the 'default' case is odd: it warns, then
>     continues as if the socket type was SOCK_STREAM.  The comment
>     explains "this could be a eg. a pty", but that makes no sense.  If
>     @fd really was a pty, getsockopt() would fail with ENOTSOCK.  If @fd
>     was a socket, but neither SOCK_DGRAM nor SOCK_STREAM, it should not
>     be treated as if it was SOCK_STREAM.
>
>     Turn this case into an error.  If there is a genuine reason to
>     support something like SOCK_RAW, it should be handled explicitly.
>
> With something like that:


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

Thank you very much. :)
Mao

> Reviewed-by: Markus Armbruster <armbru@redhat.com>

[Qemu-devel] [PATCH v4 2/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 |  2 +-
 net/net.c              | 21 ++++++++++++++++-----
 net/socket.c           | 37 ++++++++++++++++++++++---------------
 3 files changed, 39 insertions(+), 21 deletions(-)

diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 5c326db..7abffc4 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -53,7 +53,7 @@ void socket_listen_cleanup(int fd, Error **errp);
 int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
 
 /* Old, ipv4 only bits.  Don't use for new code. */
-int parse_host_port(struct sockaddr_in *saddr, const char *str);
+int parse_host_port(struct sockaddr_in *saddr, const char *str, Error **errp);
 int socket_init(void);
 
 /**
diff --git a/net/net.c b/net/net.c
index 6235aab..e55869a 100644
--- a/net/net.c
+++ b/net/net.c
@@ -100,7 +100,7 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
     return 0;
 }
 
-int parse_host_port(struct sockaddr_in *saddr, const char *str)
+int parse_host_port(struct sockaddr_in *saddr, const char *str, Error **errp)
 {
     char buf[512];
     struct hostent *he;
@@ -108,24 +108,35 @@ int parse_host_port(struct sockaddr_in *saddr, const char *str)
     int port;
 
     p = str;
-    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0)
+    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
+        error_setg(errp, "The address should contain ':', for example: "
+                   "xxxx=230.0.0.1:1234");
         return -1;
+    }
     saddr->sin_family = AF_INET;
     if (buf[0] == '\0') {
         saddr->sin_addr.s_addr = 0;
     } else {
         if (qemu_isdigit(buf[0])) {
-            if (!inet_aton(buf, &saddr->sin_addr))
+            if (!inet_aton(buf, &saddr->sin_addr)) {
+                error_setg(errp, "Host address '%s' is not a valid "
+                           "IPv4 address", buf);
                 return -1;
+            }
         } else {
-            if ((he = gethostbyname(buf)) == NULL)
+            he = gethostbyname(buf);
+            if (he == NULL) {
+                error_setg(errp, "Specified hostname is error.");
                 return - 1;
+            }
             saddr->sin_addr = *(struct in_addr *)he->h_addr;
         }
     }
     port = strtol(p, (char **)&r, 0);
-    if (r == p)
+    if (r == p) {
+        error_setg(errp, "The port number is illegal");
         return -1;
+    }
     saddr->sin_port = htons(port);
     return 0;
 }
diff --git a/net/socket.c b/net/socket.c
index 53765bd..66016c8 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -485,15 +485,17 @@ static void net_socket_accept(void *opaque)
 static int net_socket_listen_init(NetClientState *peer,
                                   const char *model,
                                   const char *name,
-                                  const char *host_str)
+                                  const char *host_str,
+                                  Error **errp)
 {
     NetClientState *nc;
     NetSocketState *s;
     struct sockaddr_in saddr;
     int fd, ret;
 
-    if (parse_host_port(&saddr, host_str) < 0)
+    if (parse_host_port(&saddr, host_str, errp) < 0) {
         return -1;
+    }
 
     fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {
@@ -531,14 +533,16 @@ static int net_socket_listen_init(NetClientState *peer,
 static int net_socket_connect_init(NetClientState *peer,
                                    const char *model,
                                    const char *name,
-                                   const char *host_str)
+                                   const char *host_str,
+                                   Error **errp)
 {
     NetSocketState *s;
     int fd, connected, ret;
     struct sockaddr_in saddr;
 
-    if (parse_host_port(&saddr, host_str) < 0)
+    if (parse_host_port(&saddr, host_str, errp) < 0) {
         return -1;
+    }
 
     fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {
@@ -580,14 +584,15 @@ static int net_socket_mcast_init(NetClientState *peer,
                                  const char *model,
                                  const char *name,
                                  const char *host_str,
-                                 const char *localaddr_str)
+                                 const char *localaddr_str,
+                                 Error **errp)
 {
     NetSocketState *s;
     int fd;
     struct sockaddr_in saddr;
     struct in_addr localaddr, *param_localaddr;
 
-    if (parse_host_port(&saddr, host_str) < 0)
+    if (parse_host_port(&saddr, host_str, errp) < 0)
         return -1;
 
     if (localaddr_str != NULL) {
@@ -619,17 +624,18 @@ static int net_socket_udp_init(NetClientState *peer,
                                  const char *model,
                                  const char *name,
                                  const char *rhost,
-                                 const char *lhost)
+                                 const char *lhost,
+                                 Error **errp)
 {
     NetSocketState *s;
     int fd, ret;
     struct sockaddr_in laddr, raddr;
 
-    if (parse_host_port(&laddr, lhost) < 0) {
+    if (parse_host_port(&laddr, lhost, errp) < 0) {
         return -1;
     }
 
-    if (parse_host_port(&raddr, rhost) < 0) {
+    if (parse_host_port(&raddr, rhost, errp) < 0) {
         return -1;
     }
 
@@ -703,15 +709,16 @@ int net_init_socket(const Netdev *netdev, const char *name,
     }
 
     if (sock->has_listen) {
-        if (net_socket_listen_init(peer, "socket", name, sock->listen) == -1) {
+        if (net_socket_listen_init(peer, "socket", name, sock->listen,
+            errp) == -1) {
             return -1;
         }
         return 0;
     }
 
     if (sock->has_connect) {
-        if (net_socket_connect_init(peer, "socket", name, sock->connect) ==
-            -1) {
+        if (net_socket_connect_init(peer, "socket", name, sock->connect,
+            errp) == -1) {
             return -1;
         }
         return 0;
@@ -721,7 +728,7 @@ int net_init_socket(const Netdev *netdev, const char *name,
         /* if sock->localaddr is missing, it has been initialized to "all bits
          * zero" */
         if (net_socket_mcast_init(peer, "socket", name, sock->mcast,
-            sock->localaddr) == -1) {
+            sock->localaddr, errp) == -1) {
             return -1;
         }
         return 0;
@@ -732,8 +739,8 @@ int net_init_socket(const Netdev *netdev, const char *name,
         error_report("localaddr= is mandatory with udp=");
         return -1;
     }
-    if (net_socket_udp_init(peer, "socket", name, sock->udp, sock->localaddr) ==
-        -1) {
+    if (net_socket_udp_init(peer, "socket", name, sock->udp, sock->localaddr,
+        errp) == -1) {
         return -1;
     }
     return 0;
-- 
2.9.3

Re: [Qemu-devel] [PATCH v4 2/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 |  2 +-
>  net/net.c              | 21 ++++++++++++++++-----
>  net/socket.c           | 37 ++++++++++++++++++++++---------------
>  3 files changed, 39 insertions(+), 21 deletions(-)
>
> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> index 5c326db..7abffc4 100644
> --- a/include/qemu/sockets.h
> +++ b/include/qemu/sockets.h
> @@ -53,7 +53,7 @@ void socket_listen_cleanup(int fd, Error **errp);
>  int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
>  
>  /* Old, ipv4 only bits.  Don't use for new code. */
> -int parse_host_port(struct sockaddr_in *saddr, const char *str);
> +int parse_host_port(struct sockaddr_in *saddr, const char *str, Error **errp);
>  int socket_init(void);
>  
>  /**
> diff --git a/net/net.c b/net/net.c
> index 6235aab..e55869a 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -100,7 +100,7 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
>      return 0;
>  }
>  
> -int parse_host_port(struct sockaddr_in *saddr, const char *str)
> +int parse_host_port(struct sockaddr_in *saddr, const char *str, Error **errp)
>  {
>      char buf[512];
>      struct hostent *he;
> @@ -108,24 +108,35 @@ int parse_host_port(struct sockaddr_in *saddr, const char *str)
>      int port;
>  
>      p = str;
> -    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0)
> +    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
> +        error_setg(errp, "The address should contain ':', for example: "
> +                   "xxxx=230.0.0.1:1234");
>          return -1;
> +    }
>      saddr->sin_family = AF_INET;
>      if (buf[0] == '\0') {
>          saddr->sin_addr.s_addr = 0;
>      } else {
>          if (qemu_isdigit(buf[0])) {
> -            if (!inet_aton(buf, &saddr->sin_addr))
> +            if (!inet_aton(buf, &saddr->sin_addr)) {
> +                error_setg(errp, "Host address '%s' is not a valid "
> +                           "IPv4 address", buf);
>                  return -1;
> +            }
>          } else {
> -            if ((he = gethostbyname(buf)) == NULL)
> +            he = gethostbyname(buf);
> +            if (he == NULL) {
> +                error_setg(errp, "Specified hostname is error.");
>                  return - 1;
> +            }
>              saddr->sin_addr = *(struct in_addr *)he->h_addr;
>          }
>      }
>      port = strtol(p, (char **)&r, 0);
> -    if (r == p)
> +    if (r == p) {
> +        error_setg(errp, "The port number is illegal");
>          return -1;
> +    }
>      saddr->sin_port = htons(port);
>      return 0;
>  }
> diff --git a/net/socket.c b/net/socket.c
> index 53765bd..66016c8 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -485,15 +485,17 @@ static void net_socket_accept(void *opaque)
>  static int net_socket_listen_init(NetClientState *peer,
>                                    const char *model,
>                                    const char *name,
> -                                  const char *host_str)
> +                                  const char *host_str,
> +                                  Error **errp)

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

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

... here, ...

>  
>      fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
>      if (fd < 0) {
           perror("socket");
           return -1;
       }

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

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

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

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

Additionally:

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

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

> @@ -580,14 +584,15 @@ static int net_socket_mcast_init(NetClientState *peer,
>                                   const char *model,
>                                   const char *name,
>                                   const char *host_str,
> -                                 const char *localaddr_str)
> +                                 const char *localaddr_str,
> +                                 Error **errp)
>  {
>      NetSocketState *s;
>      int fd;
>      struct sockaddr_in saddr;
>      struct in_addr localaddr, *param_localaddr;
>  
> -    if (parse_host_port(&saddr, host_str) < 0)
> +    if (parse_host_port(&saddr, host_str, errp) < 0)
>          return -1;
>  
>      if (localaddr_str != NULL) {

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

Additionally, convert net_socket_mcast_create() to Error.

> @@ -619,17 +624,18 @@ static int net_socket_udp_init(NetClientState *peer,
>                                   const char *model,
>                                   const char *name,
>                                   const char *rhost,
> -                                 const char *lhost)
> +                                 const char *lhost,
> +                                 Error **errp)
>  {
>      NetSocketState *s;
>      int fd, ret;
>      struct sockaddr_in laddr, raddr;
>  
> -    if (parse_host_port(&laddr, lhost) < 0) {
> +    if (parse_host_port(&laddr, lhost, errp) < 0) {
>          return -1;
>      }
>  
> -    if (parse_host_port(&raddr, rhost) < 0) {
> +    if (parse_host_port(&raddr, rhost, errp) < 0) {
>          return -1;
>      }
>  

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

> @@ -703,15 +709,16 @@ int net_init_socket(const Netdev *netdev, const char *name,
>      }
>  
>      if (sock->has_listen) {
> -        if (net_socket_listen_init(peer, "socket", name, sock->listen) == -1) {
> +        if (net_socket_listen_init(peer, "socket", name, sock->listen,
> +            errp) == -1) {
>              return -1;
>          }
>          return 0;
>      }
>  
>      if (sock->has_connect) {
> -        if (net_socket_connect_init(peer, "socket", name, sock->connect) ==
> -            -1) {
> +        if (net_socket_connect_init(peer, "socket", name, sock->connect,
> +            errp) == -1) {
>              return -1;
>          }
>          return 0;
> @@ -721,7 +728,7 @@ int net_init_socket(const Netdev *netdev, const char *name,
>          /* if sock->localaddr is missing, it has been initialized to "all bits
>           * zero" */
>          if (net_socket_mcast_init(peer, "socket", name, sock->mcast,
> -            sock->localaddr) == -1) {
> +            sock->localaddr, errp) == -1) {
>              return -1;
>          }
>          return 0;
> @@ -732,8 +739,8 @@ int net_init_socket(const Netdev *netdev, const char *name,
>          error_report("localaddr= is mandatory with udp=");
>          return -1;
>      }
> -    if (net_socket_udp_init(peer, "socket", name, sock->udp, sock->localaddr) ==
> -        -1) {
> +    if (net_socket_udp_init(peer, "socket", name, sock->udp, sock->localaddr,
> +        errp) == -1) {
>          return -1;
>      }
>      return 0;

This is a partial fix for net_init_socket()'s
    /* FIXME error_setg(errp, ...) on failure */

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

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

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

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> 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 |  2 +-
>>  net/net.c              | 21 ++++++++++++++++-----
>>  net/socket.c           | 37 ++++++++++++++++++++++---------------
>>  3 files changed, 39 insertions(+), 21 deletions(-)
>>
>> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
>> index 5c326db..7abffc4 100644
>> --- a/include/qemu/sockets.h
>> +++ b/include/qemu/sockets.h
>> @@ -53,7 +53,7 @@ void socket_listen_cleanup(int fd, Error **errp);
>>  int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
>>  
>>  /* Old, ipv4 only bits.  Don't use for new code. */
>> -int parse_host_port(struct sockaddr_in *saddr, const char *str);
>> +int parse_host_port(struct sockaddr_in *saddr, const char *str, Error **errp);
>>  int socket_init(void);
>>  
>>  /**
>> diff --git a/net/net.c b/net/net.c
>> index 6235aab..e55869a 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -100,7 +100,7 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
>>      return 0;
>>  }
>>  
>> -int parse_host_port(struct sockaddr_in *saddr, const char *str)
>> +int parse_host_port(struct sockaddr_in *saddr, const char *str, Error **errp)
>>  {
>>      char buf[512];
>>      struct hostent *he;
>> @@ -108,24 +108,35 @@ int parse_host_port(struct sockaddr_in *saddr, const char *str)
>>      int port;
>>  
>>      p = str;
>> -    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0)
>> +    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
>> +        error_setg(errp, "The address should contain ':', for example: "
>> +                   "xxxx=230.0.0.1:1234");
>>          return -1;
>> +    }
>>      saddr->sin_family = AF_INET;
>>      if (buf[0] == '\0') {
>>          saddr->sin_addr.s_addr = 0;
>>      } else {
>>          if (qemu_isdigit(buf[0])) {
>> -            if (!inet_aton(buf, &saddr->sin_addr))
>> +            if (!inet_aton(buf, &saddr->sin_addr)) {
>> +                error_setg(errp, "Host address '%s' is not a valid "
>> +                           "IPv4 address", buf);
>>                  return -1;
>> +            }
>>          } else {
>> -            if ((he = gethostbyname(buf)) == NULL)
>> +            he = gethostbyname(buf);
>> +            if (he == NULL) {
>> +                error_setg(errp, "Specified hostname is error.");
>>                  return - 1;
>> +            }
>>              saddr->sin_addr = *(struct in_addr *)he->h_addr;
>>          }
>>      }
>>      port = strtol(p, (char **)&r, 0);
>> -    if (r == p)
>> +    if (r == p) {
>> +        error_setg(errp, "The port number is illegal");
>>          return -1;
>> +    }
>>      saddr->sin_port = htons(port);
>>      return 0;
>>  }
>> diff --git a/net/socket.c b/net/socket.c
>> index 53765bd..66016c8 100644
>> --- a/net/socket.c
>> +++ b/net/socket.c
>> @@ -485,15 +485,17 @@ static void net_socket_accept(void *opaque)
>>  static int net_socket_listen_init(NetClientState *peer,
>>                                    const char *model,
>>                                    const char *name,
>> -                                  const char *host_str)
>> +                                  const char *host_str,
>> +                                  Error **errp)
>
> You convert net_socket_listen_init() to Error.  This means you have to
> make it set an error on failure.  You do ...
>
>>  {
>>      NetClientState *nc;
>>      NetSocketState *s;
>>      struct sockaddr_in saddr;
>>      int fd, ret;
>>  
>> -    if (parse_host_port(&saddr, host_str) < 0)
>> +    if (parse_host_port(&saddr, host_str, errp) < 0) {
>>          return -1;
>> +    }
>
> ... here, ...
>
>>  
>>      fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
>>      if (fd < 0) {
>            perror("socket");
>            return -1;
>        }
>
> ... but not here.  Two more error returns follow.
>
> Preexisting bug: net_socket_listen_init() reports to stderr for three
> out of four failures.  Functions should either report on no failure or
> on all.  Conversion to Error should fix that, but yours isn't complete.
> When it is, the fix should be mentioned in the commit message.
>
>> @@ -531,14 +533,16 @@ static int net_socket_listen_init(NetClientState *peer,
>>  static int net_socket_connect_init(NetClientState *peer,
>>                                     const char *model,
>>                                     const char *name,
>> -                                   const char *host_str)
>> +                                   const char *host_str,
>> +                                   Error **errp)
>>  {
>>      NetSocketState *s;
>>      int fd, connected, ret;
>>      struct sockaddr_in saddr;
>>  
>> -    if (parse_host_port(&saddr, host_str) < 0)
>> +    if (parse_host_port(&saddr, host_str, errp) < 0) {
>>          return -1;
>> +    }
>>  
>>      fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
>>      if (fd < 0) {
>
> Again, you have to make the function set an error on all failures, not
> just this one.
>
> Additionally:
>
>        s = net_socket_fd_init(peer, model, name, fd, connected);
>        if (!s)
>            return -1;
>
> net_socket_fd_init() still prints to stderr.  Please convert it to
> Error.
>
>> @@ -580,14 +584,15 @@ static int net_socket_mcast_init(NetClientState *peer,
>>                                   const char *model,
>>                                   const char *name,
>>                                   const char *host_str,
>> -                                 const char *localaddr_str)
>> +                                 const char *localaddr_str,
>> +                                 Error **errp)
>>  {
>>      NetSocketState *s;
>>      int fd;
>>      struct sockaddr_in saddr;
>>      struct in_addr localaddr, *param_localaddr;
>>  
>> -    if (parse_host_port(&saddr, host_str) < 0)
>> +    if (parse_host_port(&saddr, host_str, errp) < 0)
>>          return -1;
>>  
>>      if (localaddr_str != NULL) {
>
> Again, you have to make the function set an error on all failures.
>
> Additionally, convert net_socket_mcast_create() to Error.

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

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

to

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

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

>> @@ -619,17 +624,18 @@ static int net_socket_udp_init(NetClientState *peer,
>>                                   const char *model,
>>                                   const char *name,
>>                                   const char *rhost,
>> -                                 const char *lhost)
>> +                                 const char *lhost,
>> +                                 Error **errp)
>>  {
>>      NetSocketState *s;
>>      int fd, ret;
>>      struct sockaddr_in laddr, raddr;
>>  
>> -    if (parse_host_port(&laddr, lhost) < 0) {
>> +    if (parse_host_port(&laddr, lhost, errp) < 0) {
>>          return -1;
>>      }
>>  
>> -    if (parse_host_port(&raddr, rhost) < 0) {
>> +    if (parse_host_port(&raddr, rhost, errp) < 0) {
>>          return -1;
>>      }
>>  
>
> Again, you have make the function set an error on all failures.
>
>> @@ -703,15 +709,16 @@ int net_init_socket(const Netdev *netdev, const char *name,
>>      }
>>  
>>      if (sock->has_listen) {
>> -        if (net_socket_listen_init(peer, "socket", name, sock->listen) == -1) {
>> +        if (net_socket_listen_init(peer, "socket", name, sock->listen,
>> +            errp) == -1) {
>>              return -1;
>>          }
>>          return 0;
>>      }
>>  
>>      if (sock->has_connect) {
>> -        if (net_socket_connect_init(peer, "socket", name, sock->connect) ==
>> -            -1) {
>> +        if (net_socket_connect_init(peer, "socket", name, sock->connect,
>> +            errp) == -1) {
>>              return -1;
>>          }
>>          return 0;
>> @@ -721,7 +728,7 @@ int net_init_socket(const Netdev *netdev, const char *name,
>>          /* if sock->localaddr is missing, it has been initialized to "all bits
>>           * zero" */
>>          if (net_socket_mcast_init(peer, "socket", name, sock->mcast,
>> -            sock->localaddr) == -1) {
>> +            sock->localaddr, errp) == -1) {
>>              return -1;
>>          }
>>          return 0;
>> @@ -732,8 +739,8 @@ int net_init_socket(const Netdev *netdev, const char *name,
>>          error_report("localaddr= is mandatory with udp=");
>>          return -1;
>>      }
>> -    if (net_socket_udp_init(peer, "socket", name, sock->udp, sock->localaddr) ==
>> -        -1) {
>> +    if (net_socket_udp_init(peer, "socket", name, sock->udp, sock->localaddr,
>> +        errp) == -1) {
>>          return -1;
>>      }
>>      return 0;
>
> This is a partial fix for net_init_socket()'s
>     /* FIXME error_setg(errp, ...) on failure */
>
> Can you fix it completely?  Not required to get the patch accepted, as
> far as I'm concerned.

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

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

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

[Mao Zhongyi]

Hi, Markus


On 06/23/2017 10:21 PM, Markus Armbruster wrote:
> Markus Armbruster <armbru@redhat.com> writes:
>
>> 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 |  2 +-
>>>  net/net.c              | 21 ++++++++++++++++-----
>>>  net/socket.c           | 37 ++++++++++++++++++++++---------------
>>>  3 files changed, 39 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
>>> index 5c326db..7abffc4 100644
>>> --- a/include/qemu/sockets.h
>>> +++ b/include/qemu/sockets.h
>>> @@ -53,7 +53,7 @@ void socket_listen_cleanup(int fd, Error **errp);
>>>  int socket_dgram(SocketAddress *remote, SocketAddress *local, Error **errp);
>>>
>>>  /* Old, ipv4 only bits.  Don't use for new code. */
>>> -int parse_host_port(struct sockaddr_in *saddr, const char *str);
>>> +int parse_host_port(struct sockaddr_in *saddr, const char *str, Error **errp);
>>>  int socket_init(void);
>>>
>>>  /**
>>> diff --git a/net/net.c b/net/net.c
>>> index 6235aab..e55869a 100644
>>> --- a/net/net.c
>>> +++ b/net/net.c
>>> @@ -100,7 +100,7 @@ static int get_str_sep(char *buf, int buf_size, const char **pp, int sep)
>>>      return 0;
>>>  }
>>>
>>> -int parse_host_port(struct sockaddr_in *saddr, const char *str)
>>> +int parse_host_port(struct sockaddr_in *saddr, const char *str, Error **errp)
>>>  {
>>>      char buf[512];
>>>      struct hostent *he;
>>> @@ -108,24 +108,35 @@ int parse_host_port(struct sockaddr_in *saddr, const char *str)
>>>      int port;
>>>
>>>      p = str;
>>> -    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0)
>>> +    if (get_str_sep(buf, sizeof(buf), &p, ':') < 0) {
>>> +        error_setg(errp, "The address should contain ':', for example: "
>>> +                   "xxxx=230.0.0.1:1234");
>>>          return -1;
>>> +    }
>>>      saddr->sin_family = AF_INET;
>>>      if (buf[0] == '\0') {
>>>          saddr->sin_addr.s_addr = 0;
>>>      } else {
>>>          if (qemu_isdigit(buf[0])) {
>>> -            if (!inet_aton(buf, &saddr->sin_addr))
>>> +            if (!inet_aton(buf, &saddr->sin_addr)) {
>>> +                error_setg(errp, "Host address '%s' is not a valid "
>>> +                           "IPv4 address", buf);
>>>                  return -1;
>>> +            }
>>>          } else {
>>> -            if ((he = gethostbyname(buf)) == NULL)
>>> +            he = gethostbyname(buf);
>>> +            if (he == NULL) {
>>> +                error_setg(errp, "Specified hostname is error.");
>>>                  return - 1;
>>> +            }
>>>              saddr->sin_addr = *(struct in_addr *)he->h_addr;
>>>          }
>>>      }
>>>      port = strtol(p, (char **)&r, 0);
>>> -    if (r == p)
>>> +    if (r == p) {
>>> +        error_setg(errp, "The port number is illegal");
>>>          return -1;
>>> +    }
>>>      saddr->sin_port = htons(port);
>>>      return 0;
>>>  }
>>> diff --git a/net/socket.c b/net/socket.c
>>> index 53765bd..66016c8 100644
>>> --- a/net/socket.c
>>> +++ b/net/socket.c
>>> @@ -485,15 +485,17 @@ static void net_socket_accept(void *opaque)
>>>  static int net_socket_listen_init(NetClientState *peer,
>>>                                    const char *model,
>>>                                    const char *name,
>>> -                                  const char *host_str)
>>> +                                  const char *host_str,
>>> +                                  Error **errp)
>>
>> You convert net_socket_listen_init() to Error.  This means you have to
>> make it set an error on failure.  You do ...
>>
>>>  {
>>>      NetClientState *nc;
>>>      NetSocketState *s;
>>>      struct sockaddr_in saddr;
>>>      int fd, ret;
>>>
>>> -    if (parse_host_port(&saddr, host_str) < 0)
>>> +    if (parse_host_port(&saddr, host_str, errp) < 0) {
>>>          return -1;
>>> +    }
>>
>> ... here, ...
>>
>>>
>>>      fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
>>>      if (fd < 0) {
>>            perror("socket");
>>            return -1;
>>        }
>>
>> ... but not here.  Two more error returns follow.
>>
>> Preexisting bug: net_socket_listen_init() reports to stderr for three
>> out of four failures.  Functions should either report on no failure or
>> on all.  Conversion to Error should fix that, but yours isn't complete.
>> When it is, the fix should be mentioned in the commit message.
>>
>>> @@ -531,14 +533,16 @@ static int net_socket_listen_init(NetClientState *peer,
>>>  static int net_socket_connect_init(NetClientState *peer,
>>>                                     const char *model,
>>>                                     const char *name,
>>> -                                   const char *host_str)
>>> +                                   const char *host_str,
>>> +                                   Error **errp)
>>>  {
>>>      NetSocketState *s;
>>>      int fd, connected, ret;
>>>      struct sockaddr_in saddr;
>>>
>>> -    if (parse_host_port(&saddr, host_str) < 0)
>>> +    if (parse_host_port(&saddr, host_str, errp) < 0) {
>>>          return -1;
>>> +    }
>>>
>>>      fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
>>>      if (fd < 0) {
>>
>> Again, you have to make the function set an error on all failures, not
>> just this one.
>>
>> Additionally:
>>
>>        s = net_socket_fd_init(peer, model, name, fd, connected);
>>        if (!s)
>>            return -1;
>>
>> net_socket_fd_init() still prints to stderr.  Please convert it to
>> Error.
>>
>>> @@ -580,14 +584,15 @@ static int net_socket_mcast_init(NetClientState *peer,
>>>                                   const char *model,
>>>                                   const char *name,
>>>                                   const char *host_str,
>>> -                                 const char *localaddr_str)
>>> +                                 const char *localaddr_str,
>>> +                                 Error **errp)
>>>  {
>>>      NetSocketState *s;
>>>      int fd;
>>>      struct sockaddr_in saddr;
>>>      struct in_addr localaddr, *param_localaddr;
>>>
>>> -    if (parse_host_port(&saddr, host_str) < 0)
>>> +    if (parse_host_port(&saddr, host_str, errp) < 0)
>>>          return -1;
>>>
>>>      if (localaddr_str != NULL) {
>>
>> Again, you have to make the function set an error on all failures.
>>
>> Additionally, convert net_socket_mcast_create() to Error.
>
> Actually, you do in the next patch.  So, you first do an incorrect
> conversion in PATCH 2, then fix it in PATCH 3.  Not horrible, but
> avoiding the broken intermediate state is easy enough: convert
> net_socket_mcast_create() to Error before PATCH 2, changing its callers
> from
>
>     fd = net_socket_mcast_create(&saddr, param_localaddr);
>     if (fd < 0) {
>         // error already reported
>         take the error exit
>     }
>
> to
>
>     fd = net_socket_mcast_create(&saddr, param_localaddr, &err);
>     if (fd < 0) {
>         error_report_err(err);
>         take the error exit
>     }
>
> The error_report_err() goes away when the function containing it gets
> converted to Error.  Any questions?


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

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

Thank you very much.
Mao


>
>>> @@ -619,17 +624,18 @@ static int net_socket_udp_init(NetClientState *peer,
>>>                                   const char *model,
>>>                                   const char *name,
>>>                                   const char *rhost,
>>> -                                 const char *lhost)
>>> +                                 const char *lhost,
>>> +                                 Error **errp)
>>>  {
>>>      NetSocketState *s;
>>>      int fd, ret;
>>>      struct sockaddr_in laddr, raddr;
>>>
>>> -    if (parse_host_port(&laddr, lhost) < 0) {
>>> +    if (parse_host_port(&laddr, lhost, errp) < 0) {
>>>          return -1;
>>>      }
>>>
>>> -    if (parse_host_port(&raddr, rhost) < 0) {
>>> +    if (parse_host_port(&raddr, rhost, errp) < 0) {
>>>          return -1;
>>>      }
>>>
>>
>> Again, you have make the function set an error on all failures.
>>
>>> @@ -703,15 +709,16 @@ int net_init_socket(const Netdev *netdev, const char *name,
>>>      }
>>>
>>>      if (sock->has_listen) {
>>> -        if (net_socket_listen_init(peer, "socket", name, sock->listen) == -1) {
>>> +        if (net_socket_listen_init(peer, "socket", name, sock->listen,
>>> +            errp) == -1) {
>>>              return -1;
>>>          }
>>>          return 0;
>>>      }
>>>
>>>      if (sock->has_connect) {
>>> -        if (net_socket_connect_init(peer, "socket", name, sock->connect) ==
>>> -            -1) {
>>> +        if (net_socket_connect_init(peer, "socket", name, sock->connect,
>>> +            errp) == -1) {
>>>              return -1;
>>>          }
>>>          return 0;
>>> @@ -721,7 +728,7 @@ int net_init_socket(const Netdev *netdev, const char *name,
>>>          /* if sock->localaddr is missing, it has been initialized to "all bits
>>>           * zero" */
>>>          if (net_socket_mcast_init(peer, "socket", name, sock->mcast,
>>> -            sock->localaddr) == -1) {
>>> +            sock->localaddr, errp) == -1) {
>>>              return -1;
>>>          }
>>>          return 0;
>>> @@ -732,8 +739,8 @@ int net_init_socket(const Netdev *netdev, const char *name,
>>>          error_report("localaddr= is mandatory with udp=");
>>>          return -1;
>>>      }
>>> -    if (net_socket_udp_init(peer, "socket", name, sock->udp, sock->localaddr) ==
>>> -        -1) {
>>> +    if (net_socket_udp_init(peer, "socket", name, sock->udp, sock->localaddr,
>>> +        errp) == -1) {
>>>          return -1;
>>>      }
>>>      return 0;
>>
>> This is a partial fix for net_init_socket()'s
>>     /* FIXME error_setg(errp, ...) on failure */
>>
>> Can you fix it completely?  Not required to get the patch accepted, as
>> far as I'm concerned.
>
> Ah, you do in PATCH 4.  Suggest to explain in your commit message that
> this is a partial fix, which will be completed in the following
> commit(s).
>
>> Your commit message claims "Convert parse_host_port() to Error".  You do
>> more than that.  Please rephrase the commit message.
>
>
>

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

[Mao Zhongyi]

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

Now, convert these functions to Error.

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

diff --git a/net/socket.c b/net/socket.c
index 66016c8..9825422 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;
@@ -342,7 +351,7 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
                 goto err;
             }
             /* clone dgram socket */
-            newfd = net_socket_mcast_create(&saddr, NULL);
+            newfd = net_socket_mcast_create(&saddr, NULL, errp);
             if (newfd < 0) {
                 /* error already reported by net_socket_mcast_create() */
                 goto err;
@@ -352,8 +361,8 @@ static NetSocketState *net_socket_fd_init_dgram(NetClientState *peer,
             close(newfd);
 
         } else {
-            fprintf(stderr,
-                    "qemu: error: init_dgram: fd=%d failed getsockname(): %s\n",
+            error_setg(errp,
+                    "qemu: error: init_dgram: fd=%d failed getsockname(): %s",
                     fd, strerror(errno));
             goto err;
         }
@@ -432,25 +441,27 @@ static NetSocketState *net_socket_fd_init_stream(NetClientState *peer,
 
 static NetSocketState *net_socket_fd_init(NetClientState *peer,
                                           const char *model, const char *name,
-                                          int fd, int is_connected)
+                                          int fd, int is_connected,
+                                          Error **errp)
 {
     int so_type = -1, optlen=sizeof(so_type);
 
     if(getsockopt(fd, SOL_SOCKET, SO_TYPE, (char *)&so_type,
         (socklen_t *)&optlen)< 0) {
-        fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n",
+        error_setg(errp, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed",
                 fd);
         closesocket(fd);
         return NULL;
     }
     switch(so_type) {
     case SOCK_DGRAM:
-        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected);
+        return net_socket_fd_init_dgram(peer, model, name, fd, is_connected,
+            errp);
     case SOCK_STREAM:
         return net_socket_fd_init_stream(peer, model, name, fd, is_connected);
     default:
-        error_report("qemu: error: socket type=%d for fd=%d is not"
-                " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
+        error_setg(errp, "qemu: error: socket type=%d for fd=%d is not"
+              " SOCK_DGRAM or SOCK_STREAM", so_type, fd);
         closesocket(fd);
     }
     return NULL;
@@ -571,7 +582,7 @@ static int net_socket_connect_init(NetClientState *peer,
             break;
         }
     }
-    s = net_socket_fd_init(peer, model, name, fd, connected);
+    s = net_socket_fd_init(peer, model, name, fd, connected, errp);
     if (!s)
         return -1;
     snprintf(s->nc.info_str, sizeof(s->nc.info_str),
@@ -603,11 +614,11 @@ static int net_socket_mcast_init(NetClientState *peer,
         param_localaddr = NULL;
     }
 
-    fd = net_socket_mcast_create(&saddr, param_localaddr);
+    fd = net_socket_mcast_create(&saddr, param_localaddr, errp);
     if (fd < 0)
         return -1;
 
-    s = net_socket_fd_init(peer, model, name, fd, 0);
+    s = net_socket_fd_init(peer, model, name, fd, 0, errp);
     if (!s)
         return -1;
 
@@ -658,7 +669,7 @@ static int net_socket_udp_init(NetClientState *peer,
     }
     qemu_set_nonblock(fd);
 
-    s = net_socket_fd_init(peer, model, name, fd, 0);
+    s = net_socket_fd_init(peer, model, name, fd, 0, errp);
     if (!s) {
         return -1;
     }
@@ -702,7 +713,7 @@ int net_init_socket(const Netdev *netdev, const char *name,
             return -1;
         }
         qemu_set_nonblock(fd);
-        if (!net_socket_fd_init(peer, "socket", name, fd, 1)) {
+        if (!net_socket_fd_init(peer, "socket", name, fd, 1, errp)) {
             return -1;
         }
         return 0;
-- 
2.9.3

[Qemu-devel] [PATCH v4 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_init_socket() to Error to get rid of the superfluous second
error message. After the patch, the effect like this:

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

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

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

diff --git a/net/socket.c b/net/socket.c
index 9825422..a8f2d7f 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -510,7 +510,8 @@ static int net_socket_listen_init(NetClientState *peer,
 
     fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {
-        perror("socket");
+        error_setg_errno(errp, errno, "Create socket(PF_INET,"
+                         " SOCK_STREAM) failed");
         return -1;
     }
     qemu_set_nonblock(fd);
@@ -519,13 +520,14 @@ static int net_socket_listen_init(NetClientState *peer,
 
     ret = bind(fd, (struct sockaddr *)&saddr, sizeof(saddr));
     if (ret < 0) {
-        perror("bind");
+        error_setg_errno(errp, errno, "Bind ip=%s to fd=%d failed",
+                         inet_ntoa(saddr.sin_addr), fd);
         closesocket(fd);
         return -1;
     }
     ret = listen(fd, 0);
     if (ret < 0) {
-        perror("listen");
+        error_setg_errno(errp, errno, "Listen socket fd=%d failed", fd);
         closesocket(fd);
         return -1;
     }
@@ -557,7 +559,8 @@ static int net_socket_connect_init(NetClientState *peer,
 
     fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
     if (fd < 0) {
-        perror("socket");
+        error_setg_errno(errp, errno, "Create socket(PF_INET,"
+                         " SOCK_STREAM) failed");
         return -1;
     }
     qemu_set_nonblock(fd);
@@ -573,7 +576,7 @@ static int net_socket_connect_init(NetClientState *peer,
                        errno == EINVAL) {
                 break;
             } else {
-                perror("connect");
+                error_setg_errno(errp, errno, "Connection failed");
                 closesocket(fd);
                 return -1;
             }
@@ -607,8 +610,11 @@ static int net_socket_mcast_init(NetClientState *peer,
         return -1;
 
     if (localaddr_str != NULL) {
-        if (inet_aton(localaddr_str, &localaddr) == 0)
+        if (inet_aton(localaddr_str, &localaddr) == 0) {
+            error_setg(errp, "localaddr '%s' is not a valid IPv4 address",
+                       localaddr_str);
             return -1;
+        }
         param_localaddr = &localaddr;
     } else {
         param_localaddr = NULL;
@@ -652,18 +658,22 @@ static int net_socket_udp_init(NetClientState *peer,
 
     fd = qemu_socket(PF_INET, SOCK_DGRAM, 0);
     if (fd < 0) {
-        perror("socket(PF_INET, SOCK_DGRAM)");
+        error_setg_errno(errp, errno, "Create socket(PF_INET,"
+                         " SOCK_DGRAM) failed");
         return -1;
     }
 
     ret = socket_set_fast_reuse(fd);
     if (ret < 0) {
+        error_setg_errno(errp, errno, "Set the socket fd=%d attribute"
+                   " SO_REUSEADDR failed", fd);
         closesocket(fd);
         return -1;
     }
     ret = bind(fd, (struct sockaddr *)&laddr, sizeof(laddr));
     if (ret < 0) {
-        perror("bind");
+        error_setg_errno(errp, errno, "Bind ip=%s to fd=%d failed",
+                         inet_ntoa(laddr.sin_addr), fd);
         closesocket(fd);
         return -1;
     }
@@ -685,8 +695,6 @@ static int net_socket_udp_init(NetClientState *peer,
 int net_init_socket(const Netdev *netdev, const char *name,
                     NetClientState *peer, Error **errp)
 {
-    /* FIXME error_setg(errp, ...) on failure */
-    Error *err = NULL;
     const NetdevSocketOptions *sock;
 
     assert(netdev->type == NET_CLIENT_DRIVER_SOCKET);
@@ -694,22 +702,21 @@ int net_init_socket(const Netdev *netdev, const char *name,
 
     if (sock->has_fd + sock->has_listen + sock->has_connect + sock->has_mcast +
         sock->has_udp != 1) {
-        error_report("exactly one of fd=, listen=, connect=, mcast= or udp="
+        error_setg(errp, "exactly one of fd=, listen=, connect=, mcast= or udp="
                      " is required");
         return -1;
     }
 
     if (sock->has_localaddr && !sock->has_mcast && !sock->has_udp) {
-        error_report("localaddr= is only valid with mcast= or udp=");
+        error_setg(errp, "localaddr= is only valid with mcast= or udp=");
         return -1;
     }
 
     if (sock->has_fd) {
         int fd;
 
-        fd = monitor_fd_param(cur_mon, sock->fd, &err);
+        fd = monitor_fd_param(cur_mon, sock->fd, errp);
         if (fd == -1) {
-            error_report_err(err);
             return -1;
         }
         qemu_set_nonblock(fd);
@@ -747,7 +754,7 @@ int net_init_socket(const Netdev *netdev, const char *name,
 
     assert(sock->has_udp);
     if (!sock->has_localaddr) {
-        error_report("localaddr= is mandatory with udp=");
+        error_setg(errp, "localaddr= is mandatory with udp=");
         return -1;
     }
     if (net_socket_udp_init(peer, "socket", name, sock->udp, sock->localaddr,
-- 
2.9.3