From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id AACFF5A027B for ; Wed, 8 Mar 2023 23:45:16 +0100 (CET) Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4PX6pF6bPVz4x5R; Thu, 9 Mar 2023 09:45:09 +1100 (AEDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=201602; t=1678315509; bh=Dvl/1pez3CiqYaPUemsMljRR3/efY/jq9FJXWgczKWE=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=LKvI4Z71TBUF7hjknv3dlGawLYlipl8+UtocdwjrZciYe4xV3aNoJxZciJnqpdumw 9zAHZlADjffe1J5hgqX7q51+uN3ULxdrWNs+jNmn8IV/nWssGrhJsUDmeUMJSqO2Hs Nb2hRXXj4nXANqgGLRoMTkLO8LARkURrkueLpiQ8= Date: Thu, 9 Mar 2023 09:43:37 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 2/3] tcp, udp: Fix partial success return codes in {tcp,udp}_sock_init() Message-ID: References: <20230308123348.2232214-1-sbrivio@redhat.com> <20230308123348.2232214-3-sbrivio@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="S1RFZJ/GdGWdmMPq" Content-Disposition: inline In-Reply-To: <20230308123348.2232214-3-sbrivio@redhat.com> Message-ID-Hash: GY6WMK5473JMQS6MXRMYTXDEX5L33JC6 X-Message-ID-Hash: GY6WMK5473JMQS6MXRMYTXDEX5L33JC6 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: --S1RFZJ/GdGWdmMPq Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Mar 08, 2023 at 01:33:47PM +0100, Stefano Brivio wrote: > The comments say we should return 0 on partial success, and an error > code on complete failure. Rationale: if the user configures a port > forwarding, and we succeed to bind that port for IPv4 or IPv6 only, > that might actually be what the user intended. >=20 > Adjust the two functions to reflect the comments. >=20 > Signed-off-by: Stefano Brivio Reviewed-by: David Gibson > --- > tcp.c | 21 +++++++++------------ > udp.c | 30 ++++++++++++++---------------- > 2 files changed, 23 insertions(+), 28 deletions(-) >=20 > diff --git a/tcp.c b/tcp.c > index e209483..a29e387 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -2993,7 +2993,7 @@ static int tcp_sock_init_af(const struct ctx *c, in= t af, in_port_t port, > int tcp_sock_init(const struct ctx *c, sa_family_t af, const void *addr, > const char *ifname, in_port_t port) > { > - int ret =3D 0, af_ret; > + int r4 =3D SOCKET_MAX + 1, r6 =3D SOCKET_MAX + 1; > =20 > if (af =3D=3D AF_UNSPEC && c->ifi4 && c->ifi6) > /* Attempt to get a dual stack socket */ > @@ -3001,19 +3001,16 @@ int tcp_sock_init(const struct ctx *c, sa_family_= t af, const void *addr, > return 0; > =20 > /* Otherwise create a socket per IP version */ > - if ((af =3D=3D AF_INET || af =3D=3D AF_UNSPEC) && c->ifi4) { > - af_ret =3D tcp_sock_init_af(c, AF_INET, port, addr, ifname); > - if (af_ret < 0) > - ret =3D af_ret; > - } > + if ((af =3D=3D AF_INET || af =3D=3D AF_UNSPEC) && c->ifi4) > + r4 =3D tcp_sock_init_af(c, AF_INET, port, addr, ifname); > =20 > - if ((af =3D=3D AF_INET6 || af =3D=3D AF_UNSPEC) && c->ifi6) { > - af_ret =3D tcp_sock_init_af(c, AF_INET6, port, addr, ifname); > - if (af_ret < 0) > - ret =3D af_ret; > - } > + if ((af =3D=3D AF_INET6 || af =3D=3D AF_UNSPEC) && c->ifi6) > + r6 =3D tcp_sock_init_af(c, AF_INET6, port, addr, ifname); > =20 > - return ret; > + if (IN_INTERVAL(0, SOCKET_MAX, r4) || IN_INTERVAL(0, SOCKET_MAX, r6)) > + return 0; > + > + return r4 < 0 ? r4 : r6; > } > =20 > /** > diff --git a/udp.c b/udp.c > index 41e0afd..a5f7537 100644 > --- a/udp.c > +++ b/udp.c > @@ -983,7 +983,7 @@ int udp_sock_init(const struct ctx *c, int ns, sa_fam= ily_t af, > const void *addr, const char *ifname, in_port_t port) > { > union udp_epoll_ref uref =3D { .u32 =3D 0 }; > - int s, ret =3D 0; > + int s, r4 =3D SOCKET_MAX + 1, r6 =3D SOCKET_MAX + 1; > =20 > if (ns) { > uref.udp.port =3D (in_port_t)(port + > @@ -999,8 +999,8 @@ int udp_sock_init(const struct ctx *c, int ns, sa_fam= ily_t af, > uref.udp.orig =3D true; > =20 > if (!ns) { > - s =3D sock_l4(c, AF_INET, IPPROTO_UDP, addr, ifname, > - port, uref.u32); > + r4 =3D s =3D sock_l4(c, AF_INET, IPPROTO_UDP, addr, > + ifname, port, uref.u32); > =20 > udp_tap_map[V4][uref.udp.port].sock =3D s < 0 ? -1 : s; > udp_splice_init[V4][port].sock =3D s < 0 ? -1 : s; > @@ -1008,13 +1008,10 @@ int udp_sock_init(const struct ctx *c, int ns, sa= _family_t af, > struct in_addr loopback =3D { htonl(INADDR_LOOPBACK) }; > uref.udp.ns =3D true; > =20 > - s =3D sock_l4(c, AF_INET, IPPROTO_UDP, &loopback, > - ifname, port, uref.u32); > + r4 =3D s =3D sock_l4(c, AF_INET, IPPROTO_UDP, &loopback, > + ifname, port, uref.u32); > udp_splice_ns[V4][port].sock =3D s < 0 ? -1 : s; > } > - > - if (s < 0) > - ret =3D s; > } > =20 > if ((af =3D=3D AF_INET6 || af =3D=3D AF_UNSPEC) && c->ifi6) { > @@ -1023,24 +1020,25 @@ int udp_sock_init(const struct ctx *c, int ns, sa= _family_t af, > uref.udp.orig =3D true; > =20 > if (!ns) { > - s =3D sock_l4(c, AF_INET6, IPPROTO_UDP, addr, ifname, > - port, uref.u32); > + r6 =3D s =3D sock_l4(c, AF_INET6, IPPROTO_UDP, addr, > + ifname, port, uref.u32); > =20 > udp_tap_map[V6][uref.udp.port].sock =3D s < 0 ? -1 : s; > udp_splice_init[V6][port].sock =3D s < 0 ? -1 : s; > } else { > uref.udp.ns =3D true; > =20 > - s =3D sock_l4(c, AF_INET6, IPPROTO_UDP, &in6addr_loopback, > - ifname, port, uref.u32); > + r6 =3D s =3D sock_l4(c, AF_INET6, IPPROTO_UDP, > + &in6addr_loopback, > + ifname, port, uref.u32); > udp_splice_ns[V6][port].sock =3D s < 0 ? -1 : s; > } > - > - if (s < 0) > - ret =3D s; > } > =20 > - return ret; > + if (IN_INTERVAL(0, SOCKET_MAX, r4) || IN_INTERVAL(0, SOCKET_MAX, r6)) > + return 0; > + > + return r4 < 0 ? r4 : r6; Same comment (unless sock_l4() already looks for=20 > } > =20 > /** --=20 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 --S1RFZJ/GdGWdmMPq Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmQJD5EACgkQzQJF27ox 2GdwRBAApZ3SFGTM9x2/nGug34X8Q36ZCv+sDOwz9uj1cer7cW9szXAYELmPdhSw mLqtHQKVC5yt5Qeig3sR4YpItpTdW1cZWJFfHZ/bwsiiPX7hf0NV6uEvBHBEBEhv QqNiEHDQkzCyo9l1xlIZmPzRR4ZfmoqDYLEAQu8Ti6y/rJCxdRX65ONX7yiTfxcA yiwvXlTG2nW58Jn1GQHWOFspYugFn8R/eYvDuRsxjhyuN/+1wYb2ISrjR8tMUWE+ NFIoBAP2zSqZLp8Uw2UCrUM5NV56vZ+RaIrCuYerfaI6CwFIDHpQ+3z9fcOQfGCU 3j+KyapC+olxermmjbBXmHb9z8N1jPfx4SEHi8OE2SBuOntJJUcRe9tVAopzsqe5 DiXTx+wwzg521NX5b6TG85avT+ZsK6bexzH9oz1tEuWtThg5dzLAR4RS3pUDDzLI XmEKBBuIpoI2bApE56q3jlwOmjCY2RKICu2Q9IAdFMWeTkuS1E9a8nF3PDfDS7H3 zm7LgCDXW5JG4C+iVgAdvsTDvKk0pXVNc9+6Wl/WqdRV1YrdWksGrxQ84nni9sUh cktF3Qe74IsBFOd+PFSMzKxYiPA9Ot4nQj0M5C6/H7e9PO54EbTP3qR4AYgUpQN7 Cy2bfzDDO9bdvsNOqfQyQ5nT9JVzc+IqCfujH4UnZ/dN8AmEju8= =XTME -----END PGP SIGNATURE----- --S1RFZJ/GdGWdmMPq--