From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202510 header.b=Yupk/MiW; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 8E2CC5A026F for ; Mon, 13 Oct 2025 06:47:26 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202510; t=1760330844; bh=V+dERWd5TcQHYUpCLhYrtMLyoKATtfvppGzNSLCm9OU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Yupk/MiWjFf/NxGC0hYG8JofG9PHFTTUJSQsiywgRIVv137bnSuxi7UD/I33+mgO+ VLqYJjU2GnV8uuXa4bQzIj1qLIvOTw5XdLc38YGbZQuT91VqhrB2N/LFDrUwKrSB77 uJjI0MgmVGL7tbz922VLRLkqcqsE1zTuA+BYVTYZi5cwO1Mqt76LebShQjaiRr3E/H grQMuCtX7AyAlR5hUsN1AUdXEIThhxfhM9aebKOa1smY1ZmzcDMdec6h1P1YOMu/xt pWpQVtLcieQjPuD5SnUo2JDc9DXulsJ0GdHScJiNLAGPDHYJKQbYuhIVdzeGA0IshZ xKrXmcH8qZtIQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4clPwD3hGCz4wCc; Mon, 13 Oct 2025 15:47:24 +1100 (AEDT) Date: Mon, 13 Oct 2025 15:47:23 +1100 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH v3 3/6] util: Move epoll registration out of sock_l4_sa() Message-ID: References: <20251009130409.3931795-1-lvivier@redhat.com> <20251009130409.3931795-4-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="OnZjAZ9epOSvRxXO" Content-Disposition: inline In-Reply-To: <20251009130409.3931795-4-lvivier@redhat.com> Message-ID-Hash: OLLM4QHA52EONVSAT7UVVK47POS6PEJD X-Message-ID-Hash: OLLM4QHA52EONVSAT7UVVK47POS6PEJD X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --OnZjAZ9epOSvRxXO Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Oct 09, 2025 at 03:04:06PM +0200, Laurent Vivier wrote: > Move epoll_add() calls from sock_l4_sa() to the protocol-specific code > (icmp.c, pif.c, udp_flow.c) to give callers more control over epoll > registration. This allows sock_l4_sa() to focus solely on socket > creation and binding, while epoll management happens at a higher level. >=20 > Remove the data parameter from sock_l4_sa() and flowside_sock_l4() as > it's no longer needed - callers now construct the full epoll_ref and > register the socket themselves after creation. >=20 > Signed-off-by: Laurent Vivier Reviewed-by: David Gibson > --- > flow.c | 10 ++++------ > flow.h | 2 +- > icmp.c | 15 +++++++++++---- > pif.c | 32 +++++++++++++++++++++++++------- > udp_flow.c | 16 ++++++++++++++-- > util.c | 10 +--------- > util.h | 2 +- > 7 files changed, 57 insertions(+), 30 deletions(-) >=20 > diff --git a/flow.c b/flow.c > index feefda3ce74e..b14e9d8b63ff 100644 > --- a/flow.c > +++ b/flow.c > @@ -163,7 +163,6 @@ static void flowside_from_af(struct flowside *side, s= a_family_t af, > * @type: Socket epoll type > * @sa: Socket address > * @sl: Length of @sa > - * @data: epoll reference data > */ > struct flowside_sock_args { > const struct ctx *c; > @@ -173,7 +172,6 @@ struct flowside_sock_args { > const struct sockaddr *sa; > socklen_t sl; > const char *path; > - uint32_t data; > }; > =20 > /** flowside_sock_splice() - Create and bind socket for PIF_SPLICE based= on flowside > @@ -188,7 +186,7 @@ static int flowside_sock_splice(void *arg) > ns_enter(a->c); > =20 > a->fd =3D sock_l4_sa(a->c, a->type, a->sa, a->sl, NULL, > - a->sa->sa_family =3D=3D AF_INET6, a->data); > + a->sa->sa_family =3D=3D AF_INET6); > a->err =3D errno; > =20 > return 0; > @@ -205,7 +203,7 @@ static int flowside_sock_splice(void *arg) > * (if specified). > */ > int flowside_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t = pif, > - const struct flowside *tgt, uint32_t data) > + const struct flowside *tgt) > { > const char *ifname =3D NULL; > union sockaddr_inany sa; > @@ -225,12 +223,12 @@ int flowside_sock_l4(const struct ctx *c, enum epol= l_type type, uint8_t pif, > ifname =3D c->ip6.ifname_out; > =20 > return sock_l4_sa(c, type, &sa, sl, ifname, > - sa.sa_family =3D=3D AF_INET6, data); > + sa.sa_family =3D=3D AF_INET6); > =20 > case PIF_SPLICE: { > struct flowside_sock_args args =3D { > .c =3D c, .type =3D type, > - .sa =3D &sa.sa, .sl =3D sl, .data =3D data, > + .sa =3D &sa.sa, .sl =3D sl, > }; > NS_CALL(flowside_sock_splice, &args); > errno =3D args.err; > diff --git a/flow.h b/flow.h > index cac618ad0ca1..ef138b83add8 100644 > --- a/flow.h > +++ b/flow.h > @@ -167,7 +167,7 @@ static inline bool flowside_eq(const struct flowside = *left, > } > =20 > int flowside_sock_l4(const struct ctx *c, enum epoll_type type, uint8_t = pif, > - const struct flowside *tgt, uint32_t data); > + const struct flowside *tgt); > int flowside_connect(const struct ctx *c, int s, > uint8_t pif, const struct flowside *tgt); > =20 > diff --git a/icmp.c b/icmp.c > index c26561da80bf..56dfac6c958e 100644 > --- a/icmp.c > +++ b/icmp.c > @@ -170,10 +170,10 @@ static struct icmp_ping_flow *icmp_ping_new(const s= truct ctx *c, > { > uint8_t proto =3D af =3D=3D AF_INET ? IPPROTO_ICMP : IPPROTO_ICMPV6; > uint8_t flowtype =3D af =3D=3D AF_INET ? FLOW_PING4 : FLOW_PING6; > - union epoll_ref ref =3D { .type =3D EPOLL_TYPE_PING }; > union flow *flow =3D flow_alloc(); > struct icmp_ping_flow *pingf; > const struct flowside *tgt; > + union epoll_ref ref; > =20 > if (!flow) > return NULL; > @@ -194,9 +194,7 @@ static struct icmp_ping_flow *icmp_ping_new(const str= uct ctx *c, > =20 > pingf->seq =3D -1; > =20 > - ref.flowside =3D FLOW_SIDX(flow, TGTSIDE); > - pingf->sock =3D flowside_sock_l4(c, EPOLL_TYPE_PING, PIF_HOST, > - tgt, ref.data); > + pingf->sock =3D flowside_sock_l4(c, EPOLL_TYPE_PING, PIF_HOST, tgt); > =20 > if (pingf->sock < 0) { > warn("Cannot open \"ping\" socket. You might need to:"); > @@ -208,6 +206,15 @@ static struct icmp_ping_flow *icmp_ping_new(const st= ruct ctx *c, > if (pingf->sock > FD_REF_MAX) > goto cancel; > =20 > + ref.type =3D EPOLL_TYPE_PING; > + ref.flowside =3D FLOW_SIDX(flow, TGTSIDE); > + ref.fd =3D pingf->sock; > + > + if (epoll_add(c->epollfd, EPOLLIN, &ref) < 0) { > + close(pingf->sock); > + goto cancel; > + } > + > flow_dbg(pingf, "new socket %i for echo ID %"PRIu16, pingf->sock, id); > =20 > flow_hash_insert(c, FLOW_SIDX(pingf, INISIDE)); > diff --git a/pif.c b/pif.c > index 592fafaab58a..17b41705183c 100644 > --- a/pif.c > +++ b/pif.c > @@ -5,6 +5,7 @@ > * Passt/pasta interface types and IDs > */ > =20 > +#include > #include > #include > #include > @@ -14,7 +15,7 @@ > #include "siphash.h" > #include "ip.h" > #include "inany.h" > -#include "passt.h" > +#include "epoll_ctl.h" > =20 > const char *pif_type_str[] =3D { > [PIF_NONE] =3D "", > @@ -83,7 +84,9 @@ int pif_sock_l4(const struct ctx *c, enum epoll_type ty= pe, uint8_t pif, > .sa6.sin6_addr =3D in6addr_any, > .sa6.sin6_port =3D htons(port), > }; > + union epoll_ref ref; > socklen_t sl; > + int ret; > =20 > ASSERT(pif_is_socket(pif)); > =20 > @@ -93,11 +96,26 @@ int pif_sock_l4(const struct ctx *c, enum epoll_type = type, uint8_t pif, > ASSERT(addr && inany_is_loopback(addr)); > } > =20 > - if (!addr) > - return sock_l4_sa(c, type, &sa, sizeof(sa.sa6), > - ifname, false, data); > + if (!addr) { > + ref.fd =3D sock_l4_sa(c, type, &sa, sizeof(sa.sa6), > + ifname, false); > + } else { > + pif_sockaddr(c, &sa, &sl, pif, addr, port); > + ref.fd =3D sock_l4_sa(c, type, &sa, sl, > + ifname, sa.sa_family =3D=3D AF_INET6); > + } > + > + if (ref.fd < 0) > + return ref.fd; > + > + ref.type =3D type; > + ref.data =3D data; > + > + ret =3D epoll_add(c->epollfd, EPOLLIN, &ref); > + if (ret < 0) { > + close(ref.fd); > + return ret; > + } > =20 > - pif_sockaddr(c, &sa, &sl, pif, addr, port); > - return sock_l4_sa(c, type, &sa, sl, > - ifname, sa.sa_family =3D=3D AF_INET6, data); > + return ref.fd; > } > diff --git a/udp_flow.c b/udp_flow.c > index d9c75f1bb1d8..caaf3dababb1 100644 > --- a/udp_flow.c > +++ b/udp_flow.c > @@ -78,16 +78,28 @@ static int udp_flow_sock(const struct ctx *c, > flow_sidx_t sidx; > uint32_t data; > } fref =3D { .sidx =3D FLOW_SIDX(uflow, sidei) }; > + union epoll_ref ref; > + int rc; > int s; > =20 > - s =3D flowside_sock_l4(c, EPOLL_TYPE_UDP, pif, side, fref.data); > + s =3D flowside_sock_l4(c, EPOLL_TYPE_UDP, pif, side); > if (s < 0) { > flow_dbg_perror(uflow, "Couldn't open flow specific socket"); > return s; > } > =20 > + ref.type =3D EPOLL_TYPE_UDP; > + ref.data =3D fref.data; > + ref.fd =3D s; > + > + rc =3D epoll_add(c->epollfd, EPOLLIN, &ref); > + if (rc < 0) { > + close(s); > + return rc; > + } > + > if (flowside_connect(c, s, pif, side) < 0) { > - int rc =3D -errno; > + rc =3D -errno; > =20 > epoll_del(c->epollfd, s); > close(s); > diff --git a/util.c b/util.c > index b2490123590a..707ab3388d13 100644 > --- a/util.c > +++ b/util.c > @@ -47,16 +47,14 @@ > * @sl: Length of @sa > * @ifname: Interface for binding, NULL for any > * @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_sa(const struct ctx *c, enum epoll_type type, > const void *sa, socklen_t sl, > - const char *ifname, bool v6only, uint32_t data) > + const char *ifname, bool v6only) > { > sa_family_t af =3D ((const struct sockaddr *)sa)->sa_family; > - union epoll_ref ref =3D { .type =3D type, .data =3D data }; > bool freebind =3D false; > int fd, y =3D 1, ret; > uint8_t proto; > @@ -99,8 +97,6 @@ int sock_l4_sa(const struct ctx *c, enum epoll_type typ= e, > return -EBADF; > } > =20 > - ref.fd =3D fd; > - > if (v6only) > if (setsockopt(fd, IPPROTO_IPV6, IPV6_V6ONLY, &y, sizeof(y))) > debug("Failed to set IPV6_V6ONLY on socket %i", fd); > @@ -171,10 +167,6 @@ int sock_l4_sa(const struct ctx *c, enum epoll_type = type, > return ret; > } > =20 > - ret =3D epoll_add(c->epollfd, EPOLLIN, &ref); > - if (ret < 0) > - return ret; > - > return fd; > } > =20 > diff --git a/util.h b/util.h > index 8e4b4c5c6032..9108f8f19e14 100644 > --- a/util.h > +++ b/util.h > @@ -207,7 +207,7 @@ struct ctx; > =20 > int sock_l4_sa(const struct ctx *c, enum epoll_type type, > const void *sa, socklen_t sl, > - const char *ifname, bool v6only, uint32_t data); > + const char *ifname, bool v6only); > int sock_unix(char *sock_path); > void sock_probe_mem(struct ctx *c); > long timespec_diff_ms(const struct timespec *a, const struct timespec *b= ); > --=20 > 2.50.1 >=20 --=20 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 --OnZjAZ9epOSvRxXO Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmjshFoACgkQzQJF27ox 2Gew7RAAoYxUAjWmB8fbnwji8UIqtMmkF7VY7tC3/ChicafeOTAIW6SNy1t0FBtZ S+T7sb7q7+zDe2vcDXKULVLMKsVknki4OiufkS3YYrLxvdYk5TLatvjdiSZomskj W4HCAvzftCxgDpArAzEv+nZu5YMvtomqjl2PM0rNqwjbPRtbAT2hn5XTvxYEXCT4 cW22nBQ+I/da0/fRkP8E8s7upX1Moh0oosW5hKaATt/zsUg4dJFonFwq0i/y58f1 JtuCdvW3tFwLwxjVq9aL/buewSg32ZcFcVAP6vGYKz4MIVuE2bW9jY8nXCd/sQOU /EywGzK5qnLWF5dw+pgEE/yZ123h57jDz+OBVyTuQDHpM/AUtUc8bcQo0Jsyii8I nWYITc++yX+FZmg4ahWsjdh2CaoxrjIoHgbXxK/3CrKXLvWm07Dca+85kR8hVU5l gNZQNtgh84RPrXuzkz+2y04znKw+lY1B4mlGJXASgUXsXaAW4YDE9Y+ssMxNtMCq aSKW3bRLAddcWOEzR65bw1APajWzmEMZkYZB9FTCjb/uWRjm1TMwrBOOKv0aomVa fRkPhrOatrB7erYQ1n3fcCAawwKhS9M8v3uWX5OS4cKmFRjtZC64gi2TCAdHcsV8 eoMbSQ0JvUVG+3VDi7XmFNuz6LIThDzg5Z6108rOLNJ9DzpPISw= =mtgd -----END PGP SIGNATURE----- --OnZjAZ9epOSvRxXO--