March 2023 - gerrit-log - lists.osmocom.org

[S] Change in libosmocore[master]: configure.ac: Fix logic enabling osmo_fd fd checks
by pespin 14 Mar '23

14 Mar '23

pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/31904 ) ( 1 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: configure.ac: Fix logic enabling osmo_fd fd checks ...................................................................... configure.ac: Fix logic enabling osmo_fd fd checks The logic testing the and setting the define was inverted, which made it enabled by default. Take the chance to rename the enable flag to be "ofd" instead of "bsc-fd" (since anyway the flag was broken). Change-Id: I81112fa1f6ce1a8e5fe85468241ad385ed8805d3 --- M configure.ac 1 file changed, 18 insertions(+), 3 deletions(-) Approvals: fixeria: Looks good to me, approved Jenkins Builder: Verified diff --git a/configure.ac b/configure.ac index 108c0a1..478da9b 100644 --- a/configure.ac +++ b/configure.ac @@ -296,13 +296,13 @@ AC_ARG_ENABLE(bsc_fd_check, [AS_HELP_STRING( - [--enable-bsc-fd-check], + [--enable-ofd-check], [Instrument osmo_fd_register to check that the fd is registered] )], [fd_check=$enableval], [fd_check="no"]) -if test x"$fd_check" = x"no" +if test x"$fd_check" = x"yes" then - AC_DEFINE([OSMO_FD_CHECK],[1],[Instrument the osmo_fd_register]) + AC_DEFINE([OSMO_FD_CHECK], [1], [Instrument the osmo_fd_register]) fi AC_ARG_ENABLE([force_io_select], -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31904 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I81112fa1f6ce1a8e5fe85468241ad385ed8805d3 Gerrit-Change-Number: 31904 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

1 0

[S] Change in libosmocore[master]: configure.ac: Fix typo in enable flag description
by pespin 14 Mar '23

14 Mar '23

pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/31905 ) ( 1 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: configure.ac: Fix typo in enable flag description ...................................................................... configure.ac: Fix typo in enable flag description Change-Id: Ida390c50379a429a4b42434e32954eeb832a11ff --- M configure.ac 1 file changed, 10 insertions(+), 1 deletion(-) Approvals: fixeria: Looks good to me, approved Jenkins Builder: Verified diff --git a/configure.ac b/configure.ac index 478da9b..5e17c7a 100644 --- a/configure.ac +++ b/configure.ac @@ -312,7 +312,7 @@ )], [force_io_select=$enableval], [force_io_select="no"]) AS_IF([test "x$force_io_select" = "xyes"], [ - AC_DEFINE([FORCE_IO_SELECT], [1], [Force the use of select() instaed of poll()]) + AC_DEFINE([FORCE_IO_SELECT], [1], [Force the use of select() instead of poll()]) ]) AC_ARG_ENABLE(msgfile, -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31905 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ida390c50379a429a4b42434e32954eeb832a11ff Gerrit-Change-Number: 31905 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

1 0

[S] Change in libosmocore[master]: select.c: osmo_fd_unregister(): Avoid assert hit with old buggy users...
by pespin 14 Mar '23

14 Mar '23

pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/31903 ) Change subject: select.c: osmo_fd_unregister(): Avoid assert hit with old buggy users of the API ...................................................................... select.c: osmo_fd_unregister(): Avoid assert hit with old buggy users of the API Change-Id: If77b84d603a42a216d550d9708eb62f645634a61 --- M src/core/select.c 1 file changed, 24 insertions(+), 3 deletions(-) Approvals: Jenkins Builder: Verified osmith: Looks good to me, but someone else must approve fixeria: Looks good to me, approved diff --git a/src/core/select.c b/src/core/select.c index 67b0fd3..c60cd8e 100644 --- a/src/core/select.c +++ b/src/core/select.c @@ -223,13 +223,25 @@ * osmo_fd_is_registered() */ unregistered_count++; llist_del(&fd->list); - OSMO_ASSERT(fd->fd >= 0); - OSMO_ASSERT(fd->fd <= maxfd); - osmo_fd_lookup.table[fd->fd] = NULL; #ifndef FORCE_IO_SELECT g_poll.num_registered--; #endif /* FORCE_IO_SELECT */ + if (OSMO_UNLIKELY(fd->fd < 0 || fd->fd > maxfd)) { + /* Some old users used to incorrectly set fd = -1 *before* calling osmo_unregister(). + * Hence, in order to keep backward compatibility it's not possible to assert() here. + * Instead, print an error message since this is actually a bug in the API user. */ +#ifdef OSMO_FD_CHECK + osmo_panic("osmo_fd_unregister(fd=%u) out of expected range (0..%u), fix your code!!!\n", + fd->fd, maxfd); +#else + fprintf(stderr, "osmo_fd_unregister(fd=%u) out of expected range (0..%u), fix your code!!!\n", + fd->fd, maxfd); + return; +#endif + } + + osmo_fd_lookup.table[fd->fd] = NULL; /* If existent, free any statistical data */ osmo_stats_tcp_osmo_fd_unregister(fd); } -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31903 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: If77b84d603a42a216d550d9708eb62f645634a61 Gerrit-Change-Number: 31903 Gerrit-PatchSet: 4 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de> Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

