February 2024 - gerrit-log - lists.osmocom.org

[M] Change in libosmo-abis[master]: Fix critical bug in default TCP keepalive user timeout

by laforge

Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmo-abis/+/36079?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Verified+1 by Jenkins Builder Change subject: Fix critical bug in default TCP keepalive user timeout ...................................................................... Fix critical bug in default TCP keepalive user timeout It turns out that our calculation of the TCP_USER_TIMEOUT value was flawed in several ways: * there should have been parenthesis around the + operator (line->keepalive_probe_interval + line->keepalive_idle_timeout) as the keepalive_idle_timeout is in seconds, not milli-seconds. * in the default case, all those values are configured to -1 (E1INP_USE_DEFAULT). This means we're using 1000 * -1 * -1 + -1 = 999 i.e. just below aq second which clearly is not enough for a lossy satellite or wifi back-haul. This fixes a regression introduced in Ia7659c209aea0d26eb37d31e771adc91b17ae668 i.e. present since libosmo-abis 1.4.0 In addition, let's print the actually-used values to the log, helping to spot unintended values. Change-Id: Idca24d3e676a45d860d9eec60dc2097d8d87f3bf Closes: OS#6375 Related: OS#5785, SYS#6801 Fixes: Ia7659c209aea0d26eb37d31e771adc91b17ae668 --- M src/input/ipaccess.c 1 file changed, 48 insertions(+), 17 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmo-abis refs/changes/79/36079/2 -- To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/36079?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-abis Gerrit-Branch: master Gerrit-Change-Id: Idca24d3e676a45d860d9eec60dc2097d8d87f3bf Gerrit-Change-Number: 36079 Gerrit-PatchSet: 2 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: Jenkins Builder Gerrit-MessageType: newpatchset

1 year, 4 months

1
0
0 0

[M] Change in libosmo-abis[master]: Fix critical bug in default TCP keepalive user timeout

by laforge

laforge has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmo-abis/+/36079?usp=email ) Change subject: Fix critical bug in default TCP keepalive user timeout ...................................................................... Fix critical bug in default TCP keepalive user timeout It turns out that our calculation of the TCP_USER_TIMEOUT value was flawed in several ways: * there should have been parenthesis around the + operator (line->keepalive_probe_interval + line->keepalive_idle_timeout) as the keepalive_idle_timeout is in seconds, not milli-seconds. * in the default case, all those values are configured to -1 (E1INP_USE_DEFAULT). This means we're using 1000 * -1 * -1 + -1 = 999 i.e. just below aq second which clearly is not enough for a lossy satellite or wifi back-haul. This fixes a regression introduced in Ia7659c209aea0d26eb37d31e771adc91b17ae668 i.e. present since libosmo-abis 1.4.0 Change-Id: Idca24d3e676a45d860d9eec60dc2097d8d87f3bf Related: OS#5785, SYS#6801 Fixes: Ia7659c209aea0d26eb37d31e771adc91b17ae668 --- M src/input/ipaccess.c 1 file changed, 41 insertions(+), 16 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmo-abis refs/changes/79/36079/1 diff --git a/src/input/ipaccess.c b/src/input/ipaccess.c index 75d9693..7979b7b 100644 --- a/src/input/ipaccess.c +++ b/src/input/ipaccess.c @@ -588,7 +588,7 @@ static void update_fd_settings(struct e1inp_line *line, int fd) { int ret; - int val; + int val, timeout_val, interval_val, retry_count_val; if (line->keepalive_num_probes) { /* Enable TCP keepalive to find out if the connection is gone */ @@ -600,31 +600,29 @@ LOGPIL(line, DLINP, LOGL_NOTICE, "TCP Keepalive is enabled\n"); /* The following options are not portable! */ - val = line->keepalive_idle_timeout > 0 ? - line->keepalive_idle_timeout : - DEFAULT_TCP_KEEPALIVE_IDLE_TIMEOUT; - ret = setsockopt(fd, IPPROTO_TCP, TCP_KEEPIDLE, &val, sizeof(val)); + timeout_val = line->keepalive_idle_timeout > 0 ? + line->keepalive_idle_timeout : + DEFAULT_TCP_KEEPALIVE_IDLE_TIMEOUT; + ret = setsockopt(fd, IPPROTO_TCP, TCP_KEEPIDLE, &timeout_val, sizeof(timeout_val)); if (ret < 0) { LOGPIL(line, DLINP, LOGL_ERROR, "Failed to set TCP keepalive idle time: %s\n", strerror(errno)); } - val = line->keepalive_probe_interval > -1 ? - line->keepalive_probe_interval : - DEFAULT_TCP_KEEPALIVE_INTERVAL; - ret = setsockopt(fd, IPPROTO_TCP, TCP_KEEPINTVL, &val, sizeof(val)); + interval_val = line->keepalive_probe_interval > -1 ? + line->keepalive_probe_interval : + DEFAULT_TCP_KEEPALIVE_INTERVAL; + ret = setsockopt(fd, IPPROTO_TCP, TCP_KEEPINTVL, &interval_val, sizeof(interval_val)); if (ret < 0) { LOGPIL(line, DLINP, LOGL_ERROR, "Failed to set TCP keepalive interval: %s\n", strerror(errno)); } - val = line->keepalive_num_probes > 0 ? - line->keepalive_num_probes : - DEFAULT_TCP_KEEPALIVE_RETRY_COUNT; - ret = setsockopt(fd, IPPROTO_TCP, TCP_KEEPCNT, &val, sizeof(val)); + retry_count_val = line->keepalive_num_probes > 0 ? + line->keepalive_num_probes : + DEFAULT_TCP_KEEPALIVE_RETRY_COUNT; + ret = setsockopt(fd, IPPROTO_TCP, TCP_KEEPCNT, &retry_count_val, sizeof(retry_count_val)); if (ret < 0) LOGPIL(line, DLINP, LOGL_ERROR, "Failed to set TCP keepalive count: %s\n", strerror(errno)); - val = 1000 * line->keepalive_num_probes * - line->keepalive_probe_interval + - line->keepalive_idle_timeout; + val = 1000 * retry_count_val * (interval_val + timeout_val); ret = setsockopt(fd, IPPROTO_TCP, TCP_USER_TIMEOUT, &val, sizeof(val)); if (ret < 0) LOGPIL(line, DLINP, LOGL_ERROR, "Failed to set TCP user timeout: %s\n", strerror(errno)); -- To view, visit https://gerrit.osmocom.org/c/libosmo-abis/+/36079?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-abis Gerrit-Branch: master Gerrit-Change-Id: Idca24d3e676a45d860d9eec60dc2097d8d87f3bf Gerrit-Change-Number: 36079 Gerrit-PatchSet: 1 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-MessageType: newchange

