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

[Markus Armbruster]

Mao Zhongyi <address@hidden> writes:

> Cc: address@hidden
> Cc: address@hidden
> Cc: address@hidden
> Cc: address@hidden
> Cc: address@hidden
> Signed-off-by: Mao Zhongyi <address@hidden>
> ---
>  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.