1 0

[S] Change in libosmocore[master]: select.c: osmo_fd_unregister(): Avoid assert hit with old buggy users...
by fixeria 14 Mar '23

14 Mar '23

Attention is currently required from: laforge, pespin, daniel. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/31903 ) Change subject: select.c: osmo_fd_unregister(): Avoid assert hit with old buggy users of the API ...................................................................... Patch Set 4: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31903 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: If77b84d603a42a216d550d9708eb62f645634a61 Gerrit-Change-Number: 31903 Gerrit-PatchSet: 4 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de> Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: daniel <dwillmann(a)sysmocom.de> Gerrit-Comment-Date: Tue, 14 Mar 2023 12:33:22 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in libosmocore[master]: select.c: Clarify osmo_fd_unregister() can only be called on register...
by fixeria 14 Mar '23

14 Mar '23

Attention is currently required from: pespin. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/31902 ) Change subject: select.c: Clarify osmo_fd_unregister() can only be called on registered osmo_fds ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31902 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I5e397e121f2fe2254c7f4b474e6eefd7aebe7a83 Gerrit-Change-Number: 31902 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Tue, 14 Mar 2023 12:32:40 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[L] Change in osmocom-bb[master]: modem: move GRR specific code into its own file
by fixeria 14 Mar '23

14 Mar '23

Attention is currently required from: pespin. fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/31654 ) Change subject: modem: move GRR specific code into its own file ...................................................................... Patch Set 3: (1 comment) Patchset: PS1: > To me all this is not really grr, grr would be between LLC and RLC/MAC. […] Naming is hard. GRR in this case is not a layer itself, it's more like a collection of Radio Resource related routines, which have grown up into quite some amount of code, making it hard to read/modify `app_modem.c`. We can always rename it later, for now I would prefer to get this patch merged as-is. -- To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/31654 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Change-Id: I25caa0bd01e3d090803512f5b13cad58439f44f8 Gerrit-Change-Number: 31654 Gerrit-PatchSet: 3 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Tue, 14 Mar 2023 12:19:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[S] Change in libosmocore[master]: select.c: osmo_fd_unregister(): Avoid assert hit with old buggy users...
by osmith 14 Mar '23

14 Mar '23

Attention is currently required from: laforge, pespin, fixeria, daniel. osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/31903 ) Change subject: select.c: osmo_fd_unregister(): Avoid assert hit with old buggy users of the API ...................................................................... Patch Set 4: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31903 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: If77b84d603a42a216d550d9708eb62f645634a61 Gerrit-Change-Number: 31903 Gerrit-PatchSet: 4 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de> Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: daniel <dwillmann(a)sysmocom.de> Gerrit-Comment-Date: Tue, 14 Mar 2023 12:18:10 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in osmocom-bb[master]: layer23: Call osmo_fd_unregister() before closing and changing bfd->fd
by pespin 14 Mar '23

