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 <maet...@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 <maet...@subiron.org>
>> Signed-off-by: Samuel Thibault <samuel.thiba...@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 <th...@redhat.com>
