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

[Thomas Huth <thuth@redhat.com>]

On 12/12/15 00:14, Samuel Thibault wrote:
> Hello,
> 
> Thomas, this is the last refactoring patch which doesn't have a review
> yet, right?

Right. It somehow showed up out of order in my e-mail program, so I
missed it on Friday, sorry. So here's a review...

> 
> 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

s/paquet/packet/ and s/destinated/destined/ (I think)

>> 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

s/existant/existent/

>> 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>
>> ---
[...]
>> 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

In case you rework this patch, I think you could remove the "TODO" ...
that looks rather like an interim developer's comment that should not be
in the final patch, I think.

>> + */
>> +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 */

dito

>> +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;
>> +    }
>> +}
[...]

I'd maybe also split this up into several patches, one for
sotranslate_out(), and one or two for the remaining changes, but it also
looks fine to me as it currently is already.

Reviewed-by: Thomas Huth <thuth@redhat.com>