Attention is currently required from: matanp.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/35475?usp=email )
Change subject: abis_rsl: Separate rach expiry delay from radio link timeout
......................................................................
Patch Set 3: Code-Review-1
(3 comments)
Commit Message:
https://gerrit.osmocom.org/c/osmo-bsc/+/35475/comment/c3fcd7ac_ea05c0ef
PS3, Line 7: abis_rsl: Separate rach expiry delay from radio link timeout
Please elaborate more why a separate parameter is needed (here in COMMIT_MSG).
File src/osmo-bsc/bts_vty.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/35475/comment/5b3e3474_397dda40
PS3, Line 777: expiry timeout
Let's combine it into a single command item `expiry-timeout`.
https://gerrit.osmocom.org/c/osmo-bsc/+/35475/comment/9e726999_a8844337
PS3, Line 4532: vty_out(vty, " rach max-delay %u%s", bts->rach_max_delay, VTY_NEWLINE);
You're missing to print `rach expiry-timeout` here.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/35475?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: Iff7266672dd8bc9ce2b34b0478d98fb70691f425
Gerrit-Change-Number: 35475
Gerrit-PatchSet: 3
Gerrit-Owner: matanp <matan1008(a)gmail.com>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: matanp <matan1008(a)gmail.com>
Gerrit-Comment-Date: Sun, 31 Dec 2023 21:33:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
matanp has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/35475?usp=email )
Change subject: abis_rsl: Separate rach expiry delay from radio link timeout
......................................................................
Set Ready For Review
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/35475?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: Iff7266672dd8bc9ce2b34b0478d98fb70691f425
Gerrit-Change-Number: 35475
Gerrit-PatchSet: 3
Gerrit-Owner: matanp <matan1008(a)gmail.com>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Comment-Date: Sun, 31 Dec 2023 12:38:05 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
fixeria has submitted this change. ( https://gerrit.osmocom.org/c/libosmocore/+/35474?usp=email )
Change subject: utils: fix -Wsign-compare in definition of OSMO_STRBUF_CHAR_COUNT
......................................................................
utils: fix -Wsign-compare in definition of OSMO_STRBUF_CHAR_COUNT
We're seeing tons of -Wsign-compare warnings since I merged 0f59cebf:
include/osmocom/core/utils.h: In function 'size_t _osmo_strbuf_char_count(const osmo_strbuf*)':
include/osmocom/core/utils.h:24:29: error: comparison of integer expressions of different
signedness: 'long int' and 'long unsigned int'
[-Werror=sign-compare]
24 | #define OSMO_MIN(a, b) ((a) >= (b) ? (b) : (a))
| ~~~~^~~~~~
include/osmocom/core/utils.h:309:16: note: in expansion of macro 'OSMO_MIN'
309 | return OSMO_MIN(sb->pos - sb->buf, sb->len - 1);
| ^~~~~~~~
Interestingly enough, this -Wsign-compare problem has always been the
case, even before commit 0f59cebf. And somehow this did not show up
when building libosmocore.git, but only when building C++ projects
(osmo-pcu and osmo-trx).
Perhaps it has something to do with how g++ compiles extern "C" code.
Change-Id: I8e396459409e4260b8715f9e890e8972d4609a31
Fixes: 0f59cebf "utils: improve readability of OSMO_STRBUF_CHAR_COUNT"
---
M include/osmocom/core/utils.h
1 file changed, 30 insertions(+), 1 deletion(-)
Approvals:
fixeria: Looks good to me, approved
Jenkins Builder: Verified
diff --git a/include/osmocom/core/utils.h b/include/osmocom/core/utils.h
index 9a6e6b2..92bea59 100644
--- a/include/osmocom/core/utils.h
+++ b/include/osmocom/core/utils.h
@@ -306,7 +306,7 @@
return 0;
if (sb->pos == NULL || sb->pos <= sb->buf)
return 0;
- return OSMO_MIN(sb->pos - sb->buf, sb->len - 1);
+ return OSMO_MIN((size_t)(sb->pos - sb->buf), sb->len - 1);
}
/*! Return number of actual characters contained in struct osmo_strbuf (without terminating nul). */
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35474?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: I8e396459409e4260b8715f9e890e8972d4609a31
Gerrit-Change-Number: 35474
Gerrit-PatchSet: 2
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: merged
matanp has uploaded a new patch set (#2). ( https://gerrit.osmocom.org/c/osmo-bsc/+/35475?usp=email )
Change subject: abis_rsl: Separate rach expiry delay from radio link timeout
......................................................................
abis_rsl: Separate rach expiry delay from radio link timeout
Change-Id: Iff7266672dd8bc9ce2b34b0478d98fb70691f425
---
M include/osmocom/bsc/bts.h
M src/osmo-bsc/abis_rsl.c
M src/osmo-bsc/bts.c
M src/osmo-bsc/bts_vty.c
4 files changed, 29 insertions(+), 3 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/75/35475/2
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/35475?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: Iff7266672dd8bc9ce2b34b0478d98fb70691f425
Gerrit-Change-Number: 35475
Gerrit-PatchSet: 2
Gerrit-Owner: matanp <matan1008(a)gmail.com>
Gerrit-CC: Jenkins Builder
Gerrit-MessageType: newpatchset
matanp has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-bsc/+/35475?usp=email )
Change subject: abis_rsl: Configure rach expiry delay different than radio link timeout
......................................................................
abis_rsl: Configure rach expiry delay different than radio link timeout
Change-Id: Iff7266672dd8bc9ce2b34b0478d98fb70691f425
---
M include/osmocom/bsc/bts.h
M src/osmo-bsc/abis_rsl.c
M src/osmo-bsc/bts.c
M src/osmo-bsc/bts_vty.c
4 files changed, 29 insertions(+), 3 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bsc refs/changes/75/35475/1
diff --git a/include/osmocom/bsc/bts.h b/include/osmocom/bsc/bts.h
index 6974dca..a1799eb 100644
--- a/include/osmocom/bsc/bts.h
+++ b/include/osmocom/bsc/bts.h
@@ -663,6 +663,9 @@
/* We will ignore CHAN RQD with access delay greater than rach_max_delay */
uint8_t rach_max_delay;
+ /* We will ignore CHAN RQD sitting in the queue for period greater than rach_expiry_timeout */
+ uint8_t rach_expiry_timeout;
+
/* Is Fast return to LTE allowed during Chan Release in this BTS? */
bool srvcc_fast_return_allowed;
diff --git a/src/osmo-bsc/abis_rsl.c b/src/osmo-bsc/abis_rsl.c
index 860e4a2..49e8b52 100644
--- a/src/osmo-bsc/abis_rsl.c
+++ b/src/osmo-bsc/abis_rsl.c
@@ -1975,7 +1975,6 @@
* requests from the queue to prevent the queue from growing indefinetly. */
static void reduce_rach_dos(struct gsm_bts *bts)
{
- int rlt = gsm_bts_get_radio_link_timeout(bts);
time_t timestamp_current = time(NULL);
struct chan_rqd *rqd;
struct chan_rqd *rqd_tmp;
@@ -1983,9 +1982,9 @@
/* Drop all expired channel requests in the list */
llist_for_each_entry_safe(rqd, rqd_tmp, &bts->chan_rqd_queue, entry) {
- /* If the channel request is older than the radio link timeout we drop it. This also means that the
+ /* If the channel request is older than the rach expiry timeout we drop it. This also means that the
* queue is under its overflow limit again. */
- if (timestamp_current - rqd->timestamp > rlt) {
+ if (timestamp_current - rqd->timestamp > bts->rach_expiry_timeout) {
LOG_BTS(bts, DRSL, LOGL_INFO, "CHAN RQD: tossing expired channel request"
"(ra=0x%02x, neci=0x%02x, chreq_reason=0x%02x)\n",
rqd->ref.ra, bts->network->neci, rqd->reason);
diff --git a/src/osmo-bsc/bts.c b/src/osmo-bsc/bts.c
index 814ba41..1904137 100644
--- a/src/osmo-bsc/bts.c
+++ b/src/osmo-bsc/bts.c
@@ -470,6 +470,7 @@
bts->interf_meas_params_cfg = interf_meas_params_def;
bts->rach_max_delay = 63;
+ bts->rach_expiry_timeout = 32;
/* SRVCC is enabled by default */
bts->srvcc_fast_return_allowed = true;
diff --git a/src/osmo-bsc/bts_vty.c b/src/osmo-bsc/bts_vty.c
index 3af7296..dade2a8 100644
--- a/src/osmo-bsc/bts_vty.c
+++ b/src/osmo-bsc/bts_vty.c
@@ -772,6 +772,19 @@
return CMD_SUCCESS;
}
+DEFUN_ATTR(cfg_bts_rach_expiry_timeout,
+ cfg_bts_rach_expiry_timeout_cmd,
+ "rach expiry timeout <4-64>",
+ RACH_STR
+ "Set the timeout for channel requests expiry\n"
+ "Maximum timeout before dropping channel requests\n",
+ CMD_ATTR_IMMEDIATE)
+{
+ struct gsm_bts *bts = vty->index;
+ bts->rach_expiry_timeout = atoi(argv[0]);
+ return CMD_SUCCESS;
+}
+
#define REP_ACCH_STR "FACCH/SACCH repetition\n"
DEFUN_USRATTR(cfg_bts_rep_dl_facch,
@@ -4862,6 +4875,7 @@
install_element(BTS_NODE, &cfg_bts_rach_tx_integer_cmd);
install_element(BTS_NODE, &cfg_bts_rach_max_trans_cmd);
install_element(BTS_NODE, &cfg_bts_rach_max_delay_cmd);
+ install_element(BTS_NODE, &cfg_bts_rach_expiry_timeout_cmd);
install_element(BTS_NODE, &cfg_bts_chan_desc_att_cmd);
install_element(BTS_NODE, &cfg_bts_chan_dscr_att_cmd);
install_element(BTS_NODE, &cfg_bts_chan_desc_bs_pa_mfrms_cmd);
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/35475?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: Iff7266672dd8bc9ce2b34b0478d98fb70691f425
Gerrit-Change-Number: 35475
Gerrit-PatchSet: 1
Gerrit-Owner: matanp <matan1008(a)gmail.com>
Gerrit-MessageType: newchange
Hello Jenkins Builder,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/libosmocore/+/35474?usp=email
to look at the new patch set (#2).
Change subject: utils: fix -Wsign-compare in definition of OSMO_STRBUF_CHAR_COUNT
......................................................................
utils: fix -Wsign-compare in definition of OSMO_STRBUF_CHAR_COUNT
We're seeing tons of -Wsign-compare warnings since I merged 0f59cebf:
include/osmocom/core/utils.h: In function 'size_t _osmo_strbuf_char_count(const osmo_strbuf*)':
include/osmocom/core/utils.h:24:29: error: comparison of integer expressions of different
signedness: 'long int' and 'long unsigned int'
[-Werror=sign-compare]
24 | #define OSMO_MIN(a, b) ((a) >= (b) ? (b) : (a))
| ~~~~^~~~~~
include/osmocom/core/utils.h:309:16: note: in expansion of macro 'OSMO_MIN'
309 | return OSMO_MIN(sb->pos - sb->buf, sb->len - 1);
| ^~~~~~~~
Interestingly enough, this -Wsign-compare problem has always been the
case, even before commit 0f59cebf. And somehow this did not show up
when building libosmocore.git, but only when building C++ projects
(osmo-pcu and osmo-trx).
Perhaps it has something to do with how g++ compiles extern "C" code.
Change-Id: I8e396459409e4260b8715f9e890e8972d4609a31
Fixes: 0f59cebf "utils: improve readability of OSMO_STRBUF_CHAR_COUNT"
---
M include/osmocom/core/utils.h
1 file changed, 30 insertions(+), 1 deletion(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/74/35474/2
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35474?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: I8e396459409e4260b8715f9e890e8972d4609a31
Gerrit-Change-Number: 35474
Gerrit-PatchSet: 2
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-MessageType: newpatchset
fixeria has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmocore/+/35474?usp=email )
Change subject: utils: fix -Wsign-compare in definition of OSMO_STRBUF_CHAR_COUNT
......................................................................
utils: fix -Wsign-compare in definition of OSMO_STRBUF_CHAR_COUNT
Recent commit 0f59cebf had a problem with passing signed and unsigned
values to OSMO_MIN, what caused tons of -Wsign-compare warnings being
printed for each *.c file including <osmocom/core/utils.h>.
include/osmocom/core/utils.h: In function 'size_t _osmo_strbuf_char_count(const osmo_strbuf*)':
include/osmocom/core/utils.h:24:29: error: comparison of integer expressions of different
signedness: 'long int' and 'long unsigned int'
[-Werror=sign-compare]
24 | #define OSMO_MIN(a, b) ((a) >= (b) ? (b) : (a))
| ~~~~^~~~~~
include/osmocom/core/utils.h:309:16: note: in expansion of macro 'OSMO_MIN'
309 | return OSMO_MIN(sb->pos - sb->buf, sb->len - 1);
| ^~~~~~~~
Interestingly enough, this has always been the case before 0f59cebf.
And somehow this did not show up when building libosmocore.git,
but only for projects written in C++ (osmo-pcu and osmo-trx).
Perhaps it has something to do with how g++ compules extern "C" code.
Change-Id: I8e396459409e4260b8715f9e890e8972d4609a31
Fixes: 0f59cebf "utils: improve readability of OSMO_STRBUF_CHAR_COUNT"
---
M include/osmocom/core/utils.h
1 file changed, 31 insertions(+), 1 deletion(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/74/35474/1
diff --git a/include/osmocom/core/utils.h b/include/osmocom/core/utils.h
index 9a6e6b2..92bea59 100644
--- a/include/osmocom/core/utils.h
+++ b/include/osmocom/core/utils.h
@@ -306,7 +306,7 @@
return 0;
if (sb->pos == NULL || sb->pos <= sb->buf)
return 0;
- return OSMO_MIN(sb->pos - sb->buf, sb->len - 1);
+ return OSMO_MIN((size_t)(sb->pos - sb->buf), sb->len - 1);
}
/*! Return number of actual characters contained in struct osmo_strbuf (without terminating nul). */
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35474?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: I8e396459409e4260b8715f9e890e8972d4609a31
Gerrit-Change-Number: 35474
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newchange
fixeria has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmocore/+/35471?usp=email )
Change subject: tests/utils: do not test strbuf_example2() with buf=NULL
......................................................................
tests/utils: do not test strbuf_example2() with buf=NULL
The following can be seen when building with CC=clang:
utils/utils_test.c:1239:2: runtime error: applying non-zero offset 99 to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior utils/utils_test.c:1239:2 in
utils/utils_test.c:1241:3: runtime error: applying non-zero offset 99 to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior utils/utils_test.c:1241:3 in
utils/utils_test.c:1242:2: runtime error: applying non-zero offset 99 to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior utils/utils_test.c:1242:2 in
44. testsuite.at:274: 44. utils (testsuite.at:274): FAILED (testsuite.at:278)
This makes utils_test fail due to unexpected UBSan's output.
Even though passing NULL to the strbuf API is relatively safe, it makes
no sense and the API user should ensure that this never happens. And
so we should not be testing this case.
Change-Id: Icd2323e93ec64afc1822d48e5e1d090083edf539
---
M tests/utils/utils_test.c
M tests/utils/utils_test.ok
2 files changed, 25 insertions(+), 4 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/71/35471/1
diff --git a/tests/utils/utils_test.c b/tests/utils/utils_test.c
index 26c94dc..9ab12a1 100644
--- a/tests/utils/utils_test.c
+++ b/tests/utils/utils_test.c
@@ -1280,9 +1280,6 @@
snprintf(buf, sizeof(buf), "0x2b 0x2b 0x2b...");
printf("4: (need %d chars, had size=0) %s\n", rc, buf);
- rc = strbuf_example2(NULL, 99);
- printf("5: (need %d chars, had NULL buffer)\n", rc);
-
printf("\ncascade:\n");
rc = strbuf_cascade(buf, sizeof(buf));
printf("(need %d chars)\n%s\n", rc, buf);
diff --git a/tests/utils/utils_test.ok b/tests/utils/utils_test.ok
index c0c9560..8a66ba8 100644
--- a/tests/utils/utils_test.ok
+++ b/tests/utils/utils_test.ok
@@ -458,7 +458,6 @@
2: (need 42 chars, had size=42) T minus 10 9 8 7 6 5 4 3 2 1 ... Lift off
3: (need 42 chars, had size=42+1) T minus 10 9 8 7 6 5 4 3 2 1 ... Lift off!
4: (need 42 chars, had size=0) 0x2b 0x2b 0x2b...
-5: (need 42 chars, had NULL buffer)
cascade:
(need 134 chars)
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/35471?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: Icd2323e93ec64afc1822d48e5e1d090083edf539
Gerrit-Change-Number: 35471
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newchange