From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41462) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8RV9-000570-I8 for qemu-devel@nongnu.org; Mon, 14 Dec 2015 06:41:12 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a8RV5-0001Ux-9f for qemu-devel@nongnu.org; Mon, 14 Dec 2015 06:41:11 -0500 Received: from mx1.redhat.com ([209.132.183.28]:58081) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a8RV5-0001Ui-0r for qemu-devel@nongnu.org; Mon, 14 Dec 2015 06:41:07 -0500 References: <20151211001505.GV2905@var.home> <1449792930-27293-1-git-send-email-samuel.thibault@ens-lyon.org> <1449792930-27293-5-git-send-email-samuel.thibault@ens-lyon.org> <20151211231431.GD2764@var.home> From: Thomas Huth Message-ID: <566EAACC.6040405@redhat.com> Date: Mon, 14 Dec 2015 12:41:00 +0100 MIME-Version: 1.0 In-Reply-To: <20151211231431.GD2764@var.home> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 05/18] slirp: Factorizing address translation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Samuel Thibault , qemu-devel@nongnu.org Cc: zhanghailiang , Li Zhijian , Stefan Hajnoczi , Jason Wang , Dave Gilbert , Vasiliy Tolstov , Huangpeng , Gonglei , Jan Kiszka , Yang Hongyang , Guillaume Subiron On 12/12/15 00:14, Samuel Thibault wrote: > Hello, >=20 > 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... >=20 > Samuel Thibault, on Fri 11 Dec 2015 01:15:17 +0100, wrote: >> From: Guillaume Subiron >> >> 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 t= he host >> for port forwarding: if it comes from localhost, the host virtual addr= ess is >> used instead. >> >> Signed-off-by: Guillaume Subiron >> Signed-off-by: Samuel Thibault >> --- [...] >> 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 =3D sizeof(struct sockaddr_storage); >> =20 >> DEBUG_CALL("sorecvfrom"); >> @@ -525,11 +526,17 @@ sorecvfrom(struct socket *so) >> =20 >> /* >> * If this packet was destined for CTL_ADDR, >> - * make it look like that's where it came from, done by udp_outp= ut >> + * make it look like that's where it came from >> */ >> + saddr =3D addr; >> + sotranslate_in(so, &saddr); >> + daddr =3D 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 =3D so->slirp; >> int ret; >> - struct sockaddr_in addr; >> + struct sockaddr_storage addr; >> =20 >> DEBUG_CALL("sosendto"); >> DEBUG_ARG("so =3D %p", so); >> DEBUG_ARG("m =3D %p", m); >> =20 >> - addr.sin_family =3D AF_INET; >> - if ((so->so_faddr.s_addr & slirp->vnetwork_mask.s_addr) =3D=3D >> - slirp->vnetwork_addr.s_addr) { >> - /* It's an alias */ >> - if (so->so_faddr.s_addr =3D=3D slirp->vnameserver_addr.s_addr) { >> - if (get_dns_addr(&addr.sin_addr) < 0) >> - addr.sin_addr =3D loopback_addr; >> - } else { >> - addr.sin_addr =3D loopback_addr; >> - } >> - } else >> - addr.sin_addr =3D so->so_faddr; >> - addr.sin_port =3D so->so_fport; >> - >> - DEBUG_MISC((dfd, " sendto()ing, addr.sin_port=3D%d, addr.sin_addr.s_= addr=3D%.16s\n", ntohs(addr.sin_port), inet_ntoa(addr.sin_addr))); >> + addr =3D so->fhost.ss; >> + DEBUG_CALL(" sendto()ing)"); >> + sotranslate_out(so, &addr); >> =20 >> /* Don't care what port we get */ >> ret =3D 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; >> =20 >> @@ -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 =3D so->slirp; >> + struct sockaddr_in *sin =3D (struct sockaddr_in *)addr; >> + >> + switch (addr->ss_family) { >> + case AF_INET: >> + if ((so->so_faddr.s_addr & slirp->vnetwork_mask.s_addr) =3D=3D >> + slirp->vnetwork_addr.s_addr) { >> + /* It's an alias */ >> + if (so->so_faddr.s_addr =3D=3D slirp->vnameserver_addr.s_= addr) { >> + if (get_dns_addr(&sin->sin_addr) < 0) { >> + sin->sin_addr =3D loopback_addr; >> + } >> + } else { >> + sin->sin_addr =3D loopback_addr; >> + } >> + } >> + >> + DEBUG_MISC((dfd, " addr.sin_port=3D%d, " >> + "addr.sin_addr.s_addr=3D%.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 =3D so->slirp; >> + struct sockaddr_in *sin =3D (struct sockaddr_in *)addr; >> + >> + switch (addr->ss_family) { >> + case AF_INET: >> + if ((so->so_faddr.s_addr & slirp->vnetwork_mask.s_addr) =3D=3D >> + slirp->vnetwork_addr.s_addr) { >> + uint32_t inv_mask =3D ~slirp->vnetwork_mask.s_addr; >> + >> + if ((so->so_faddr.s_addr & inv_mask) =3D=3D inv_mask) { >> + sin->sin_addr =3D slirp->vhost_addr; >> + } else if (sin->sin_addr.s_addr =3D=3D loopback_addr.s_ad= dr || >> + so->so_faddr.s_addr !=3D slirp->vhost_addr.s_a= ddr) { >> + sin->sin_addr =3D so->so_faddr; >> + } >> + } >> + break; >> + >> + default: >> + break; >> + } >> +} >> + >> +/* >> + * Translate connections from localhost to the real hostname >> + * :TODO: IPv6 >> + */ >> +void sotranslate_accept(struct socket *so) >> +{ >> + Slirp *slirp =3D so->slirp; >> + >> + switch (so->so_ffamily) { >> + case AF_INET: >> + if (so->so_faddr.s_addr =3D=3D INADDR_ANY || >> + (so->so_faddr.s_addr & loopback_mask) =3D=3D >> + (loopback_addr.s_addr & loopback_mask)) { >> + so->so_faddr =3D 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