[PATCH 1/3] tcp: Merge tcp_ns_sock_init[46]() into tcp_sock_init_one()
Surprisingly little logic is shared between the path for creating a
listen()ing socket in the guest namespace versus in the host namespace.
Improve this, by extending tcp_sock_init_one() to take a pif parameter
indicating where it should open the socket. This allows
tcp_ns_sock_init[46]() to be removed entirely.
We generalise tcp_sock_init() in the same way, although we don't use it
yet, due to some subtle differences in how we bind for -t versus -T.
Signed-off-by: David Gibson
On Fri, 17 Oct 2025 11:34:45 +1100
David Gibson
Surprisingly little logic is shared between the path for creating a listen()ing socket in the guest namespace versus in the host namespace. Improve this, by extending tcp_sock_init_one() to take a pif parameter indicating where it should open the socket. This allows tcp_ns_sock_init[46]() to be removed entirely.
We generalise tcp_sock_init() in the same way, although we don't use it yet, due to some subtle differences in how we bind for -t versus -T.
Signed-off-by: David Gibson
--- conf.c | 2 +- tcp.c | 96 ++++++++++++++++++---------------------------------------- tcp.h | 5 +-- 3 files changed, 33 insertions(+), 70 deletions(-) diff --git a/conf.c b/conf.c index 66b9e634..26f1bcc0 100644 --- a/conf.c +++ b/conf.c @@ -169,7 +169,7 @@ static void conf_ports_range_except(const struct ctx *c, char optname, fwd->delta[i] = to - first;
if (optname == 't') - ret = tcp_sock_init(c, addr, ifname, i); + ret = tcp_sock_init(c, PIF_HOST, addr, ifname, i); else if (optname == 'u') ret = udp_sock_init(c, 0, addr, ifname, i); else diff --git a/tcp.c b/tcp.c index 0f9e9b3f..15c012d7 100644 --- a/tcp.c +++ b/tcp.c @@ -2515,29 +2515,38 @@ void tcp_sock_handler(const struct ctx *c, union epoll_ref ref, /** * tcp_sock_init_one() - Initialise listening socket for address and port * @c: Execution context + * @pif: Interface to open the socket for (PIF_HOST or PIF_SPLICE) * @addr: Pointer to address for binding, NULL for dual stack any * @ifname: Name of interface to bind to, NULL if not configured * @port: Port, host order * * Return: fd for the new listening socket, negative error code on failure + * + * If pif == PIF_SPLICE, must have already entered the namespace. */ -static int tcp_sock_init_one(const struct ctx *c, const union inany_addr *addr, - const char *ifname, in_port_t port) +static int tcp_sock_init_one(const struct ctx *c, uint8_t pif, + const union inany_addr *addr, const char *ifname, + in_port_t port) { + const struct fwd_ports *fwd = pif == PIF_HOST ? + &c->tcp.fwd_in : &c->tcp.fwd_out;
While I appreciate the resulting brevity, I wonder if it would make more sense to have this as an explicit if / else clause, for readability. Same for similar occurrences in the next patches (which I didn't fully review, yet). Another alternative is: const struct fwd_ports *fwd; fwd = (pif == PIF_HOST) ? &c->tcp.fwd_in : &c->tcp.fwd_out; ...still two lines of code, perhaps just slightly less readable than the five obvious ones: const struct fwd_ports *fwd; if (pif == PIF_HOST) fwd = &c->tcp.fwd_in; else fwd = &c->tcp.fwd_out; -- Stefano
On Fri, 17 Oct 2025 11:34:45 +1100
David Gibson
Surprisingly little logic is shared between the path for creating a listen()ing socket in the guest namespace versus in the host namespace. Improve this, by extending tcp_sock_init_one() to take a pif parameter indicating where it should open the socket. This allows tcp_ns_sock_init[46]() to be removed entirely.
We generalise tcp_sock_init() in the same way, although we don't use it yet, due to some subtle differences in how we bind for -t versus -T.
Signed-off-by: David Gibson
--- conf.c | 2 +- tcp.c | 96 ++++++++++++++++++---------------------------------------- tcp.h | 5 +-- 3 files changed, 33 insertions(+), 70 deletions(-) diff --git a/conf.c b/conf.c index 66b9e634..26f1bcc0 100644 --- a/conf.c +++ b/conf.c @@ -169,7 +169,7 @@ static void conf_ports_range_except(const struct ctx *c, char optname, fwd->delta[i] = to - first;
if (optname == 't') - ret = tcp_sock_init(c, addr, ifname, i); + ret = tcp_sock_init(c, PIF_HOST, addr, ifname, i); else if (optname == 'u') ret = udp_sock_init(c, 0, addr, ifname, i); else diff --git a/tcp.c b/tcp.c index 0f9e9b3f..15c012d7 100644 --- a/tcp.c +++ b/tcp.c @@ -2515,29 +2515,38 @@ void tcp_sock_handler(const struct ctx *c, union epoll_ref ref, /** * tcp_sock_init_one() - Initialise listening socket for address and port * @c: Execution context + * @pif: Interface to open the socket for (PIF_HOST or PIF_SPLICE) * @addr: Pointer to address for binding, NULL for dual stack any * @ifname: Name of interface to bind to, NULL if not configured * @port: Port, host order * * Return: fd for the new listening socket, negative error code on failure + * + * If pif == PIF_SPLICE, must have already entered the namespace.
...I forgot: nit: a subject in this sentence would be nice for readability. -- Stefano
On Mon, Oct 20, 2025 at 08:08:39AM +0200, Stefano Brivio wrote:
On Fri, 17 Oct 2025 11:34:45 +1100 David Gibson
wrote: Surprisingly little logic is shared between the path for creating a listen()ing socket in the guest namespace versus in the host namespace. Improve this, by extending tcp_sock_init_one() to take a pif parameter indicating where it should open the socket. This allows tcp_ns_sock_init[46]() to be removed entirely.
We generalise tcp_sock_init() in the same way, although we don't use it yet, due to some subtle differences in how we bind for -t versus -T.
Signed-off-by: David Gibson
--- conf.c | 2 +- tcp.c | 96 ++++++++++++++++++---------------------------------------- tcp.h | 5 +-- 3 files changed, 33 insertions(+), 70 deletions(-) diff --git a/conf.c b/conf.c index 66b9e634..26f1bcc0 100644 --- a/conf.c +++ b/conf.c @@ -169,7 +169,7 @@ static void conf_ports_range_except(const struct ctx *c, char optname, fwd->delta[i] = to - first;
if (optname == 't') - ret = tcp_sock_init(c, addr, ifname, i); + ret = tcp_sock_init(c, PIF_HOST, addr, ifname, i); else if (optname == 'u') ret = udp_sock_init(c, 0, addr, ifname, i); else diff --git a/tcp.c b/tcp.c index 0f9e9b3f..15c012d7 100644 --- a/tcp.c +++ b/tcp.c @@ -2515,29 +2515,38 @@ void tcp_sock_handler(const struct ctx *c, union epoll_ref ref, /** * tcp_sock_init_one() - Initialise listening socket for address and port * @c: Execution context + * @pif: Interface to open the socket for (PIF_HOST or PIF_SPLICE) * @addr: Pointer to address for binding, NULL for dual stack any * @ifname: Name of interface to bind to, NULL if not configured * @port: Port, host order * * Return: fd for the new listening socket, negative error code on failure + * + * If pif == PIF_SPLICE, must have already entered the namespace. */ -static int tcp_sock_init_one(const struct ctx *c, const union inany_addr *addr, - const char *ifname, in_port_t port) +static int tcp_sock_init_one(const struct ctx *c, uint8_t pif, + const union inany_addr *addr, const char *ifname, + in_port_t port) { + const struct fwd_ports *fwd = pif == PIF_HOST ? + &c->tcp.fwd_in : &c->tcp.fwd_out;
While I appreciate the resulting brevity, I wonder if it would make more sense to have this as an explicit if / else clause, for readability. Same for similar occurrences in the next patches (which I didn't fully review, yet).
Another alternative is:
const struct fwd_ports *fwd;
fwd = (pif == PIF_HOST) ? &c->tcp.fwd_in : &c->tcp.fwd_out;
...still two lines of code, perhaps just slightly less readable than the five obvious ones:
const struct fwd_ports *fwd;
if (pif == PIF_HOST) fwd = &c->tcp.fwd_in; else fwd = &c->tcp.fwd_out;
Good point. I suspect this will be shuffled again in later patches, but I might as well go with the less obfuscated version in the meantime. -- David Gibson (he or they) | 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
On Mon, Oct 20, 2025 at 08:09:59AM +0200, Stefano Brivio wrote:
On Fri, 17 Oct 2025 11:34:45 +1100 David Gibson
wrote: Surprisingly little logic is shared between the path for creating a listen()ing socket in the guest namespace versus in the host namespace. Improve this, by extending tcp_sock_init_one() to take a pif parameter indicating where it should open the socket. This allows tcp_ns_sock_init[46]() to be removed entirely.
We generalise tcp_sock_init() in the same way, although we don't use it yet, due to some subtle differences in how we bind for -t versus -T.
Signed-off-by: David Gibson
--- conf.c | 2 +- tcp.c | 96 ++++++++++++++++++---------------------------------------- tcp.h | 5 +-- 3 files changed, 33 insertions(+), 70 deletions(-) diff --git a/conf.c b/conf.c index 66b9e634..26f1bcc0 100644 --- a/conf.c +++ b/conf.c @@ -169,7 +169,7 @@ static void conf_ports_range_except(const struct ctx *c, char optname, fwd->delta[i] = to - first;
if (optname == 't') - ret = tcp_sock_init(c, addr, ifname, i); + ret = tcp_sock_init(c, PIF_HOST, addr, ifname, i); else if (optname == 'u') ret = udp_sock_init(c, 0, addr, ifname, i); else diff --git a/tcp.c b/tcp.c index 0f9e9b3f..15c012d7 100644 --- a/tcp.c +++ b/tcp.c @@ -2515,29 +2515,38 @@ void tcp_sock_handler(const struct ctx *c, union epoll_ref ref, /** * tcp_sock_init_one() - Initialise listening socket for address and port * @c: Execution context + * @pif: Interface to open the socket for (PIF_HOST or PIF_SPLICE) * @addr: Pointer to address for binding, NULL for dual stack any * @ifname: Name of interface to bind to, NULL if not configured * @port: Port, host order * * Return: fd for the new listening socket, negative error code on failure + * + * If pif == PIF_SPLICE, must have already entered the namespace.
...I forgot: nit: a subject in this sentence would be nice for readability.
Noted, will adjust. -- David Gibson (he or they) | 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
participants (2)
-
David Gibson
-
Stefano Brivio