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

[S] Change in libosmocore[master]: select.c: Clarify osmo_fd_(un)register() API expectations of 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/+/31901 ) Change subject: select.c: Clarify osmo_fd_(un)register() API expectations of registered fd ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31901 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I53d95aad15b33dd66aa5c7dd11745a35c4481f33 Gerrit-Change-Number: 31901 Gerrit-PatchSet: 1 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 11:24:28 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in libosmocore[master]: select.c: Clarify osmo_fd_(un)register() API expectations of register...
by osmith 14 Mar '23

14 Mar '23

Attention is currently required from: pespin. osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/31901 ) Change subject: select.c: Clarify osmo_fd_(un)register() API expectations of registered fd ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/31901 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I53d95aad15b33dd66aa5c7dd11745a35c4481f33 Gerrit-Change-Number: 31901 Gerrit-PatchSet: 1 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Tue, 14 Mar 2023 11:24:02 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes 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 2: (1 comment) File src/core/select.c: https://gerrit.osmocom.org/c/libosmocore/+/31903/comment/a7ce13d0_a4a68bb1 PS2, Line 234: fprintf(stderr, "osmo_fd_unregister(fd=%u) out of expected range (0..%u), fix your code!!!\n", why printf and not an osmocom error log message? -- 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: 2 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 11:23:03 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in osmo-pcu[master]: Update libosmogsm deprecated include
by osmith 14 Mar '23

14 Mar '23

Attention is currently required from: pespin. osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/31888 ) Change subject: Update libosmogsm deprecated include ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/31888 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: I027abbf3ed4c71331000565af1ef4f08f10cfafc Gerrit-Change-Number: 31888 Gerrit-PatchSet: 1 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: osmith <osmith(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Tue, 14 Mar 2023 11:21:32 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in osmo-pcu[master]: osmo_bts_sock.c: Call osmo_fd_unregister() before closing and changin...
by osmith 14 Mar '23

14 Mar '23

Attention is currently required from: laforge, pespin, daniel. osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/31887 ) Change subject: osmo_bts_sock.c: Call osmo_fd_unregister() before closing and changing bfd->fd ...................................................................... Patch Set 1: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/31887 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Iec0faba781bcb2bc6de8710392655785790b9af4 Gerrit-Change-Number: 31887 Gerrit-PatchSet: 1 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillmann(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 11:20:57 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in osmo-bsc[master]: bscc_sccp: Small optimiztion in bsc_sccp_inst_next_conn_id()
by pespin 14 Mar '23

14 Mar '23

pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31811 ) Change subject: bscc_sccp: Small optimiztion in bsc_sccp_inst_next_conn_id() ...................................................................... bscc_sccp: Small optimiztion in bsc_sccp_inst_next_conn_id() Refactor the double loop to check a code path matching the sccp_instance once instead of doing so for every subscr_conn. If for instance let's say we have 1000 concurrent calls in progress, which means we have 1000 subscr_conn, which means we at least do the extra check regarding SMLC vs MSC 1000 times (at least, xN times if N conn_id already used are already found). That overhead happens every time a new subscr_conn is created (which in a BSC with already 1000 concurrent calls can potentially happen quite frequently). Change-Id: Ic32b1eeb201fc51110e1ee130110824845f81e82 --- M src/osmo-bsc/bsc_sccp.c 1 file changed, 51 insertions(+), 13 deletions(-) Approvals: fixeria: Looks good to me, but someone else must approve Jenkins Builder: Verified laforge: Looks good to me, approved diff --git a/src/osmo-bsc/bsc_sccp.c b/src/osmo-bsc/bsc_sccp.c index 431e194..e0cce39 100644 --- a/src/osmo-bsc/bsc_sccp.c +++ b/src/osmo-bsc/bsc_sccp.c @@ -47,6 +47,33 @@ /* This looks really suboptimal, but in most cases the static next_id should indicate exactly the next unused * conn_id, and we only iterate all conns once to make super sure that it is not already in use. */ + /* SCCP towards SMLC: */ + if (bsc_gsmnet->smlc->sccp == sccp) { + for (i = 0; i < SCCP_CONN_ID_MAX; i++) { + struct gsm_subscriber_connection *conn; + uint32_t conn_id = next_id; + bool conn_id_already_used = false; + + /* Optimized modulo operation (% SCCP_CONN_ID_MAX) using bitwise AND plus CMP: */ + next_id = (next_id + 1) & 0x00FFFFFF; + if (OSMO_UNLIKELY(next_id == 0x00FFFFFF)) + next_id = 0; + + llist_for_each_entry(conn, &bsc_gsmnet->subscr_conns, entry) { + if (conn->lcs.lb.state != SUBSCR_SCCP_ST_NONE && + conn->lcs.lb.conn_id == conn_id) { + conn_id_already_used = true; + break; + } + } + + if (!conn_id_already_used) + return conn_id; + } + return 0xFFFFFFFF; + } + + /* SCCP towards MSC: */ for (i = 0; i < SCCP_CONN_ID_MAX; i++) { struct gsm_subscriber_connection *conn; uint32_t conn_id = next_id; @@ -58,19 +85,10 @@ next_id = 0; llist_for_each_entry(conn, &bsc_gsmnet->subscr_conns, entry) { - if (conn->sccp.msc && conn->sccp.msc->a.sccp == sccp) { - if (conn_id == conn->sccp.conn_id) { - conn_id_already_used = true; - break; - } - } - - if (bsc_gsmnet->smlc->sccp == sccp - && conn->lcs.lb.state != SUBSCR_SCCP_ST_NONE) { - if (conn_id == conn->lcs.lb.conn_id) { - conn_id_already_used = true; - break; - } + if (conn->sccp.msc && conn->sccp.msc->a.sccp == sccp && + conn->sccp.conn_id == conn_id) { + conn_id_already_used = true; + break; } } -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31811 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: Ic32b1eeb201fc51110e1ee130110824845f81e82 Gerrit-Change-Number: 31811 Gerrit-PatchSet: 3 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: merged

