diff mbox series

[RFC,1/4] slirp: add helper for tcp6 socket creation

Message ID 1540512223-21199-2-git-send-email-max7255@yandex-team.ru (mailing list archive)
State New, archived
Headers show
Series slirp: support hostfwd for ipv6 addresses | expand

Commit Message

max7255 Oct. 26, 2018, 12:03 a.m. UTC 
Signed-off-by: Maxim Samoylov <max7255@yandex-team.ru>
---
 slirp/socket.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 slirp/socket.h |  2 ++
 2 files changed, 75 insertions(+)

Comments

Samuel Thibault Oct. 27, 2018, 11:11 a.m. UTC | #1 
Hello,

Thanks for working on this!

There is a lot of overlap with tcp_listen. It'd be much better to
refactor it this way:

struct socket *
tcpx_listen(Slirp *slirp, struct sockaddr *addr, socklen_t *addrlen, int flags)
{
	... The current content of tcp_listen, with all heading and
	trailing addr manipulations removed...
	
	... so->so_lfamily = addr->sa_family;...
	... s = qemu_socket(addr->sa_family, SOCK_STREAM, 0);...
        ... (bind(s, addr, *addrlen) < 0) ||...
	... getsockname(s, addr, addrlen);
}

struct socket *
tcp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr,
           u_int lport, int flags)
{
	struct sockaddr_in addr;
	struct socket *so;
	socklen_t addrsize = sizeof(addr);

	addr.sin_family = AF_INET;
	addr.sin_addr.s_addr = haddr;
	addr.sin_port = hport;

	so = tcpx_listen(slirp, (struct sockaddr *) &addr, &addrsize, flags);

	so->so_lport = lport; /* Kept in network format */
	so->so_laddr.s_addr = laddr; /* Ditto */

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

The v6 version then boils down to

struct socket *
tcp6_listen(Slirp *slirp, struct in6_addr haddr, u_int hport, struct
	    in6_addr laddr, u_int lport, int flags)
{
	struct sockaddr_in6 addr;
	struct socket *so;
	socklen_t addrsize = sizeof(addr);

	addr.sin6_family = AF_INET6;
	addr.sin6_addr = haddr;
	addr.sin6_port = hport;

	so = tcpx_listen(slirp, (struct sockaddr *) &addr, &addrsize, flags);

	so->so_lport6 = lport; /* Kept in network format */
	so->so_laddr6 = laddr; /* Ditto */

	so->fhost.sin6 = addr;
	    
	if (!memcmp(&addr.sin6_addr, &in6addr_any, sizeof(in6addr_any)) ||
	    !memcmp(&addr.sin6_addr, &in6addr_loopback,
	            sizeof(in6addr_loopback))) {
	
	    memcpy(&so->so_faddr6, &slirp->vhost_addr6, sizeof(slirp->vhost_addr6));
	}
}

modulo all typos etc. I may have done.

Maxim Samoylov, le ven. 26 oct. 2018 03:03:40 +0300, a ecrit:
> +    qemu_setsockopt(s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(int));

Is there a reason why you set SO_OOBINLINE, but not TCP_NODELAY? That's
the kind of discrepancy we don't want to let unseen, thus the reason for
a shared implementation :)

