Re: [PATCH 1/4] util: Split construction of bind socket address from the rest of sock_l4()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 7309 bytes --]

On Thu, Jun 13, 2024 at 05:06:25PM +0200, Stefano Brivio wrote:
> Sorry for the delay, nits only (I can fix them up on merge):

Ok, I'll respin anyway, though, since it looks like there's a slightly
non-trivial rebase needed on Laurent's stuff.

> On Wed,  5 Jun 2024 11:39:00 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > sock_l4() creates, binds and otherwise prepares a new socket.  It builds
> > the socket address to bind from separately provided address and port.
> > However, we have use cases coming up where it's more natural to construct
> > the socket address in the caller.
> > 
> > Prepare for this by adding sock_l4_sa() which takes a pre-constructed
> > socket address, and rewriting sock_l4() in terms of it.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  util.c | 123 ++++++++++++++++++++++++++++++++-------------------------
> >  1 file changed, 70 insertions(+), 53 deletions(-)
> > 
> > diff --git a/util.c b/util.c
> > index cc1c73ba..4e5f6d23 100644
> > --- a/util.c
> > +++ b/util.c
> > @@ -33,36 +33,25 @@
> >  #include "log.h"
> >  
> >  /**
> > - * sock_l4() - Create and bind socket for given L4, add to epoll list
> > + * sock_l4_sa() - Create and bind socket for given L4, add to epoll list
> 
> That doesn't quite tell the difference from sock_l4(), perhaps:
> 
>  * sock_l4_sa() - Create and bind socket given socket address, add to epoll list

Good idea, done.

> >   * @c:		Execution context
> > - * @af:		Address family, AF_INET or AF_INET6
> >   * @proto:	Protocol number
> > - * @bind_addr:	Address for binding, NULL for any
> > + * @sa:		Socket address to bind to
> > + * @sl:		Length of @sa
> >   * @ifname:	Interface for binding, NULL for any
> > - * @port:	Port, host order
> > + * @v6only:	Set IPV6_V6ONLY socket option
> >   * @data:	epoll reference portion for protocol handlers
> >   *
> >   * Return: newly created socket, negative error code on failure
> >   */
> > -int sock_l4(const struct ctx *c, sa_family_t af, uint8_t proto,
> > -	    const void *bind_addr, const char *ifname, uint16_t port,
> > -	    uint32_t data)
> > +static int sock_l4_sa(const struct ctx *c, uint8_t proto,
> > +		      const void *sa, socklen_t sl,
> > +		      const char *ifname, bool v6only, uint32_t data)
> >  {
> > +	sa_family_t af =((const struct sockaddr *)sa)->sa_family;
> 
> Missing whitespace after =.

Oops.

> >  	union epoll_ref ref = { .data = data };
> > -	struct sockaddr_in addr4 = {
> > -		.sin_family = AF_INET,
> > -		.sin_port = htons(port),
> > -		{ 0 }, { 0 },
> > -	};
> > -	struct sockaddr_in6 addr6 = {
> > -		.sin6_family = AF_INET6,
> > -		.sin6_port = htons(port),
> > -		0, IN6ADDR_ANY_INIT, 0,
> > -	};
> > -	const struct sockaddr *sa;
> > -	bool dual_stack = false;
> > -	int fd, sl, y = 1, ret;
> >  	struct epoll_event ev;
> > +	int fd, y = 1, ret;
> >  
> >  	switch (proto) {
> >  	case IPPROTO_TCP:
> > @@ -79,13 +68,6 @@ int sock_l4(const struct ctx *c, sa_family_t af, uint8_t proto,
> >  		return -EPFNOSUPPORT;	/* Not implemented. */
> >  	}
> >  
> > -	if (af == AF_UNSPEC) {
> > -		if (!DUAL_STACK_SOCKETS || bind_addr)
> > -			return -EINVAL;
> > -		dual_stack = true;
> > -		af = AF_INET6;
> > -	}
> > -
> >  	if (proto == IPPROTO_TCP)
> >  		fd = socket(af, SOCK_STREAM | SOCK_NONBLOCK, proto);
> >  	else
> > @@ -104,30 +86,9 @@ int sock_l4(const struct ctx *c, sa_family_t af, uint8_t proto,
> >  
> >  	ref.fd = fd;
> >  
> > -	if (af == AF_INET) {
> > -		if (bind_addr)
> > -			addr4.sin_addr = *(struct in_addr *)bind_addr;
> > -
> > -		sa = (const struct sockaddr *)&addr4;
> > -		sl = sizeof(addr4);
> > -	} else {
> > -		if (bind_addr) {
> > -			addr6.sin6_addr = *(struct in6_addr *)bind_addr;
> > -
> > -			if (!memcmp(bind_addr, &c->ip6.addr_ll,
> > -			    sizeof(c->ip6.addr_ll)))
> > -				addr6.sin6_scope_id = c->ifi6;
> > -		}
> > -
> > -		sa = (const struct sockaddr *)&addr6;
> > -		sl = sizeof(addr6);
> > -
> > -		if (!dual_stack)
> > -			if (setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY,
> > -				       &y, sizeof(y)))
> > -				debug("Failed to set IPV6_V6ONLY on socket %i",
> > -				      fd);
> > -	}
> > +	if (v6only)
> > +		if (setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &y, sizeof(y)))
> > +			debug("Failed to set IPV6_V6ONLY on socket %i", fd);
> >  
> >  	if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &y, sizeof(y)))
> >  		debug("Failed to set SO_REUSEADDR on socket %i", fd);
> > @@ -140,9 +101,12 @@ int sock_l4(const struct ctx *c, sa_family_t af, uint8_t proto,
> >  		 */
> >  		if (setsockopt(fd, SOL_SOCKET, SO_BINDTODEVICE,
> >  			       ifname, strlen(ifname))) {
> > +			char str[SOCKADDR_STRLEN];
> > +
> >  			ret = -errno;
> > -			warn("Can't bind %s socket for port %u to %s, closing",
> > -			     EPOLL_TYPE_STR(proto), port, ifname);
> > +			warn("Can't bind %s socket for %s to %s, closing",
> > +			     EPOLL_TYPE_STR(proto),
> > +			     sockaddr_ntop(sa, str, sizeof(str)), ifname);
> >  			close(fd);
> >  			return ret;
> >  		}
> > @@ -178,6 +142,59 @@ int sock_l4(const struct ctx *c, sa_family_t af, uint8_t proto,
> >  
> >  	return fd;
> >  }
> > +/**
> > + * sock_l4() - Create and bind socket for given L4, add to epoll list
> > + * @c:		Execution context
> > + * @af:		Address family, AF_INET or AF_INET6
> > + * @proto:	Protocol number
> > + * @bind_addr:	Address for binding, NULL for any
> > + * @ifname:	Interface for binding, NULL for any
> > + * @port:	Port, host order
> > + * @data:	epoll reference portion for protocol handlers
> > + *
> > + * Return: newly created socket, negative error code on failure
> > + */
> > +int sock_l4(const struct ctx *c, sa_family_t af, uint8_t proto,
> > +	    const void *bind_addr, const char *ifname, uint16_t port,
> > +	    uint32_t data)
> > +{
> > +	switch (af) {
> > +	case AF_INET: {
> > +		struct sockaddr_in addr4 = {
> > +			.sin_family = AF_INET,
> > +			.sin_port = htons(port),
> > +			{ 0 }, { 0 },
> > +		};
> > +		if (bind_addr)
> > +			addr4.sin_addr = *(struct in_addr *)bind_addr;
> > +		return sock_l4_sa(c, proto, &addr4, sizeof(addr4), ifname,
> > +				  false, data);
> > +	}
> > +
> > +	case AF_UNSPEC:
> > +		if (!DUAL_STACK_SOCKETS || bind_addr)
> > +			 return -EINVAL;
> > +		/* fallthrough */
> > +	case AF_INET6: {
> > +		struct sockaddr_in6 addr6 = {
> > +			.sin6_family = AF_INET6,
> > +			.sin6_port = htons(port),
> > +			0, IN6ADDR_ANY_INIT, 0,
> > +		};
> > +		if (bind_addr) {
> > +			addr6.sin6_addr = *(struct in6_addr *)bind_addr;
> > +
> > +			if (!memcmp(bind_addr, &c->ip6.addr_ll,
> > +			    sizeof(c->ip6.addr_ll)))
> > +				addr6.sin6_scope_id = c->ifi6;
> > +		}
> > +		return sock_l4_sa(c, proto, &addr6, sizeof(addr6), ifname,
> > +				  af == AF_INET6, data);
> > +	}
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> >  
> >  /**
> >   * sock_probe_mem() - Check if setting high SO_SNDBUF and SO_RCVBUF is allowed
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]