1 year, 4 months

1
0
0 0

[S] Change in libosmocore[master]: osmo_io: Add osmo_io_get_ioops() function

by pespin

Attention is currently required from: laforge. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/36076?usp=email ) Change subject: osmo_io: Add osmo_io_get_ioops() function ...................................................................... Patch Set 1: Code-Review+1 (1 comment) File include/osmocom/core/osmo_io.h: https://gerrit.osmocom.org/c/libosmocore/+/36076/comment/3c07305a_ad1f92bd PS1, Line 112: void osmo_iofd_get_ioops(struct osmo_io_fd *iofd, struct osmo_io_ops *ioops); It may make more sense to return a "const struct osmo_io_ops*" so that no copy is needed by default. This also means the struct is not allocated by the user, which may be an advantage if the struct size changes? Not a strong opinion though, just raising the topic. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/36076?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: I03398c811b9534f50c6644b21eea89a04be29fb0 Gerrit-Change-Number: 36076 Gerrit-PatchSet: 1 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Comment-Date: Fri, 23 Feb 2024 19:55:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 year, 4 months

1
0
0 0

[M] Change in libosmocore[master]: osmo_io: sendmsg/recvmsg support

by pespin

Attention is currently required from: daniel, fixeria, jolly, laforge. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35068?usp=email ) Change subject: osmo_io: sendmsg/recvmsg support ...................................................................... Patch Set 11: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35068?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: I89eb519b22d21011d61a7855b2364bc3c295df82 Gerrit-Change-Number: 35068 Gerrit-PatchSet: 11 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-CC: jolly <andreas(a)eversberg.eu> Gerrit-Attention: jolly <andreas(a)eversberg.eu> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: daniel <dwillmann(a)sysmocom.de> Gerrit-Comment-Date: Fri, 23 Feb 2024 19:52:21 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 year, 4 months

1
0
0 0

[M] Change in libosmocore[master]: osmo_io: Change struct osmo_io_ops to contain struct, not union

by pespin

