Attention is currently required from: neels, laforge.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/33169 )
Change subject: fix umts_cell_id_name(): show CID
......................................................................
Patch Set 3: Code-Review+1
(1 comment)
Commit Message:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33169/comment/11674beb_58da39d6
PS3, Line 12: Use OTC_SELECT
> I'm also a bit worried about this change, depending on how often we end up logging the cell-ID. […]
That's usually my opinion too, but since I have the feeling I'm usually the only one against this I refrain from making the same comments again and again. But yeah, I second that and I would avoid heap allocation on code paths potentially used tons of times, like some function used to log some object.
--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33169
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: Id903b8579ded8b908e644808e440d373bcca8da4
Gerrit-Change-Number: 33169
Gerrit-PatchSet: 3
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Wed, 07 Jun 2023 09:56:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Gerrit-MessageType: comment
Attention is currently required from: neels.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/33179 )
Change subject: include Global RNC-ID in RESET
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
File src/osmo-hnbgw/cnlink.c:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33179/comment/7586cbc1_441ddb35
PS4, Line 147: /
why are we handling that case if it is in violation of the 3GPP specs? For backwards compatibility with old config files? In that case it is ok but we should probably warn the user (via the log) that his config is missing vital information and hence we're sending non-compliant messages.
--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33179
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: I2cd5d7ea86a6ed0916befe219dbf21373afbd95b
Gerrit-Change-Number: 33179
Gerrit-PatchSet: 4
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 07 Jun 2023 09:56:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: neels.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/33177 )
Change subject: cnpool: return Paging Resp to the exact CN link that Paged
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33177
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: I907dbcaeb442ca5630146f8cad40601c448fc40e
Gerrit-Change-Number: 33177
Gerrit-PatchSet: 4
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 07 Jun 2023 09:53:59 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: neels.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/33176 )
Change subject: use cnlink state in cnpool decisions
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33176
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: I28490a4a27bcda8fd689db8b8652e451103e3a9d
Gerrit-Change-Number: 33176
Gerrit-PatchSet: 4
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 07 Jun 2023 09:53:32 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
dexter has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/32733 )
Change subject: paging: parse PCUIF data indication outside of paging.c
......................................................................
paging: parse PCUIF data indication outside of paging.c
The function paging_add_macblock takes a data and length parameter. The
first three byte of that string are the last three digits of the IMSI
from which the paging group is calculated.
As the layout of this data buffer is a property of the PCUIF interface
API, we should do this separation outside of paging.c. Also we should
supply the IMSI as a valid null terminated string since PCUIF v.11 also
uses this format.
Change-Id: I9f3799916e8102bf1ce0f21891f2d24f43091f01
Related: OS#5927
---
M include/osmo-bts/paging.h
M src/common/paging.c
M src/common/pcu_sock.c
3 files changed, 42 insertions(+), 16 deletions(-)
Approvals:
pespin: Looks good to me, but someone else must approve
fixeria: Looks good to me, approved
Jenkins Builder: Verified
diff --git a/include/osmo-bts/paging.h b/include/osmo-bts/paging.h
index f1ae8e3..c009831 100644
--- a/include/osmo-bts/paging.h
+++ b/include/osmo-bts/paging.h
@@ -37,8 +37,7 @@
/* Add a ready formatted MAC block message to the paging queue, this can be an IMMEDIATE ASSIGNMENT, or a
* PAGING COMMAND (from the PCU) */
-int paging_add_macblock(struct paging_state *ps,
- const uint8_t *data, uint8_t len);
+int paging_add_macblock(struct paging_state *ps, const char *imsi, const uint8_t *macblock);
/* generate paging message for given gsm time */
int paging_gen_msg(struct paging_state *ps, uint8_t *out_buf, struct gsm_time *gt,
diff --git a/src/common/paging.c b/src/common/paging.c
index 284b71c..f49a36b 100644
--- a/src/common/paging.c
+++ b/src/common/paging.c
@@ -269,12 +269,13 @@
/* Add a ready formatted MAC block message to the paging queue, this can be an IMMEDIATE ASSIGNMENT, or a
* PAGING COMMAND (from the PCU) */
-int paging_add_macblock(struct paging_state *ps,
- const uint8_t *data, uint8_t len)
+int paging_add_macblock(struct paging_state *ps, const char *imsi, const uint8_t *macblock)
{
struct llist_head *group_q;
struct paging_record *pr;
- uint16_t imsi, paging_group;
+ uint16_t paging_group;
+ uint16_t _imsi;
+ size_t imsi_len = strlen(imsi);
check_congestion(ps);
@@ -285,17 +286,16 @@
return -ENOSPC;
}
- if (len != GSM_MACBLOCK_LEN + 3) {
- LOGP(DPAG, LOGL_ERROR, "MAC block with invalid length %d (GSM_MACBLOCK_LEN + 3)\n", len);
+ /* Tha paging group is calculated from the last three digits of the IMSI */
+ if (imsi_len < 3) {
+ LOGP(DPAG, LOGL_ERROR, "IMSI with invalid length %zu (expecting at least the last 3 digits)\n", imsi_len);
return -EINVAL;
}
- len -= 3;
-
- imsi = 100 * ((*(data++)) - '0');
- imsi += 10 * ((*(data++)) - '0');
- imsi += (*(data++)) - '0';
- paging_group = gsm0502_calc_paging_group(&ps->chan_desc, imsi);
-
+ imsi = imsi + imsi_len - 3;
+ _imsi = 100 * ((*(imsi++)) - '0');
+ _imsi += 10 * ((*(imsi++)) - '0');
+ _imsi += (*(imsi++)) - '0';
+ paging_group = gsm0502_calc_paging_group(&ps->chan_desc, _imsi);
group_q = &ps->paging_queue[paging_group];
pr = talloc_zero(ps, struct paging_record);
@@ -305,7 +305,7 @@
LOGP(DPAG, LOGL_INFO, "Add MAC block to paging queue (group=%u)\n",
paging_group);
- memcpy(pr->u.macblock.msg, data, GSM_MACBLOCK_LEN);
+ memcpy(pr->u.macblock.msg, macblock, GSM_MACBLOCK_LEN);
/* enqueue the new message to the HEAD of the queue */
llist_add(&pr->list, group_q);
diff --git a/src/common/pcu_sock.c b/src/common/pcu_sock.c
index 00debad..ea244b8 100644
--- a/src/common/pcu_sock.c
+++ b/src/common/pcu_sock.c
@@ -667,6 +667,7 @@
struct gsm_bts_trx_ts *ts;
struct msgb *msg;
int rc = 0;
+ char imsi[4];
LOGP(DPCU, LOGL_DEBUG, "Data request received: sapi=%s arfcn=%d "
"block=%d data=%s\n", sapi_string[data_req->sapi],
@@ -675,7 +676,14 @@
switch (data_req->sapi) {
case PCU_IF_SAPI_PCH:
- paging_add_macblock(bts->paging_state, data_req->data, data_req->len);
+ OSMO_STRLCPY_ARRAY(imsi, (char *)data_req->data);
+ if (data_req->len-3 != GSM_MACBLOCK_LEN) {
+ LOGP(DPCU, LOGL_ERROR, "MAC block with invalid length %d (expecting GSM_MACBLOCK_LEN = %d)\n",
+ data_req->len-3, GSM_MACBLOCK_LEN);
+ rc = -ENOMEM;
+ break;
+ }
+ paging_add_macblock(bts->paging_state, imsi, data_req->data + 3);
break;
case PCU_IF_SAPI_AGCH:
msg = msgb_alloc(data_req->len, "pcu_agch");
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/32733
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I9f3799916e8102bf1ce0f21891f2d24f43091f01
Gerrit-Change-Number: 32733
Gerrit-PatchSet: 9
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: merged
Attention is currently required from: neels, pespin.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/33173 )
Change subject: cnpool: add context_map_cnlink_lost() handling
......................................................................
Patch Set 4:
(1 comment)
File include/osmocom/hnbgw/context_map.h:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33173/comment/01676d82_831c3e13
PS3, Line 70: MAP_SCCP_EV_CN_LINK_LOST,
> if it has RESET, why calling it LOST? :D
I'm also a bit confused. "RESET" to me means some IuReset procedure is happening using RANAP messages over an established link. link-lost means something different.
--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33173
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: Ic0a6fcfb747dc093528ca2bd12a269ad390d465c
Gerrit-Change-Number: 33173
Gerrit-PatchSet: 4
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 07 Jun 2023 09:52:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: dexter.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/32734 )
Change subject: paging: do not confirm PAGING COMMAND messages
......................................................................
Patch Set 12:
(1 comment)
File src/common/pcu_sock.c:
https://gerrit.osmocom.org/c/osmo-bts/+/32734/comment/1716b5dc_25d99523
PS12, Line 684: gsm48_imm_ass->msg_type
Another problem here is that you're accessing `data_req->data` (via `struct gsm48_imm_ass *`) before checking the `data_req->len`. This is not super critical, because AFAIR the `data_req->data` is a static buffer of some large size. But still, it's more logical to check the data length first.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/32734
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I8b8264d28b1b1deb08774cdba58dd4c6dafe115d
Gerrit-Change-Number: 32734
Gerrit-PatchSet: 12
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 07 Jun 2023 09:51:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment