[M] Change in libosmocore[master]: socket: Reimplement osmo_sock_init2_multiaddr()
pespin has submitted this change. (
https://gerrit.osmocom.org/c/libosmocore/+/35232?usp=email )
Change subject: socket: Reimplement osmo_sock_init2_multiaddr()
..
socket: Reimplement osmo_sock_init2_multiaddr()
This is an attempt to fix several downsides of current
osmo_sock_init2_multiaddr() API, mainly the requirement to pass an explicit
local address (!NULL). It also now works fine if OSMO_SOCK_F_BIND flag
is not used.
This reimplementation is based on the follwing logic:
- If caller passed family=AF_INET or family=AF_INET6, that same family
is used and kernel will fail if something is wrong.
- If caller passes family=AF_UNSPEC, the function will try to find the
required family to create the socket. The decision is taken on the
assumption that an AF_INET6 socket can handle both AF_INET6 and AF_INET
addresses (through v4v6 mapping). Hence, if any of the addresses in the
local or remote set of addresses resolves through getaddrinfo() to an
IPv6 address, then AF_INET6 is used; AF_INET is used otherwise.
Related: OS#6279
Change-Id: I2641fbaca6f477404b094dbc53c0c1a3dd3fd2fd
---
M src/core/socket.c
1 file changed, 128 insertions(+), 44 deletions(-)
Approvals:
Jenkins Builder: Verified
osmith: Looks good to me, approved
diff --git a/src/core/socket.c b/src/core/socket.c
index 1dc8e46..0bc275b 100644
--- a/src/core/socket.c
+++ b/src/core/socket.c
@@ -616,18 +616,45 @@
#ifdef HAVE_LIBSCTP
-/* Check whether there's an IPv6 Addr as first option of any addrinfo item in
the addrinfo set */
+/* Check whether there's an addrinfo item in the addrinfo set with an IPv4 or
IPv6 option */
static void addrinfo_has_v4v6addr(const struct addrinfo **result, size_t
result_count, bool *has_v4, bool *has_v6)
{
size_t host_idx;
+ const struct addrinfo *rp;
*has_v4 = false;
*has_v6 = false;
for (host_idx = 0; host_idx < result_count; host_idx++) {
- if (result[host_idx]->ai_family == AF_INET)
- *has_v4 = true;
- else if (result[host_idx]->ai_family == AF_INET6)
- *has_v6 = true;
+ for (rp = result[host_idx]; rp != NULL; rp = rp->ai_next) {
+ if (result[host_idx]->ai_family == AF_INET)
+ *has_v4 = true;
+ else if (result[host_idx]->ai_family == AF_INET6)
+ *has_v6 = true;
+ }
+ }
+}
+
+/* Check whether there's an addrinfo item in the addrinfo set with only an
IPv4 or IPv6 option */
+static void addrinfo_has_v4v6only_addr(const struct addrinfo **result, size_t
result_count, bool *has_v4only, bool *has_v6only)
+{
+ size_t host_idx;
+ const struct addrinfo *rp;
+ *has_v4only = false;
+ *has_v6only = false;
+
+ for (host_idx = 0; host_idx < result_count; host_idx++) {
+ bool has_v4 = false;
+ bool has_v6 = false;
+ for (rp = result[host_idx]; rp != NULL; rp = rp->ai_next) {
+ if (rp->ai_family == AF_INET6)
+ has_v6 = true;
+ else
+ has_v4 = true;
+ }
+ if (has_v4 && !has_v6)
+ *has_v4only = true;
+ else if (has_v6 && !has_v4)
+ *has_v6only = true;
}
}
@@ -636,13 +663,16 @@
{
size_t host_idx;
struct in6_addr in6addr_any = IN6ADDR_ANY_INIT;
+ const struct addrinfo *rp;
for (host_idx = 0; host_idx < result_count; host_idx++) {
- if (result[host_idx]->ai_family != AF_INET6)
- continue;
- if (memcmp(&((struct sockaddr_in6
*)result[host_idx]->ai_addr)->sin6_addr,
- _any, sizeof(in6addr_any)) == 0)
- return true;
+ for (rp = result[host_idx]; rp != NULL; rp = rp->ai_next) {
+ if (rp->ai_family != AF_INET6)
+ continue;
+ if (memcmp(&((struct sockaddr_in6
*)rp->ai_addr)->sin6_addr,
+ _any, sizeof(in6addr_any)) == 0)
+ return true;
+ }
}
return false;
}
@@ -676,22 +706,28 @@
for (host_idx = 0; host_idx < host_cont; host_idx++) {
/* Addresses are ordered based on RFC 3484, see man getaddrinfo
*/
for (rp = result[host_idx]; rp != NULL; rp = rp->ai_next) {
- if (family != AF_UNSPEC && rp->ai_family != family)
- continue;
- if (offset + rp->ai_addrlen > addrs_buf_len) {
- LOGP(DLGLOBAL, LOGL_ERROR, "Output buffer to
small: %zu\n",
-addrs_buf_len);
-
[M] Change in libosmocore[master]: socket: Reimplement osmo_sock_init2_multiaddr()
Attention is currently required from: laforge, pespin.
osmith has posted comments on this change. (
https://gerrit.osmocom.org/c/libosmocore/+/35232?usp=email )
Change subject: socket: Reimplement osmo_sock_init2_multiaddr()
..
Patch Set 3: Code-Review+2
(3 comments)
Patchset:
PS3:
re-applying previous 1+1 after comment fix
File src/core/socket.c:
https://gerrit.osmocom.org/c/libosmocore/+/35232/comment/7a4070cf_38760574
PS2, Line 933: * Both are checked here through "or" here to account for "bind
flag set,
> I'm talking about OSMO_SOCK_F_BIND or OSMO_SOCK_F_CONNECT flags here. […]
Ack, now it makes sense.
https://gerrit.osmocom.org/c/libosmocore/+/35232/comment/9e98c74d_f0bf49fc
PS2, Line 945: (loc_has_v4addr != rem_has_v4addr || loc_has_v6addr !=
rem_has_v6addr)) {
> I thought about it, but in the end writing it this way is the easiest way of
> writing it, instead of […]
Okay
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35232?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I2641fbaca6f477404b094dbc53c0c1a3dd3fd2fd
Gerrit-Change-Number: 35232
Gerrit-PatchSet: 3
Gerrit-Owner: pespin
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge
Gerrit-Reviewer: osmith
Gerrit-Attention: laforge
Gerrit-Attention: pespin
Gerrit-Comment-Date: Wed, 06 Dec 2023 13:38:21 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: osmith
Comment-In-Reply-To: pespin
Gerrit-MessageType: comment
[M] Change in libosmocore[master]: socket: Reimplement osmo_sock_init2_multiaddr()
Attention is currently required from: laforge, osmith. Hello Jenkins Builder, laforge, osmith, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmocore/+/35232?usp=email to look at the new patch set (#3). The following approvals got outdated and were removed: Code-Review+1 by laforge, Code-Review+1 by osmith, Verified+1 by Jenkins Builder Change subject: socket: Reimplement osmo_sock_init2_multiaddr() .. socket: Reimplement osmo_sock_init2_multiaddr() This is an attempt to fix several downsides of current osmo_sock_init2_multiaddr() API, mainly the requirement to pass an explicit local address (!NULL). It also now works fine if OSMO_SOCK_F_BIND flag is not used. This reimplementation is based on the follwing logic: - If caller passed family=AF_INET or family=AF_INET6, that same family is used and kernel will fail if something is wrong. - If caller passes family=AF_UNSPEC, the function will try to find the required family to create the socket. The decision is taken on the assumption that an AF_INET6 socket can handle both AF_INET6 and AF_INET addresses (through v4v6 mapping). Hence, if any of the addresses in the local or remote set of addresses resolves through getaddrinfo() to an IPv6 address, then AF_INET6 is used; AF_INET is used otherwise. Related: OS#6279 Change-Id: I2641fbaca6f477404b094dbc53c0c1a3dd3fd2fd --- M src/core/socket.c 1 file changed, 128 insertions(+), 44 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/32/35232/3 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35232?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I2641fbaca6f477404b094dbc53c0c1a3dd3fd2fd Gerrit-Change-Number: 35232 Gerrit-PatchSet: 3 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: osmith Gerrit-Attention: osmith Gerrit-Attention: laforge Gerrit-MessageType: newpatchset
[M] Change in libosmocore[master]: socket: Reimplement osmo_sock_init2_multiaddr()
Attention is currently required from: osmith.
pespin has posted comments on this change. (
https://gerrit.osmocom.org/c/libosmocore/+/35232?usp=email )
Change subject: socket: Reimplement osmo_sock_init2_multiaddr()
..
Patch Set 2:
(3 comments)
File src/core/socket.c:
https://gerrit.osmocom.org/c/libosmocore/+/35232/comment/9eb46716_ec2e3ae6
PS2, Line 922: IPv4 or IPv6
> IPv4-only or IPv6-only
Ack
https://gerrit.osmocom.org/c/libosmocore/+/35232/comment/a8a8901b_f66bfa2a
PS2, Line 933: * Both are checked here through "or" here to account for "bind
flag set,
> what do you mean with 'checked through "or" here', did you mean to write it
> like this? […]
I'm talking about OSMO_SOCK_F_BIND or OSMO_SOCK_F_CONNECT flags here.
If for instance OSMO_SOCK_F_BIND is not set, rem_has_v6addr needs checking, and
if OSMO_SOCK_F_CONNECT is not set, loc_has_v6addr needs checking. If both are
set, both need checking.
https://gerrit.osmocom.org/c/libosmocore/+/35232/comment/0e84c9ea_7790b3dc
PS2, Line 945: (loc_has_v4addr != rem_has_v4addr || loc_has_v6addr !=
rem_has_v6addr)) {
> with multiple return paths I mean, to potentially make it more readable
I thought about it, but in the end writing it this way is the easiest way of
writing it, instead of writing lots of cases. I think the comment already hints
it.
My point is that no matter how you write it, it's going to need some thinking
when reading it.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35232?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I2641fbaca6f477404b094dbc53c0c1a3dd3fd2fd
Gerrit-Change-Number: 35232
Gerrit-PatchSet: 2
Gerrit-Owner: pespin
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge
Gerrit-Reviewer: osmith
Gerrit-Attention: osmith
Gerrit-Comment-Date: Wed, 06 Dec 2023 12:35:11 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: osmith
Gerrit-MessageType: comment
[M] Change in libosmocore[master]: socket: Reimplement osmo_sock_init2_multiaddr()
Attention is currently required from: pespin.
osmith has posted comments on this change. (
https://gerrit.osmocom.org/c/libosmocore/+/35232?usp=email )
Change subject: socket: Reimplement osmo_sock_init2_multiaddr()
..
Patch Set 2:
(1 comment)
File src/core/socket.c:
https://gerrit.osmocom.org/c/libosmocore/+/35232/comment/97f6d9df_6d77f3dc
PS2, Line 945: (loc_has_v4addr != rem_has_v4addr || loc_has_v6addr !=
rem_has_v6addr)) {
> the if condition looks right, but is IMHO a bit hard to read, maybe move it /
> or the whole block her […]
with multiple return paths I mean, to potentially make it more readable
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35232?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I2641fbaca6f477404b094dbc53c0c1a3dd3fd2fd
Gerrit-Change-Number: 35232
Gerrit-PatchSet: 2
Gerrit-Owner: pespin
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge
Gerrit-Reviewer: osmith
Gerrit-Attention: pespin
Gerrit-Comment-Date: Wed, 06 Dec 2023 07:32:05 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: osmith
Gerrit-MessageType: comment
[M] Change in libosmocore[master]: socket: Reimplement osmo_sock_init2_multiaddr()
Attention is currently required from: pespin.
osmith has posted comments on this change. (
https://gerrit.osmocom.org/c/libosmocore/+/35232?usp=email )
Change subject: socket: Reimplement osmo_sock_init2_multiaddr()
..
Patch Set 2: Code-Review+1
(4 comments)
Patchset:
PS2:
logic looks fine, just some cosmetics
File src/core/socket.c:
https://gerrit.osmocom.org/c/libosmocore/+/35232/comment/707ab214_70e3d9a9
PS2, Line 922: IPv4 or IPv6
IPv4-only or IPv6-only
https://gerrit.osmocom.org/c/libosmocore/+/35232/comment/c4fb0cd8_50b0b67d
PS2, Line 933: * Both are checked here through "or" here to account for "bind
flag set,
what do you mean with 'checked through "or" here', did you mean to write it
like this?
```
if (loc_has_v6addr || rem_has_v6addr)
family = AF_INET6;
else
family = AF_INET;
```
https://gerrit.osmocom.org/c/libosmocore/+/35232/comment/1f5f1437_11cdfece
PS2, Line 945: (loc_has_v4addr != rem_has_v4addr || loc_has_v6addr !=
rem_has_v6addr)) {
the if condition looks right, but is IMHO a bit hard to read, maybe move it /
or the whole block here to a separate function?
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35232?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I2641fbaca6f477404b094dbc53c0c1a3dd3fd2fd
Gerrit-Change-Number: 35232
Gerrit-PatchSet: 2
Gerrit-Owner: pespin
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge
Gerrit-Reviewer: osmith
Gerrit-Attention: pespin
Gerrit-Comment-Date: Wed, 06 Dec 2023 07:26:48 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
[M] Change in libosmocore[master]: socket: Reimplement osmo_sock_init2_multiaddr()
Attention is currently required from: pespin. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35232?usp=email ) Change subject: socket: Reimplement osmo_sock_init2_multiaddr() .. Patch Set 2: Code-Review+1 (1 comment) Patchset: PS2: lgtm, thanks! -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35232?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I2641fbaca6f477404b094dbc53c0c1a3dd3fd2fd Gerrit-Change-Number: 35232 Gerrit-PatchSet: 2 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Attention: pespin Gerrit-Comment-Date: Tue, 05 Dec 2023 20:24:33 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
[M] Change in libosmocore[master]: socket: Reimplement osmo_sock_init2_multiaddr()
Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmocore/+/35232?usp=email to look at the new patch set (#2). Change subject: socket: Reimplement osmo_sock_init2_multiaddr() .. socket: Reimplement osmo_sock_init2_multiaddr() This is an attempt to fix several downsides of current osmo_sock_init2_multiaddr() API, mainly the requirement to pass an explicit local address (!NULL). It also now works fine if OSMO_SOCK_F_BIND flag is not used. This reimplementation is based on the follwing logic: - If caller passed family=AF_INET or family=AF_INET6, that same family is used and kernel will fail if something is wrong. - If caller passes family=AF_UNSPEC, the function will try to find the required family to create the socket. The decision is taken on the assumption that an AF_INET6 socket can handle both AF_INET6 and AF_INET addresses (through v4v6 mapping). Hence, if any of the addresses in the local or remote set of addresses resolves through getaddrinfo() to an IPv6 address, then AF_INET6 is used; AF_INET is used otherwise. Related: OS#6279 Change-Id: I2641fbaca6f477404b094dbc53c0c1a3dd3fd2fd --- M src/core/socket.c 1 file changed, 128 insertions(+), 44 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/32/35232/2 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35232?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I2641fbaca6f477404b094dbc53c0c1a3dd3fd2fd Gerrit-Change-Number: 35232 Gerrit-PatchSet: 2 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-MessageType: newpatchset