Samuel
Samuel Thibault Oct. 27, 2018, 11:13 a.m. UTC | #2 
Samuel Thibault, le sam. 27 oct. 2018 13:11:41 +0200, a ecrit:
> struct socket *
> tcp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr,
>            u_int lport, int flags)
> {
> 	struct sockaddr_in addr;
> 	struct socket *so;
> 	socklen_t addrsize = sizeof(addr);

Also  memset(&addr, 0, sizeof(addr);

> 	addr.sin_family = AF_INET;
> 	addr.sin_addr.s_addr = haddr;
> 	addr.sin_port = hport;
max7255 Oct. 30, 2018, 1:58 p.m. UTC | #3 
On 27.10.2018 14:11, Samuel Thibault wrote:
> Hello,
> 
> Thanks for working on this!
> 

Hi!

I preferred to make this RFC simple and straightforward
with dumb code duplication because I wasn't sure if the feature is
useful for upstream at all :)

I will then unify v4 and v6 implementations as you suggested
(for other patches in the series too) and post the re-spin.

> There is a lot of overlap with tcp_listen. It'd be much better to
> refactor it this way:
> 
> struct socket *
> tcpx_listen(Slirp *slirp, struct sockaddr *addr, socklen_t *addrlen, int flags)
> {
> 	... The current content of tcp_listen, with all heading and
> 	trailing addr manipulations removed...
> 	
> 	... so->so_lfamily = addr->sa_family;...
> 	... s = qemu_socket(addr->sa_family, SOCK_STREAM, 0);...
>          ... (bind(s, addr, *addrlen) < 0) ||...
> 	... getsockname(s, addr, addrlen);
> }
> 
> struct socket *
> tcp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr,
>             u_int lport, int flags)
> {
> 	struct sockaddr_in addr;
> 	struct socket *so;
> 	socklen_t addrsize = sizeof(addr);
> 
> 	addr.sin_family = AF_INET;
> 	addr.sin_addr.s_addr = haddr;
> 	addr.sin_port = hport;
> 
> 	so = tcpx_listen(slirp, (struct sockaddr *) &addr, &addrsize, flags);
> 
> 	so->so_lport = lport; /* Kept in network format */
> 	so->so_laddr.s_addr = laddr; /* Ditto */
> 
> 	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;
> }
> 
> The v6 version then boils down to
> 
> struct socket *
> tcp6_listen(Slirp *slirp, struct in6_addr haddr, u_int hport, struct
> 	    in6_addr laddr, u_int lport, int flags)
> {
> 	struct sockaddr_in6 addr;
> 	struct socket *so;
> 	socklen_t addrsize = sizeof(addr);
> 
> 	addr.sin6_family = AF_INET6;
> 	addr.sin6_addr = haddr;
> 	addr.sin6_port = hport;
> 
> 	so = tcpx_listen(slirp, (struct sockaddr *) &addr, &addrsize, flags);
> 
> 	so->so_lport6 = lport; /* Kept in network format */
> 	so->so_laddr6 = laddr; /* Ditto */
> 
> 	so->fhost.sin6 = addr;
> 	
> 	if (!memcmp(&addr.sin6_addr, &in6addr_any, sizeof(in6addr_any)) ||
> 	    !memcmp(&addr.sin6_addr, &in6addr_loopback,
> 	            sizeof(in6addr_loopback))) {
> 	
> 	    memcpy(&so->so_faddr6, &slirp->vhost_addr6, sizeof(slirp->vhost_addr6));
> 	}
> }
> 
> modulo all typos etc. I may have done.
> 
> Maxim Samoylov, le ven. 26 oct. 2018 03:03:40 +0300, a ecrit:
>> +    qemu_setsockopt(s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(int));
> 
> Is there a reason why you set SO_OOBINLINE, but not TCP_NODELAY? That's
> the kind of discrepancy we don't want to let unseen, thus the reason for
> a shared implementation :)

Actually my code was initially based on the last year master state, so I
missed your changes on TCP_NODELAY while rebasing, will fix.

> 
> Samuel
>

---
Maxim Samoylov, max7255@yandex-team.ru
Samuel Thibault Oct. 30, 2018, 4 p.m. UTC | #4 
Maxim Samoylov, le mar. 30 oct. 2018 16:58:17 +0300, a ecrit:
> On 27.10.2018 14:11, Samuel Thibault wrote:
> > Is there a reason why you set SO_OOBINLINE, but not TCP_NODELAY? That's
> > the kind of discrepancy we don't want to let unseen, thus the reason for
> > a shared implementation :)
> 
> Actually my code was initially based on the last year master state, so I
> missed your changes on TCP_NODELAY while rebasing

