Re: [PATCH v9 09/16] net: dgram: make dgram_dst generic

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 9758 bytes --]

On Mon, Sep 26, 2022 at 09:50:41PM +0200, Laurent Vivier wrote:
> dgram_dst is a sockaddr_in structure. To be able to use it with
> unix socket, use a pointer to a generic sockaddr structure.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> Reviewed-by: Stefano Brivio <sbrivio@redhat.com>

LGTM as far as it goes, but.. how can one actually use this pointer as
a generic sockaddr without also storing a socklen_t somewhere?

> ---
>  net/dgram.c | 73 ++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 44 insertions(+), 29 deletions(-)
> 
> diff --git a/net/dgram.c b/net/dgram.c
> index 45d869efc844..57933bfbd1c1 100644
> --- a/net/dgram.c
> +++ b/net/dgram.c
> @@ -39,7 +39,7 @@ typedef struct NetDgramState {
>      NetClientState nc;
>      int fd;
>      SocketReadState rs;
> -    struct sockaddr_in dgram_dst; /* contains destination iff connectionless */
> +    struct sockaddr *dgram_dst; /* contains destination iff connectionless */
>      bool read_poll;               /* waiting to receive data? */
>      bool write_poll;              /* waiting to transmit data? */
>  } NetDgramState;
> @@ -83,10 +83,9 @@ static ssize_t net_dgram_receive_dgram(NetClientState *nc,
>      ssize_t ret;
>  
>      do {
> -        if (s->dgram_dst.sin_family != AF_UNIX) {
> -            ret = sendto(s->fd, buf, size, 0,
> -                         (struct sockaddr *)&s->dgram_dst,
> -                         sizeof(s->dgram_dst));
> +        if (s->dgram_dst) {
> +            ret = sendto(s->fd, buf, size, 0, s->dgram_dst,
> +                         sizeof(struct sockaddr_in));
>          } else {
>              ret = send(s->fd, buf, size, 0);
>          }
> @@ -243,6 +242,8 @@ static void net_dgram_cleanup(NetClientState *nc)
>          close(s->fd);
>          s->fd = -1;
>      }
> +    g_free(s->dgram_dst);
> +    s->dgram_dst = NULL;
>  }
>  
>  static NetClientInfo net_dgram_socket_info = {
> @@ -259,7 +260,7 @@ static NetDgramState *net_dgram_fd_init_dgram(NetClientState *peer,
>                                                SocketAddress *mcast,
>                                                Error **errp)
>  {
> -    struct sockaddr_in saddr;
> +    struct sockaddr_in *saddr = NULL;
>      int newfd;
>      NetClientState *nc;
>      NetDgramState *s;
> @@ -281,24 +282,25 @@ static NetDgramState *net_dgram_fd_init_dgram(NetClientState *peer,
>       */
>  
>      if (is_fd && mcast != NULL) {
> -            if (convert_host_port(&saddr, mcast->u.inet.host,
> -                                  mcast->u.inet.port, errp) < 0) {
> +            saddr = g_new(struct sockaddr_in, 1);
> +
> +            if (convert_host_port(saddr, mcast->u.inet.host, mcast->u.inet.port,
> +                                  errp) < 0) {
>                  goto err;
>              }
>              /* must be bound */
> -            if (saddr.sin_addr.s_addr == 0) {
> +            if (saddr->sin_addr.s_addr == 0) {
>                  error_setg(errp, "can't setup multicast destination address");
>                  goto err;
>              }
>              /* clone dgram socket */
> -            newfd = net_dgram_mcast_create(&saddr, NULL, errp);
> +            newfd = net_dgram_mcast_create(saddr, NULL, errp);
>              if (newfd < 0) {
>                  goto err;
>              }
>              /* clone newfd to fd, close newfd */
>              dup2(newfd, fd);
>              close(newfd);
> -
>      }
>  
>      nc = qemu_new_net_client(&net_dgram_socket_info, peer, model, name);
> @@ -310,16 +312,13 @@ static NetDgramState *net_dgram_fd_init_dgram(NetClientState *peer,
>      net_dgram_read_poll(s, true);
>  
>      /* mcast: save bound address as dst */
> -    if (is_fd && mcast != NULL) {
> -        s->dgram_dst = saddr;
> +    if (saddr) {
> +        g_assert(s->dgram_dst == NULL);
> +        s->dgram_dst = (struct sockaddr *)saddr;
>          snprintf(nc->info_str, sizeof(nc->info_str),
>                   "fd=%d (cloned mcast=%s:%d)",
> -                 fd, inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));
> +                 fd, inet_ntoa(saddr->sin_addr), ntohs(saddr->sin_port));
>      } else {
> -        if (sa_type == SOCKET_ADDRESS_TYPE_UNIX) {
> -            s->dgram_dst.sin_family = AF_UNIX;
> -        }
> -
>          snprintf(nc->info_str, sizeof(nc->info_str), "fd=%d %s", fd,
>                   SocketAddressType_str(sa_type));
>      }
> @@ -327,6 +326,7 @@ static NetDgramState *net_dgram_fd_init_dgram(NetClientState *peer,
>      return s;
>  
>  err:
> +    g_free(saddr);
>      closesocket(fd);
>      return NULL;
>  }
> @@ -340,21 +340,24 @@ static int net_dgram_mcast_init(NetClientState *peer,
>  {
>      NetDgramState *s;
>      int fd, ret;
> -    struct sockaddr_in saddr;
> +    struct sockaddr_in *saddr;
>  
>      if (remote->type != SOCKET_ADDRESS_TYPE_INET) {
>          error_setg(errp, "multicast only support inet type");
>          return -1;
>      }
>  
> -    if (convert_host_port(&saddr, remote->u.inet.host, remote->u.inet.port,
> +    saddr = g_new(struct sockaddr_in, 1);
> +    if (convert_host_port(saddr, remote->u.inet.host, remote->u.inet.port,
>                            errp) < 0) {
> +        g_free(saddr);
>          return -1;
>      }
>  
>      if (!local) {
> -        fd = net_dgram_mcast_create(&saddr, NULL, errp);
> +        fd = net_dgram_mcast_create(saddr, NULL, errp);
>          if (fd < 0) {
> +            g_free(saddr);
>              return -1;
>          }
>      } else {
> @@ -363,13 +366,15 @@ static int net_dgram_mcast_init(NetClientState *peer,
>              struct in_addr localaddr;
>  
>              if (inet_aton(local->u.inet.host, &localaddr) == 0) {
> +                g_free(saddr);
>                  error_setg(errp, "localaddr '%s' is not a valid IPv4 address",
>                             local->u.inet.host);
>                  return -1;
>              }
>  
> -            fd = net_dgram_mcast_create(&saddr, &localaddr, errp);
> +            fd = net_dgram_mcast_create(saddr, &localaddr, errp);
>              if (fd < 0) {
> +                g_free(saddr);
>                  return -1;
>              }
>              break;
> @@ -377,16 +382,19 @@ static int net_dgram_mcast_init(NetClientState *peer,
>          case SOCKET_ADDRESS_TYPE_FD:
>              fd = monitor_fd_param(monitor_cur(), local->u.fd.str, errp);
>              if (fd == -1) {
> +                g_free(saddr);
>                  return -1;
>              }
>              ret = qemu_socket_try_set_nonblock(fd);
>              if (ret < 0) {
> +                g_free(saddr);
>                  error_setg_errno(errp, -ret, "%s: Can't use file descriptor %d",
>                                   name, fd);
>                  return -1;
>              }
>              break;
>          default:
> +            g_free(saddr);
>              error_setg(errp, "only support inet or fd type for local");
>              return -1;
>          }
> @@ -396,13 +404,16 @@ static int net_dgram_mcast_init(NetClientState *peer,
>                                   local->type == SOCKET_ADDRESS_TYPE_FD,
>                                   remote, errp);
>      if (!s) {
> +        g_free(saddr);
>          return -1;
>      }
>  
> -    s->dgram_dst = saddr;
> +    g_assert(s->dgram_dst == NULL);
> +    s->dgram_dst = (struct sockaddr *)saddr;
>  
>      snprintf(s->nc.info_str, sizeof(s->nc.info_str), "mcast=%s:%d",
> -             inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port));
> +             inet_ntoa(saddr->sin_addr), ntohs(saddr->sin_port));
> +
>      return 0;
>  
>  }
> @@ -416,8 +427,8 @@ static int net_dgram_init(NetClientState *peer,
>  {
>      NetDgramState *s;
>      int fd, ret;
> -    struct sockaddr_in raddr_in;
>      gchar *info_str;
> +    struct sockaddr *dgram_dst;
>  
>      /* detect multicast address */
>      if (remote && remote->type == SOCKET_ADDRESS_TYPE_INET) {
> @@ -458,7 +469,7 @@ static int net_dgram_init(NetClientState *peer,
>  
>      switch (local->type) {
>      case SOCKET_ADDRESS_TYPE_INET: {
> -        struct sockaddr_in laddr_in;
> +        struct sockaddr_in laddr_in, raddr_in;
>  
>          if (convert_host_port(&laddr_in, local->u.inet.host, local->u.inet.port,
>                                errp) < 0) {
> @@ -492,9 +503,12 @@ static int net_dgram_init(NetClientState *peer,
>          }
>          qemu_socket_set_nonblock(fd);
>  
> +        dgram_dst = g_malloc(sizeof(raddr_in));
> +        memcpy(dgram_dst, &raddr_in, sizeof(raddr_in));
> +
>          info_str = g_strdup_printf("udp=%s:%d/%s:%d",
> -                 inet_ntoa(laddr_in.sin_addr), ntohs(laddr_in.sin_port),
> -                 inet_ntoa(raddr_in.sin_addr), ntohs(raddr_in.sin_port));
> +                        inet_ntoa(laddr_in.sin_addr), ntohs(laddr_in.sin_port),
> +                        inet_ntoa(raddr_in.sin_addr), ntohs(raddr_in.sin_port));
>  
>          break;
>      }
> @@ -521,7 +535,8 @@ static int net_dgram_init(NetClientState *peer,
>      }
>  
>      if (remote) {
> -        s->dgram_dst = raddr_in;
> +        g_assert(s->dgram_dst == NULL);
> +        s->dgram_dst = dgram_dst;
>  
>          pstrcpy(s->nc.info_str, sizeof(s->nc.info_str), info_str);
>          g_free(info_str);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]