14 Mar '23

pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/31893 ) Change subject: layer23: Call osmo_fd_unregister() before closing and changing bfd->fd ...................................................................... layer23: Call osmo_fd_unregister() before closing and changing bfd->fd Change-Id: I886062c1a944acf0982755a7acddf7e2233d13a8 --- M src/host/layer23/src/common/l1l2_interface.c M src/host/layer23/src/common/sap_interface.c M src/host/layer23/src/mobile/mncc_sock.c 3 files changed, 12 insertions(+), 4 deletions(-) Approvals: fixeria: Looks good to me, approved Jenkins Builder: Verified diff --git a/src/host/layer23/src/common/l1l2_interface.c b/src/host/layer23/src/common/l1l2_interface.c index 94303c2..8e27052 100644 --- a/src/host/layer23/src/common/l1l2_interface.c +++ b/src/host/layer23/src/common/l1l2_interface.c @@ -126,9 +126,9 @@ if (ms->l2_wq.bfd.fd <= 0) return -EINVAL; + osmo_fd_unregister(&ms->l2_wq.bfd); close(ms->l2_wq.bfd.fd); ms->l2_wq.bfd.fd = -1; - osmo_fd_unregister(&ms->l2_wq.bfd); osmo_wqueue_clear(&ms->l2_wq); return 0; diff --git a/src/host/layer23/src/common/sap_interface.c b/src/host/layer23/src/common/sap_interface.c index 86c70a9..0f9f8c2 100644 --- a/src/host/layer23/src/common/sap_interface.c +++ b/src/host/layer23/src/common/sap_interface.c @@ -321,10 +321,9 @@ if (ms->sap_wq.bfd.fd <= 0) return -EINVAL; + osmo_fd_unregister(&ms->sap_wq.bfd); close(ms->sap_wq.bfd.fd); ms->sap_wq.bfd.fd = -1; - - osmo_fd_unregister(&ms->sap_wq.bfd); osmo_wqueue_clear(&ms->sap_wq); return 0; diff --git a/src/host/layer23/src/mobile/mncc_sock.c b/src/host/layer23/src/mobile/mncc_sock.c index 514ab72..d31df29 100644 --- a/src/host/layer23/src/mobile/mncc_sock.c +++ b/src/host/layer23/src/mobile/mncc_sock.c @@ -89,9 +89,9 @@ LOGP(DMNCC, LOGL_NOTICE, "MNCC Socket has closed connection\n"); + osmo_fd_unregister(bfd); close(bfd->fd); bfd->fd = -1; - osmo_fd_unregister(bfd); /* re-enable the generation of ACCEPT for new connections */ osmo_fd_read_enable(&state->listen_bfd); -- To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/31893 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Change-Id: I886062c1a944acf0982755a7acddf7e2233d13a8 Gerrit-Change-Number: 31893 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

1 0

[S] Change in libosmocore[master]: select.c: osmo_fd_unregister(): Avoid assert hit with old buggy users...
by pespin 14 Mar '23

14 Mar '23

