Re: [Qemu-devel] [PATCH 05/18] slirp: Factorizing address translation

[Samuel Thibault <samuel.thibault@gnu.org>]

Hello,

Thomas, this is the last refactoring patch which doesn't have a review
yet, right?

Samuel

Samuel Thibault, on Fri 11 Dec 2015 01:15:17 +0100, wrote:
> From: Guillaume Subiron <maethor@subiron.org>
> 
> This patch factorizes some duplicate code into a new function,
> sotranslate_out(). This function perform the address translation when a
> packet is transmitted to the host network. If the paquet is destinated
> to the host, the loopback address is used, and if the paquet is
> destinated to the virtual DNS, the real DNS address is used. This code
> is just a copy of the existant, but factorized and ready to manage the
> IPv6 case.
> 
> On the same model, the major part of udp_output() code is moved into a
> new sotranslate_in(). This function is directly used in sorecvfrom(),
> like sotranslate_out() in sosendto().
> udp_output() becoming useless, it is removed and udp_output2() is
> renamed into udp_output(). This adds consistency with the udp6_output()
> function introduced by further patches.
> 
> Lastly, this factorizes some duplicate code into sotranslate_accept(), which
> performs the address translation when a connection is established on the host
> for port forwarding: if it comes from localhost, the host virtual address is
> used instead.
> 
> Signed-off-by: Guillaume Subiron <maethor@subiron.org>
> Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
> ---
>  slirp/bootp.c    |   2 +-
>  slirp/ip_icmp.c  |  19 ++-------
>  slirp/socket.c   | 115 +++++++++++++++++++++++++++++++++++++++++++++----------
>  slirp/socket.h   |   5 +++
>  slirp/tcp_subr.c |  35 ++++-------------
>  slirp/tftp.c     |   6 +--
>  slirp/udp.c      |  37 ++----------------
>  slirp/udp.h      |   3 +-
>  8 files changed, 119 insertions(+), 103 deletions(-)
> 
> diff --git a/slirp/bootp.c b/slirp/bootp.c
> index 1baaab1..0027279 100644
> --- a/slirp/bootp.c
> +++ b/slirp/bootp.c
> @@ -325,7 +325,7 @@ static void bootp_reply(Slirp *slirp, const struct bootp_t *bp)
>  
>      m->m_len = sizeof(struct bootp_t) -
>          sizeof(struct ip) - sizeof(struct udphdr);
> -    udp_output2(NULL, m, &saddr, &daddr, IPTOS_LOWDELAY);
> +    udp_output(NULL, m, &saddr, &daddr, IPTOS_LOWDELAY);
>  }
>  
>  void bootp_input(struct mbuf *m)
> diff --git a/slirp/ip_icmp.c b/slirp/ip_icmp.c
> index 58b7ceb..3a29847 100644
> --- a/slirp/ip_icmp.c
> +++ b/slirp/ip_icmp.c
> @@ -157,7 +157,7 @@ icmp_input(struct mbuf *m, int hlen)
>          goto freeit;
>      } else {
>        struct socket *so;
> -      struct sockaddr_in addr;
> +      struct sockaddr_storage addr;
>        if ((so = socreate(slirp)) == NULL) goto freeit;
>        if (icmp_send(so, m, hlen) == 0) {
>          return;
> @@ -181,20 +181,9 @@ icmp_input(struct mbuf *m, int hlen)
>        so->so_state = SS_ISFCONNECTED;
>  
>        /* Send the packet */
> -      addr.sin_family = AF_INET;
> -      if ((so->so_faddr.s_addr & slirp->vnetwork_mask.s_addr) ==
> -          slirp->vnetwork_addr.s_addr) {
> -	/* It's an alias */
> -	if (so->so_faddr.s_addr == slirp->vnameserver_addr.s_addr) {
> -	  if (get_dns_addr(&addr.sin_addr) < 0)
> -	    addr.sin_addr = loopback_addr;
> -	} else {
> -	  addr.sin_addr = loopback_addr;
> -	}
> -      } else {
> -	addr.sin_addr = so->so_faddr;
> -      }
> -      addr.sin_port = so->so_fport;
> +      addr = so->fhost.ss;
> +      sotranslate_out(so, &addr);
> +
>        if(sendto(so->s, icmp_ping_msg, strlen(icmp_ping_msg), 0,
>  		(struct sockaddr *)&addr, sizeof(addr)) == -1) {
>  	DEBUG_MISC((dfd,"icmp_input udp sendto tx errno = %d-%s\n",
> diff --git a/slirp/socket.c b/slirp/socket.c
> index bf603c9..97948e8 100644
> --- a/slirp/socket.c
> +++ b/slirp/socket.c
> @@ -438,6 +438,7 @@ void
>  sorecvfrom(struct socket *so)
>  {
>  	struct sockaddr_storage addr;
> +	struct sockaddr_storage saddr, daddr;
>  	socklen_t addrlen = sizeof(struct sockaddr_storage);
>  
>  	DEBUG_CALL("sorecvfrom");
> @@ -525,11 +526,17 @@ sorecvfrom(struct socket *so)
>  
>  	    /*
>  	     * If this packet was destined for CTL_ADDR,
> -	     * make it look like that's where it came from, done by udp_output
> +	     * make it look like that's where it came from
>  	     */
> +	    saddr = addr;
> +	    sotranslate_in(so, &saddr);
> +	    daddr = so->lhost.ss;
> +
>  	    switch (so->so_ffamily) {
>  	    case AF_INET:
> -	        udp_output(so, m, (struct sockaddr_in *) &addr);
> +	        udp_output(so, m, (struct sockaddr_in *) &saddr,
> +	                   (struct sockaddr_in *) &daddr,
> +	                   so->so_iptos);
>  	        break;
>  	    default:
>  	        break;
> @@ -544,33 +551,20 @@ sorecvfrom(struct socket *so)
>  int
>  sosendto(struct socket *so, struct mbuf *m)
>  {
> -	Slirp *slirp = so->slirp;
>  	int ret;
> -	struct sockaddr_in addr;
> +	struct sockaddr_storage addr;
>  
>  	DEBUG_CALL("sosendto");
>  	DEBUG_ARG("so = %p", so);
>  	DEBUG_ARG("m = %p", m);
>  
> -        addr.sin_family = AF_INET;
> -	if ((so->so_faddr.s_addr & slirp->vnetwork_mask.s_addr) ==
> -	    slirp->vnetwork_addr.s_addr) {
> -	  /* It's an alias */
> -	  if (so->so_faddr.s_addr == slirp->vnameserver_addr.s_addr) {
> -	    if (get_dns_addr(&addr.sin_addr) < 0)
> -	      addr.sin_addr = loopback_addr;
> -	  } else {
> -	    addr.sin_addr = loopback_addr;
> -	  }
> -	} else
> -	  addr.sin_addr = so->so_faddr;
> -	addr.sin_port = so->so_fport;
> -
> -	DEBUG_MISC((dfd, " sendto()ing, addr.sin_port=%d, addr.sin_addr.s_addr=%.16s\n", ntohs(addr.sin_port), inet_ntoa(addr.sin_addr)));
> +	addr = so->fhost.ss;
> +	DEBUG_CALL(" sendto()ing)");
> +	sotranslate_out(so, &addr);
>  
>  	/* Don't care what port we get */
>  	ret = sendto(so->s, m->m_data, m->m_len, 0,
> -		     (struct sockaddr *)&addr, sizeof (struct sockaddr));
> +		     (struct sockaddr *)&addr, sizeof(addr));
>  	if (ret < 0)
>  		return -1;
>  
> @@ -726,3 +720,84 @@ sofwdrain(struct socket *so)
>  	else
>  		sofcantsendmore(so);
>  }
> +
> +/*
> + * Translate addr in host addr when it is a virtual address
> + * :TODO:maethor:130314: Manage IPv6
> + */
> +void sotranslate_out(struct socket *so, struct sockaddr_storage *addr)
> +{
> +    Slirp *slirp = so->slirp;
> +    struct sockaddr_in *sin = (struct sockaddr_in *)addr;
> +
> +    switch (addr->ss_family) {
> +    case AF_INET:
> +        if ((so->so_faddr.s_addr & slirp->vnetwork_mask.s_addr) ==
> +                slirp->vnetwork_addr.s_addr) {
> +            /* It's an alias */
> +            if (so->so_faddr.s_addr == slirp->vnameserver_addr.s_addr) {
> +                if (get_dns_addr(&sin->sin_addr) < 0) {
> +                    sin->sin_addr = loopback_addr;
> +                }
> +            } else {
> +                sin->sin_addr = loopback_addr;
> +            }
> +        }
> +
> +        DEBUG_MISC((dfd, " addr.sin_port=%d, "
> +            "addr.sin_addr.s_addr=%.16s\n",
> +            ntohs(sin->sin_port), inet_ntoa(sin->sin_addr)));
> +        break;
> +
> +    default:
> +        break;
> +    }
> +}
> +
> +/* :TODO:maethor:130314: IPv6 */
> +void sotranslate_in(struct socket *so, struct sockaddr_storage *addr)
> +{
> +    Slirp *slirp = so->slirp;
> +    struct sockaddr_in *sin = (struct sockaddr_in *)addr;
> +
> +    switch (addr->ss_family) {
> +    case AF_INET:
> +        if ((so->so_faddr.s_addr & slirp->vnetwork_mask.s_addr) ==
> +            slirp->vnetwork_addr.s_addr) {
> +            uint32_t inv_mask = ~slirp->vnetwork_mask.s_addr;
> +
> +            if ((so->so_faddr.s_addr & inv_mask) == inv_mask) {
> +                sin->sin_addr = slirp->vhost_addr;
> +            } else if (sin->sin_addr.s_addr == loopback_addr.s_addr ||
> +                       so->so_faddr.s_addr != slirp->vhost_addr.s_addr) {
> +                sin->sin_addr = so->so_faddr;
> +            }
> +        }
> +        break;
> +
> +    default:
> +        break;
> +    }
> +}
> +
> +/*
> + * Translate connections from localhost to the real hostname
> + * :TODO: IPv6
> + */
> +void sotranslate_accept(struct socket *so)
> +{
> +    Slirp *slirp = so->slirp;
> +
> +    switch (so->so_ffamily) {
> +    case AF_INET:
> +        if (so->so_faddr.s_addr == INADDR_ANY ||
> +            (so->so_faddr.s_addr & loopback_mask) ==
> +            (loopback_addr.s_addr & loopback_mask)) {
> +           so->so_faddr = slirp->vhost_addr;
> +        }
> +        break;
> +
> +    default:
> +        break;
> +    }
> +}
> diff --git a/slirp/socket.h b/slirp/socket.h
> index e854903..b27bbb2 100644
> --- a/slirp/socket.h
> +++ b/slirp/socket.h
> @@ -105,4 +105,9 @@ struct iovec; /* For win32 */
>  size_t sopreprbuf(struct socket *so, struct iovec *iov, int *np);
>  int soreadbuf(struct socket *so, const char *buf, int size);
>  
> +void sotranslate_out(struct socket *, struct sockaddr_storage *);
> +void sotranslate_in(struct socket *, struct sockaddr_storage *);
> +void sotranslate_accept(struct socket *);
> +
> +
>  #endif /* _SOCKET_H_ */
> diff --git a/slirp/tcp_subr.c b/slirp/tcp_subr.c
> index 47262db..76c716f 100644
> --- a/slirp/tcp_subr.c
> +++ b/slirp/tcp_subr.c
> @@ -326,7 +326,6 @@ tcp_sockclosed(struct tcpcb *tp)
>   */
>  int tcp_fconnect(struct socket *so)
>  {
> -  Slirp *slirp = so->slirp;
>    int ret=0;
>  
>    DEBUG_CALL("tcp_fconnect");
> @@ -334,30 +333,17 @@ int tcp_fconnect(struct socket *so)
>  
>    if( (ret = so->s = qemu_socket(AF_INET,SOCK_STREAM,0)) >= 0) {
>      int opt, s=so->s;
> -    struct sockaddr_in addr;
> +    struct sockaddr_storage addr;
>  
>      qemu_set_nonblock(s);
>      socket_set_fast_reuse(s);
>      opt = 1;
>      qemu_setsockopt(s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(opt));
>  
> -    addr.sin_family = AF_INET;
> -    if ((so->so_faddr.s_addr & slirp->vnetwork_mask.s_addr) ==
> -        slirp->vnetwork_addr.s_addr) {
> -      /* It's an alias */
> -      if (so->so_faddr.s_addr == slirp->vnameserver_addr.s_addr) {
> -	if (get_dns_addr(&addr.sin_addr) < 0)
> -	  addr.sin_addr = loopback_addr;
> -      } else {
> -	addr.sin_addr = loopback_addr;
> -      }
> -    } else
> -      addr.sin_addr = so->so_faddr;
> -    addr.sin_port = so->so_fport;
> -
> -    DEBUG_MISC((dfd, " connect()ing, addr.sin_port=%d, "
> -		"addr.sin_addr.s_addr=%.16s\n",
> -		ntohs(addr.sin_port), inet_ntoa(addr.sin_addr)));
> +    addr = so->fhost.ss;
> +    DEBUG_CALL(" connect()ing")
> +    sotranslate_out(so, &addr);
> +
>      /* We don't care what port we get */
>      ret = connect(s,(struct sockaddr *)&addr,sizeof (addr));
>  
> @@ -431,15 +417,8 @@ void tcp_connect(struct socket *inso)
>      qemu_setsockopt(s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(int));
>      socket_set_nodelay(s);
>  
> -    so->so_ffamily = AF_INET;
> -    so->so_fport = addr.sin_port;
> -    so->so_faddr = addr.sin_addr;
> -    /* Translate connections from localhost to the real hostname */
> -    if (so->so_faddr.s_addr == 0 ||
> -        (so->so_faddr.s_addr & loopback_mask) ==
> -        (loopback_addr.s_addr & loopback_mask)) {
> -        so->so_faddr = slirp->vhost_addr;
> -    }
> +    so->fhost.sin = addr;
> +    sotranslate_accept(so);
>  
>      /* Close the accept() socket, set right state */
>      if (inso->so_state & SS_FACCEPTONCE) {
> diff --git a/slirp/tftp.c b/slirp/tftp.c
> index a329fb2..ccb6130 100644
> --- a/slirp/tftp.c
> +++ b/slirp/tftp.c
> @@ -155,7 +155,7 @@ static int tftp_send_oack(struct tftp_session *spt,
>  
>      m->m_len = sizeof(struct tftp_t) - 514 + n -
>          sizeof(struct ip) - sizeof(struct udphdr);
> -    udp_output2(NULL, m, &saddr, &daddr, IPTOS_LOWDELAY);
> +    udp_output(NULL, m, &saddr, &daddr, IPTOS_LOWDELAY);
>  
>      return 0;
>  }
> @@ -193,7 +193,7 @@ static void tftp_send_error(struct tftp_session *spt,
>    m->m_len = sizeof(struct tftp_t) - 514 + 3 + strlen(msg) -
>          sizeof(struct ip) - sizeof(struct udphdr);
>  
> -  udp_output2(NULL, m, &saddr, &daddr, IPTOS_LOWDELAY);
> +  udp_output(NULL, m, &saddr, &daddr, IPTOS_LOWDELAY);
>  
>  out:
>    tftp_session_terminate(spt);
> @@ -243,7 +243,7 @@ static void tftp_send_next_block(struct tftp_session *spt,
>    m->m_len = sizeof(struct tftp_t) - (512 - nobytes) -
>          sizeof(struct ip) - sizeof(struct udphdr);
>  
> -  udp_output2(NULL, m, &saddr, &daddr, IPTOS_LOWDELAY);
> +  udp_output(NULL, m, &saddr, &daddr, IPTOS_LOWDELAY);
>  
>    if (nobytes == 512) {
>      tftp_session_update(spt);
> diff --git a/slirp/udp.c b/slirp/udp.c
> index 7a5c95b..8203eb1 100644
> --- a/slirp/udp.c
> +++ b/slirp/udp.c
> @@ -236,7 +236,7 @@ bad:
>  	m_free(m);
>  }
>  
> -int udp_output2(struct socket *so, struct mbuf *m,
> +int udp_output(struct socket *so, struct mbuf *m,
>                  struct sockaddr_in *saddr, struct sockaddr_in *daddr,
>                  int iptos)
>  {
> @@ -287,31 +287,6 @@ int udp_output2(struct socket *so, struct mbuf *m,
>  	return (error);
>  }
>  
> -int udp_output(struct socket *so, struct mbuf *m,
> -               struct sockaddr_in *addr)
> -
> -{
> -    Slirp *slirp = so->slirp;
> -    struct sockaddr_in saddr, daddr;
> -
> -    saddr = *addr;
> -    if ((so->so_faddr.s_addr & slirp->vnetwork_mask.s_addr) ==
> -        slirp->vnetwork_addr.s_addr) {
> -        uint32_t inv_mask = ~slirp->vnetwork_mask.s_addr;
> -
> -        if ((so->so_faddr.s_addr & inv_mask) == inv_mask) {
> -            saddr.sin_addr = slirp->vhost_addr;
> -        } else if (addr->sin_addr.s_addr == loopback_addr.s_addr ||
> -                   so->so_faddr.s_addr != slirp->vhost_addr.s_addr) {
> -            saddr.sin_addr = so->so_faddr;
> -        }
> -    }
> -    daddr.sin_addr = so->so_laddr;
> -    daddr.sin_port = so->so_lport;
> -
> -    return udp_output2(so, m, &saddr, &daddr, so->so_iptos);
> -}
> -
>  int
>  udp_attach(struct socket *so)
>  {
> @@ -378,14 +353,8 @@ udp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr,
>  	socket_set_fast_reuse(so->s);
>  
>  	getsockname(so->s,(struct sockaddr *)&addr,&addrlen);
> -	so->so_ffamily = AF_INET;
> -	so->so_fport = addr.sin_port;
> -	if (addr.sin_addr.s_addr == 0 ||
> -	    addr.sin_addr.s_addr == loopback_addr.s_addr) {
> -	   so->so_faddr = slirp->vhost_addr;
> -	} else {
> -	   so->so_faddr = addr.sin_addr;
> -	}
> +	so->fhost.sin = addr;
> +	sotranslate_accept(so);
>  	so->so_lfamily = AF_INET;
>  	so->so_lport = lport;
>  	so->so_laddr.s_addr = laddr;
> diff --git a/slirp/udp.h b/slirp/udp.h
> index 9bf31fe..a04b8ce 100644
> --- a/slirp/udp.h
> +++ b/slirp/udp.h
> @@ -76,12 +76,11 @@ struct mbuf;
>  void udp_init(Slirp *);
>  void udp_cleanup(Slirp *);
>  void udp_input(register struct mbuf *, int);
> -int udp_output(struct socket *, struct mbuf *, struct sockaddr_in *);
>  int udp_attach(struct socket *);
>  void udp_detach(struct socket *);
>  struct socket * udp_listen(Slirp *, uint32_t, u_int, uint32_t, u_int,
>                             int);
> -int udp_output2(struct socket *so, struct mbuf *m,
> +int udp_output(struct socket *so, struct mbuf *m,
>                  struct sockaddr_in *saddr, struct sockaddr_in *daddr,
>                  int iptos);
>  #endif
> -- 
> 2.6.2
> 

-- 
Samuel
<c> hiri, le cri ici, c des marrants
<c> j'ai un rep ".uglyhackdirectorywithoutacls" ds mon home
 -+- #ens-mim en stage -+-