Re: [Qemu-devel] [PATCH v2] Change net/socket.c to use socket_*() functions

[Paolo Bonzini]

On 18/06/2016 09:54, Ashijeet Acharya wrote:
> Use socket_*() functions from include/qemu/sockets.h instead of
> listen()/bind()/ connect()/parse_host_port(). socket_*() fucntions are
> QAPI based and this patch performs this api conversion since everything
> will be using QAPI based sockets in the future. Also add a helper
> function socket_address_to_string() in util/qemu-sockets.c which returns
> the string representation of socket address. Thetask was listed on
> http://wiki.qemu.org/BiteSizedTasks page.
> 
> Signed-off-by: Ashijeet Acharya <address@hidden>

Reviewed-by: Paolo Bonzini <address@hidden>

Jason, are you going to take this through the net tree?

Thanks,

Paolo

> ---
>  include/qemu/sockets.h | 16 ++++++++++++++-
>  net/socket.c           | 55 
> +++++++++++++++++++++++++-------------------------
>  util/qemu-sockets.c    | 36 +++++++++++++++++++++++++++++++++
>  3 files changed, 78 insertions(+), 29 deletions(-)
> 
> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> index 1bd9218..3a1a887 100644
> --- a/include/qemu/sockets.h
> +++ b/include/qemu/sockets.h
> @@ -110,4 +110,18 @@ SocketAddress *socket_remote_address(int fd, Error 
> **errp);
>  void qapi_copy_SocketAddress(SocketAddress **p_dest,
>                               SocketAddress *src);
>  
> -#endif /* QEMU_SOCKET_H */
> +/**
> + * socket_address_to_string:
> + * @addr: the socket address struct
> + * @errp: pointer to uninitialized error object
> + *
> + * Get the string representation of the socket
> + * address. A pointer to the char array containing
> + * string format will be returned, the caller is
> + * required to release the returned value when no
> + * longer required with g_free.
> + *
> + * Returns: the socket address in string format, or NULL on error
> + */
> +char *socket_address_to_string(struct SocketAddress *addr, Error **errp);
> +#endif /* QEMU_SOCKET_H */
> \ No newline at end of file
> diff --git a/net/socket.c b/net/socket.c
> index 333fb9e..ae6f921 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -489,41 +489,30 @@ static int net_socket_listen_init(NetClientState *peer,
>  {
>      NetClientState *nc;
>      NetSocketState *s;
> -    struct sockaddr_in saddr;
> -    int fd, ret;
> +    SocketAddress *saddr;
> +    int ret;
> +    Error *local_error = NULL;
>  
> -    if (parse_host_port(&saddr, host_str) < 0)
> -        return -1;
> -
> -    fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
> -    if (fd < 0) {
> -        perror("socket");
> +    saddr = socket_parse(host_str, &local_error);
> +    if (saddr == NULL) {
> +        error_report_err(local_error);
>          return -1;
>      }
> -    qemu_set_nonblock(fd);
>  
> -    socket_set_fast_reuse(fd);
> -
> -    ret = bind(fd, (struct sockaddr *)&saddr, sizeof(saddr));
> +    ret = socket_listen(saddr, &local_error);
>      if (ret < 0) {
> -        perror("bind");
> -        closesocket(fd);
> -        return -1;
> -    }
> -    ret = listen(fd, 0);
> -    if (ret < 0) {
> -        perror("listen");
> -        closesocket(fd);
> +        error_report_err(local_error);
>          return -1;
>      }
>  
>      nc = qemu_new_net_client(&net_socket_info, peer, model, name);
>      s = DO_UPCAST(NetSocketState, nc, nc);
>      s->fd = -1;
> -    s->listen_fd = fd;
> +    s->listen_fd = ret;
>      s->nc.link_down = true;
>  
>      qemu_set_fd_handler(s->listen_fd, net_socket_accept, NULL, s);
> +    qapi_free_SocketAddress(saddr);
>      return 0;
>  }
>  
> @@ -534,10 +523,15 @@ static int net_socket_connect_init(NetClientState *peer,
>  {
>      NetSocketState *s;
>      int fd, connected, ret;
> -    struct sockaddr_in saddr;
> +    char *addr_str;
> +    SocketAddress *saddr;
> +    Error *local_error = NULL;
>  
> -    if (parse_host_port(&saddr, host_str) < 0)
> +    saddr = socket_parse(host_str, &local_error);
> +    if (saddr == NULL) {
> +        error_report_err(local_error);
>          return -1;
> +    }
>  
>      fd = qemu_socket(PF_INET, SOCK_STREAM, 0);
>      if (fd < 0) {
> @@ -545,10 +539,9 @@ static int net_socket_connect_init(NetClientState *peer,
>          return -1;
>      }
>      qemu_set_nonblock(fd);
> -
>      connected = 0;
>      for(;;) {
> -        ret = connect(fd, (struct sockaddr *)&saddr, sizeof(saddr));
> +        ret = socket_connect(saddr, &local_error, NULL, NULL);
>          if (ret < 0) {
>              if (errno == EINTR || errno == EWOULDBLOCK) {
>                  /* continue */
> @@ -557,7 +550,7 @@ static int net_socket_connect_init(NetClientState *peer,
>                         errno == EINVAL) {
>                  break;
>              } else {
> -                perror("connect");
> +                error_report_err(local_error);
>                  closesocket(fd);
>                  return -1;
>              }
> @@ -569,9 +562,15 @@ static int net_socket_connect_init(NetClientState *peer,
>      s = net_socket_fd_init(peer, model, name, fd, connected);
>      if (!s)
>          return -1;
> +
> +    addr_str = socket_address_to_string(saddr, &local_error);
> +    if (addr_str == NULL)
> +        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));
> +             "socket: connect to %s", addr_str);
> +    qapi_free_SocketAddress(saddr);
> +    g_free(addr_str);
>      return 0;
>  }
>  
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 0d6cd1f..771b7db 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1151,3 +1151,39 @@ void qapi_copy_SocketAddress(SocketAddress **p_dest,
>      qmp_input_visitor_cleanup(qiv);
>      qobject_decref(obj);
>  }
> +
> +char *socket_address_to_string(struct SocketAddress *addr, Error **errp)
> +{
> +    char *buf;
> +    InetSocketAddress *inet;
> +    char host_port[INET6_ADDRSTRLEN + 5 + 4];
> +
> +    switch (addr->type) {
> +    case SOCKET_ADDRESS_KIND_INET:
> +        inet = addr->u.inet.data;
> +        if (strchr(inet->host, ':') == NULL) {
> +            snprintf(host_port, sizeof(host_port), "%s:%s", inet->host,
> +                    inet->port);
> +            buf = g_strdup(host_port);
> +        } else {
> +            snprintf(host_port, sizeof(host_port), "[%s]:%s", inet->host,
> +                    inet->port);
> +            buf = g_strdup(host_port);
> +        }
> +        break;
> +
> +    case SOCKET_ADDRESS_KIND_UNIX:
> +        buf = g_strdup(addr->u.q_unix.data->path);
> +        break;
> +
> +    case SOCKET_ADDRESS_KIND_FD:
> +        buf = g_strdup(addr->u.fd.data->str);
> +        break;
> +
> +    default:
> +        error_setg(errp, "socket family %d unsupported",
> +                   addr->type);
> +        return NULL;
> +    }
> +    return buf;
> +}
>