1 0

[S] Change in osmo-bsc[master]: Assert conn_id being looked up is inside expected range
by pespin 14 Mar '23

14 Mar '23

pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31812 ) Change subject: Assert conn_id being looked up is inside expected range ...................................................................... Assert conn_id being looked up is inside expected range Change-Id: Iffe580ad5f974fdbef7061a14306b50bd4875eb1 --- M src/osmo-bsc/lb.c M src/osmo-bsc/osmo_bsc_sigtran.c 2 files changed, 15 insertions(+), 1 deletion(-) Approvals: fixeria: Looks good to me, approved Jenkins Builder: Verified laforge: Looks good to me, approved diff --git a/src/osmo-bsc/lb.c b/src/osmo-bsc/lb.c index c34d88e..6e2d074 100644 --- a/src/osmo-bsc/lb.c +++ b/src/osmo-bsc/lb.c @@ -36,6 +36,9 @@ { struct gsm_subscriber_connection *conn; + /* Range (0..SCCP_CONN_ID_MAX) expected, see bsc_sccp_inst_next_conn_id() */ + OSMO_ASSERT(conn_id <= SCCP_CONN_ID_MAX); + llist_for_each_entry(conn, &bsc_gsmnet->subscr_conns, entry) { if (conn->lcs.lb.state != SUBSCR_SCCP_ST_NONE && conn->lcs.lb.conn_id == conn_id) diff --git a/src/osmo-bsc/osmo_bsc_sigtran.c b/src/osmo-bsc/osmo_bsc_sigtran.c index 1d4fe82..973832b 100644 --- a/src/osmo-bsc/osmo_bsc_sigtran.c +++ b/src/osmo-bsc/osmo_bsc_sigtran.c @@ -48,10 +48,12 @@ /* Helper function to Check if the given connection id is already assigned */ static struct gsm_subscriber_connection *get_bsc_conn_by_conn_id(const struct osmo_sccp_user *scu, uint32_t conn_id) { - conn_id &= SCCP_CONN_ID_MAX; struct gsm_subscriber_connection *conn; const struct osmo_sccp_instance *sccp = osmo_sccp_get_sccp(scu); + /* Range (0..SCCP_CONN_ID_MAX) expected, see bsc_sccp_inst_next_conn_id() */ + OSMO_ASSERT(conn_id <= SCCP_CONN_ID_MAX); + llist_for_each_entry(conn, &bsc_gsmnet->subscr_conns, entry) { if (conn->sccp.msc && conn->sccp.msc->a.sccp != sccp) continue; -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31812 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: Iffe580ad5f974fdbef7061a14306b50bd4875eb1 Gerrit-Change-Number: 31812 Gerrit-PatchSet: 3 Gerrit-Owner: pespin <pespin(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

[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 uploaded a new patch set (#2). ( 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, 19 insertions(+), 3 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/03/31903/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: 2 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-CC: Jenkins Builder Gerrit-MessageType: newpatchset

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 uploaded this change for review. ( 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, 19 insertions(+), 3 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/03/31903/1 diff --git a/src/core/select.c b/src/core/select.c index ea8c654..924356a 100644 --- a/src/core/select.c +++ b/src/core/select.c @@ -223,13 +223,20 @@ * 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. */ + fprintf(stderr, "osmo_fd_unregister(fd=%u) out of expected range (0..%u), fix your code!!!\n", + fd->fd, maxfd); + return; + } + + 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: 1 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: newchange

1 0

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

14 Mar '23

pespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmocore/+/31902 ) Change subject: select.c: Clarify osmo_fd_unregister() can only be called on registered osmo_fds ...................................................................... select.c: Clarify osmo_fd_unregister() can only be called on registered osmo_fds Change-Id: I5e397e121f2fe2254c7f4b474e6eefd7aebe7a83 --- M src/core/select.c 1 file changed, 10 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/02/31902/1 diff --git a/src/core/select.c b/src/core/select.c index 584de24..ea8c654 100644 --- a/src/core/select.c +++ b/src/core/select.c @@ -212,6 +212,7 @@ /*! Unregister a file descriptor from select loop abstraction * \param[in] fd osmocom file descriptor to be unregistered * + * Caller is responsible from ensuring the fd is really registered before calling this API. * This function must be called before changing the value of the fd field in * the struct osmo_fd. */ -- 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: 1 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: newchange

1 0