Which shows why it's useful to share the code :)

Samuel
diff mbox series

Patch

diff --git a/slirp/socket.c b/slirp/socket.c
index 322383a..e16e6c1 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -776,6 +776,79 @@  tcp_listen(Slirp *slirp, uint32_t haddr, u_int hport, uint32_t laddr,
 	return so;
 }
 
+struct socket *
+tcp6_listen(Slirp *slirp, struct in6_addr haddr, u_int hport,
+            struct in6_addr laddr, u_int lport, int flags)
+{
+    struct sockaddr_in6 addr;
+    struct socket *so;
+    int s, opt = 1;
+    socklen_t addrlen = sizeof(addr);
+    memset(&addr, 0, addrlen);
+
+    /* The same flow as in tcp_listen above */
+
+    so = socreate(slirp);
+    if (!so) {
+        return NULL;
+    }
+
+    so->so_tcpcb = tcp_newtcpcb(so);
+    if (so->so_tcpcb == NULL) {
+        free(so);
+        return NULL;
+    }
+
+    insque(so, &slirp->tcb);
+
+    if (flags & SS_FACCEPTONCE) {
+        so->so_tcpcb->t_timer[TCPT_KEEP] = TCPTV_KEEP_INIT * 2;
+    }
+
+    so->so_state &= SS_PERSISTENT_MASK;
+    so->so_state |= (SS_FACCEPTCONN | flags);
+    so->so_lfamily = AF_INET6;
+    so->so_lport6 = lport; /* Kept in network format */
+    so->so_laddr6 = laddr;
+
+    addr.sin6_family = AF_INET6;
+    addr.sin6_addr = haddr;
+    addr.sin6_port = hport;
+
+    s = qemu_socket(AF_INET6, SOCK_STREAM, 0);
+    if ((s < 0) ||
+        (socket_set_fast_reuse(s) < 0) ||
+        (bind(s, (struct sockaddr *)&addr, sizeof(addr)) < 0) ||
+        (listen(s, 1) < 0)) {
+        int tmperrno = errno; /* Don't clobber the real reason we failed */
+            if (s >= 0) {
+                closesocket(s);
+            }
+        sofree(so);
+        /* Restore the real errno */
+#ifdef _WIN32
+        WSASetLastError(tmperrno);
+#else
+        errno = tmperrno;
+#endif
+        return NULL;
+    }
+    qemu_setsockopt(s, SOL_SOCKET, SO_OOBINLINE, &opt, sizeof(int));
+
+    getsockname(s, (struct sockaddr *)&addr, &addrlen);
+    so->fhost.sin6 = addr;
+
+    if (!memcmp(&addr.sin6_addr, &in6addr_any, sizeof(in6addr_any)) ||
+        !memcmp(&addr.sin6_addr, &in6addr_loopback,
+                sizeof(in6addr_loopback))) {
+
+        memcpy(&so->so_faddr6, &slirp->vhost_addr6, sizeof(slirp->vhost_addr6));
+    }
+
+    so->s = s;
+    return so;
+}
+
 /*
  * Various session state calls
  * XXX Should be #define's
diff --git a/slirp/socket.h b/slirp/socket.h
index 2f224bc..b200bb6 100644
--- a/slirp/socket.h
+++ b/slirp/socket.h
@@ -144,6 +144,8 @@  void sorecvfrom(struct socket *);
 int sosendto(struct socket *, struct mbuf *);
 struct socket * tcp_listen(Slirp *, uint32_t, u_int, uint32_t, u_int,
                                int);
+struct socket *tcp6_listen(Slirp *, struct in6_addr, u_int,
+                           struct in6_addr, u_int, int);
 void soisfconnecting(register struct socket *);
 void soisfconnected(register struct socket *);
 void sofwdrain(struct socket *);