Attention is currently required from: laforge, pespin, fixeria, daniel. Hello osmith, Jenkins Builder, laforge, fixeria, daniel, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmocore/+/31903 to look at the new patch set (#4). Change subject: select.c: osmo_fd_unregister(): Avoid assert hit with old buggy users of the API ...................................................................... select.c: osmo_fd_unregister(): Avoid assert hit with old buggy users of the API Change-Id: If77b84d603a42a216d550d9708eb62f645634a61 --- M src/core/select.c 1 file changed, 24 insertions(+), 3 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/03/31903/4 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31903 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: If77b84d603a42a216d550d9708eb62f645634a61 Gerrit-Change-Number: 31903 Gerrit-PatchSet: 4 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillmann(a)sysmocom.de> Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: daniel <dwillmann(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 0

[S] Change in osmocom-bb[master]: modem: get rid of app_data.chan_req, use ms->rrlayer
by fixeria 14 Mar '23

14 Mar '23

fixeria has submitted this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/31653 ) ( 2 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: modem: get rid of app_data.chan_req, use ms->rrlayer ...................................................................... modem: get rid of app_data.chan_req, use ms->rrlayer Change-Id: I77d0a834a73c511f157c6f1524b9e98df3f4def4 Related: OS#5500 --- M src/host/layer23/src/modem/app_modem.c 1 file changed, 35 insertions(+), 12 deletions(-) Approvals: pespin: Looks good to me, but someone else must approve laforge: Looks good to me, approved Jenkins Builder: Verified diff --git a/src/host/layer23/src/modem/app_modem.c b/src/host/layer23/src/modem/app_modem.c index b5275f9..6538114 100644 --- a/src/host/layer23/src/modem/app_modem.c +++ b/src/host/layer23/src/modem/app_modem.c @@ -60,12 +60,6 @@ static struct { struct osmocom_ms *ms; - - /* TODO: use mobile->rrlayer API instead */ - struct { - bool ref_valid; - struct gsm48_req_ref ref; - } chan_req; } app_data; /* Local network-originated IP packet, needs to be sent via SNDCP/LLC (GPRS) towards GSM network */ @@ -141,6 +135,8 @@ return -ENOTSUP; rr->cr_ra = gen_chan_req(single_block); + memset(&rr->cr_hist[0], 0x00, sizeof(rr->cr_hist)); + LOGP(DRR, LOGL_NOTICE, "Sending CHANNEL REQUEST (0x%02x)\n", rr->cr_ra); l1ctl_tx_rach_req(ms, RSL_CHAN_RACH, 0x00, rr->cr_ra, 0, cs->ccch_mode == CCCH_MODE_COMBINED); @@ -149,6 +145,22 @@ return 0; } +static bool modem_match_req_ref(struct osmocom_ms *ms, + const struct gsm48_req_ref *ref) +{ + struct gsm48_rrlayer *rr = &ms->rrlayer; + + for (unsigned int i = 0; i < ARRAY_SIZE(rr->cr_hist); i++) { + const struct gsm48_cr_hist *hist = &rr->cr_hist[i]; + if (!hist->valid) + continue; + if (memcmp(&hist->ref, ref, sizeof(*ref)) == 0) + return true; + } + + return false; +} + static int handle_si1(struct osmocom_ms *ms, struct msgb *msg) { struct gsm322_cellsel *cs = &ms->cellsel; @@ -291,9 +303,7 @@ if (rr->state != GSM48_RR_ST_CONN_PEND) return 0; - if (!app_data.chan_req.ref_valid) - return 0; - if (memcmp(&ia->req_ref, &app_data.chan_req.ref, sizeof(ia->req_ref))) + if (!modem_match_req_ref(ms, &ia->req_ref)) return 0; if (rsl_dec_chan_nr(ia->chan_desc.chan_nr, &ch_type, &ch_subch, &ch_ts) != 0) { @@ -467,9 +477,12 @@ LOGP(DRSL, LOGL_NOTICE, "Rx RACH.conf (RA=0x%02x, T1=%u, T3=%u, T2=%u)\n", rr->cr_ra, ref->t1, ref->t3_high << 3 | ref->t3_low, ref->t2); - memcpy(&app_data.chan_req.ref, ref, sizeof(*ref)); - app_data.chan_req.ref.ra = rr->cr_ra; - app_data.chan_req.ref_valid = true; + /* shift the CHANNEL REQUEST history buffer */ + memmove(&rr->cr_hist[1], &rr->cr_hist[0], ARRAY_SIZE(rr->cr_hist) - 1); + /* store the new entry */ + rr->cr_hist[0].ref = *ref; + rr->cr_hist[0].ref.ra = rr->cr_ra; + rr->cr_hist[0].valid = 1; return 0; } /* fall-through */ -- To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/31653 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmocom-bb Gerrit-Branch: master Gerrit-Change-Id: I77d0a834a73c511f157c6f1524b9e98df3f4def4 Gerrit-Change-Number: 31653 Gerrit-PatchSet: 3 Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

1 0