Attention is currently required from: daniel, jolly, laforge. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/36062?usp=email ) Change subject: osmo_io: Change struct osmo_io_ops to contain struct, not union ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/36062?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: I9d302df8d00369e7b30437a52deb205f75882be3 Gerrit-Change-Number: 36062 Gerrit-PatchSet: 2 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de> Gerrit-Reviewer: jolly <andreas(a)eversberg.eu> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: jolly <andreas(a)eversberg.eu> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: daniel <dwillmann(a)sysmocom.de> Gerrit-Comment-Date: Fri, 23 Feb 2024 19:51:05 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 year, 4 months

1
0
0 0

[M] Change in osmo-ttcn3-hacks[master]: pgw: Introduce test TC_s2b_createSession

by pespin

pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/36075?usp=email ) Change subject: pgw: Introduce test TC_s2b_createSession ...................................................................... Patch Set 2: This change is ready for review. -- To view, visit https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/36075?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-ttcn3-hacks Gerrit-Branch: master Gerrit-Change-Id: I38e469edf0e00feca5a648035b64645e2c905937 Gerrit-Change-Number: 36075 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Comment-Date: Fri, 23 Feb 2024 19:14:32 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 year, 4 months

1
0
0 0

[S] Change in libosmo-netif[master]: examples/stream-{server,client}: Generate talloc report on SIGUSR1

by laforge

laforge has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmo-netif/+/36078?usp=email ) Change subject: examples/stream-{server,client}: Generate talloc report on SIGUSR1 ...................................................................... examples/stream-{server,client}: Generate talloc report on SIGUSR1 This can help with memleak debugging. Change-Id: Ia16632602b937db88e2da2b053192887bdb8a986 --- M examples/stream-client.c M examples/stream-server.c 2 files changed, 33 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/78/36078/1 diff --git a/examples/stream-client.c b/examples/stream-client.c index 5b79076..97aaa1c 100644 --- a/examples/stream-client.c +++ b/examples/stream-client.c @@ -90,6 +90,15 @@ return 0; } +static void signal_handler(int signum) +{ + switch (signum) { + case SIGUSR1: + talloc_report_full(tall_test, stdout); + break; + } +} + int main(int argc, char **argv) { struct osmo_fd *kbd_ofd; @@ -110,6 +119,8 @@ } } + signal(SIGUSR1, &signal_handler); + tall_test = talloc_named_const(NULL, 1, "osmo_stream_cli_test"); msgb_talloc_ctx_init(tall_test, 0); osmo_init_logging2(tall_test, &osmo_stream_cli_test_log_info); diff --git a/examples/stream-server.c b/examples/stream-server.c index 0ab4b20..d0647b7 100644 --- a/examples/stream-server.c +++ b/examples/stream-server.c @@ -117,6 +117,15 @@ return 0; } +static void signal_handler(int signum) +{ + switch (signum) { + case SIGUSR1: + talloc_report(tall_test, stderr); + break; + } +} + int main(int argc, char **argv) { struct osmo_fd *kbd_ofd; @@ -137,6 +146,8 @@ } } + signal(SIGUSR1, &signal_handler); + tall_test = talloc_named_const(NULL, 1, "osmo_stream_srv_test"); msgb_talloc_ctx_init(tall_test, 0); osmo_init_logging2(tall_test, &osmo_stream_srv_test_log_info); -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/36078?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: Ia16632602b937db88e2da2b053192887bdb8a986 Gerrit-Change-Number: 36078 Gerrit-PatchSet: 1 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-MessageType: newchange

1 year, 4 months

1
0
0 0

[S] Change in libosmo-netif[master]: osmo_io: make use of osmo_iofd_get_ioops()

by laforge

laforge has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmo-netif/+/36077?usp=email ) Change subject: osmo_io: make use of osmo_iofd_get_ioops() ...................................................................... osmo_io: make use of osmo_iofd_get_ioops() This avoids resetting the callbacks with the wrong ones in the SCTP case. Change-Id: Id3cb11bc0000299faa79d980de8abc1980916d70 Depends: libosmocore.git Change-Id I03398c811b9534f50c6644b21eea89a04be29fb0 --- M TODO-RELEASE M src/stream_cli.c M src/stream_srv.c 3 files changed, 16 insertions(+), 8 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmo-netif refs/changes/77/36077/1 diff --git a/TODO-RELEASE b/TODO-RELEASE index 1d860cb..6642459 100644 --- a/TODO-RELEASE +++ b/TODO-RELEASE @@ -10,5 +10,6 @@ libosmocore >1.9.0 working osmo_sock_init2_multiaddr2() without setting flag OSMO_SOCK_F_BIND libosmocore >1.9.0 use osmo_sock_multiaddr_get_name_buf() libosmocore >1.9.0 use OSMO_IO_FD_MODE_RECVMSG_SENDMSG +libosmocore >1.9.0 use osmo_iofd_get_ioops() libosmo-netif added osmo_stream_srv_get_sockname() libosmo-netif update-dependency libosmocore > 1.9.0 required for I89eb519b22d21011d61a7855b2364bc3c295df82 diff --git a/src/stream_cli.c b/src/stream_cli.c index 1800640..88a697d 100644 --- a/src/stream_cli.c +++ b/src/stream_cli.c @@ -631,10 +631,7 @@ { /* Copy default settings */ struct osmo_io_ops client_ops; - if (cli->proto == IPPROTO_SCTP) - client_ops = osmo_stream_cli_ioops_sctp; - else - client_ops = osmo_stream_cli_ioops; + osmo_iofd_get_ioops(cli->iofd, &client_ops); /* Set segmentation cb for this client */ client_ops.segmentation_cb = segmentation_cb; osmo_iofd_set_ioops(cli->iofd, &client_ops); diff --git a/src/stream_srv.c b/src/stream_srv.c index 70ebef8..33494fa 100644 --- a/src/stream_srv.c +++ b/src/stream_srv.c @@ -886,10 +886,7 @@ OSMO_ASSERT(conn->mode == OSMO_STREAM_MODE_OSMO_IO); /* Copy default settings */ struct osmo_io_ops conn_ops; - if (conn->srv->proto == IPPROTO_SCTP) - conn_ops = srv_ioops_sctp; - else - conn_ops = srv_ioops; + osmo_iofd_get_ioops(conn->iofd, &conn_ops); /* Set segmentation cb for this connection */ conn_ops.segmentation_cb = segmentation_cb; osmo_iofd_set_ioops(conn->iofd, &conn_ops); -- To view, visit https://gerrit.osmocom.org/c/libosmo-netif/+/36077?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmo-netif Gerrit-Branch: master Gerrit-Change-Id: Id3cb11bc0000299faa79d980de8abc1980916d70 Gerrit-Change-Number: 36077 Gerrit-PatchSet: 1 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-MessageType: newchange

1 year, 4 months

1
0
0 0

[M] Change in libosmocore[master]: osmo_io: Change struct osmo_io_ops to contain struct, not union

by laforge

Attention is currently required from: daniel, jolly, pespin. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/36062?usp=email ) Change subject: osmo_io: Change struct osmo_io_ops to contain struct, not union ...................................................................... Patch Set 2: (1 comment) Patchset: PS1: > I think I already provided my opinion on this previously, I see no need to change it to struct, but […] I would say over half of the problems in this patchset I saw the last few days were related to this being a union. Now those have been resolved, but I see potential for more of those problems in the future. So better be safe than to optimize for 4-5 * sizeof(unsigned long). If we really need to conserve memory, struct osmo_io_fd should simply point to the io_ops, rather than copying it into each and every osmo_fd. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/36062?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: I9d302df8d00369e7b30437a52deb205f75882be3 Gerrit-Change-Number: 36062 Gerrit-PatchSet: 2 Gerrit-Owner: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de> Gerrit-Reviewer: jolly <andreas(a)eversberg.eu> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: jolly <andreas(a)eversberg.eu> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: daniel <dwillmann(a)sysmocom.de> Gerrit-Comment-Date: Fri, 23 Feb 2024 17:07:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 year, 4 months

1
0
0 0

[M] Change in libosmocore[master]: osmo_io: Use poll/select to notify socket connection at osmo_io_uring.c

by laforge

Attention is currently required from: daniel, jolly, pespin. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35910?usp=email ) Change subject: osmo_io: Use poll/select to notify socket connection at osmo_io_uring.c ...................................................................... Patch Set 8: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35910?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: I4eca9ea72beb0d6ea4d44cce81ed620033f07270 Gerrit-Change-Number: 35910 Gerrit-PatchSet: 8 Gerrit-Owner: jolly <andreas(a)eversberg.eu> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: jolly <andreas(a)eversberg.eu> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: daniel <dwillmann(a)sysmocom.de> Gerrit-Comment-Date: Fri, 23 Feb 2024 17:03:53 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 year, 4 months

1
0
0 0

2025

2024

2023

2022

gerrit-log February 2024