20 Feb
2025
20 Feb
'25
3:28 p.m.
Attention is currently required from: laforge, lynxis lazus.
neels has posted comments on this change by lynxis lazus. (
https://gerrit.osmocom.org/c/osmo-iuh/+/38946?usp=email )
Change subject: iu_client: add ranap_iu_page_cs2/ranap_iu_page_ps2
......................................................................
Patch Set 13: Code-Review+1
(14 comments)
Patchset:
PS13:
in the end, after CR, i notice that this is the iu_client.c code -- which osmo-msc does
not use anymore IIRC, meaning that all of the _cs code here is dead? So it might make
sense to drop iu_client and copy what osmo-sgsn needs into osmo-sgsn.git. iu_client was an
early attempt by me to abstract some common parts between SGSN and MSC, but ended up
making more problems than helping. At some point osmo-msc (years ago) started its own copy
of this code, to move independently.
we can merge this if you like, of course.
Commit Message:
https://gerrit.osmocom.org/c/osmo-iuh/+/38946/comment/f2099148_d214ac50?usp… :
PS13, Line 9: new
hehe, the slightly newER =)
File src/iu_client.c:
https://gerrit.osmocom.org/c/osmo-iuh/+/38946/comment/6e0975dd_51b7ab46?usp… :
PS13, Line 842: LOGL_ERROR
(marking: this was ERROR)
https://gerrit.osmocom.org/c/osmo-iuh/+/38946/comment/e50ff02b_10431cb2?usp… :
PS13, Line 869: int ranap_iu_page_cs2(const char *imsi, const uint32_t *tmsi, const struct
osmo_location_area_id *lai)
it would be great to have an API doc explaining how version 2 behaves differently
https://gerrit.osmocom.org/c/osmo-iuh/+/38946/comment/1911863e_194c2777?usp… :
PS13, Line 879: exact
"exactly one"
-- but is that overall exactly one paging to exactly one RNC?
the code looks more like one per RNC that matches the lai ..
am i even making sense, can multiple RNC match the same LAI = PLMN+LAC?
https://gerrit.osmocom.org/c/osmo-iuh/+/38946/comment/a29c4113_f6c6027c?usp… :
PS13, Line 883: if (!osmo_lai_cmp(&entry->rai.lac, lai)) {
(early exit: 'if (osmo_lai_cmp(..)) continue;')
https://gerrit.osmocom.org/c/osmo-iuh/+/38946/comment/c0da0d5a_54001c60?usp… :
PS13, Line 893: }
if it's exactly one, then it seems that this loop could also get an 'if (paged)
break;' ?
https://gerrit.osmocom.org/c/osmo-iuh/+/38946/comment/8fa93a3f_bbfa1220?usp… :
PS13, Line 900: %d RN
(then again this looks like multiple matches are possible)
please use snprintf() with sizeof(log_msg)
https://gerrit.osmocom.org/c/osmo-iuh/+/38946/comment/1af45be7_bb0f1a1c?usp… :
PS13, Line 909: LOGPIUC(log_level, ", for IMSI %s\n", imsi);
please avoid LOGPC, because for gsm_tap_log it rips apart log lines to multiple gsmtap
packages. so let's rather add ugly 'tmsi ? "TMSI" :
"IMSI"' args, and a '\n', above
https://gerrit.osmocom.org/c/osmo-iuh/+/38946/comment/451aebd7_4e830839?usp… :
PS13, Line 914: int ranap_iu_page_ps2(const char *imsi, const uint32_t *ptmsi, const
struct osmo_routing_area_id *rai)
api doc
https://gerrit.osmocom.org/c/osmo-iuh/+/38946/comment/98190d1a_2093f173?usp… :
PS13, Line 938: }
if (paged) break?
https://gerrit.osmocom.org/c/osmo-iuh/+/38946/comment/580719c6_dc639a0e?usp… :
PS13, Line 943: LOGL_INFO
this previously was LOGL_ERROR and the _cs2 also does ERROR.
If it should change, explain in commit log and/or move the change to separate commit?
https://gerrit.osmocom.org/c/osmo-iuh/+/38946/comment/dce94ce9_ab46de9f?usp… :
PS13, Line 945: sprintf(log_msg, "Paged %d RNCs", paged);
snprintf()
https://gerrit.osmocom.org/c/osmo-iuh/+/38946/comment/f1ec434f_a799bb32?usp… :
PS13, Line 954: LOGPIUC(log_level, ", for IMSI %s\n", imsi);
avoid LOGPC
--
To view, visit https://gerrit.osmocom.org/c/osmo-iuh/+/38946?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-iuh
Gerrit-Branch: master
Gerrit-Change-Id: I1f07e96642737160d387de3e4c3f71d288d356dd
Gerrit-Change-Number: 38946
Gerrit-PatchSet: 13
Gerrit-Owner: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: lynxis lazus <lynxis(a)fe80.eu>
Gerrit-Comment-Date: Thu, 20 Feb 2025 14:28:11 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes