14 Jul
2025
14 Jul
'25
1:50 p.m.
Attention is currently required from: laforge, osmith, pespin.
neels has posted comments on this change by pespin. (
https://gerrit.osmocom.org/c/osmo-msc/+/40630?usp=email )
Change subject: msc: Initial implementation of N-PCSTATE.ind
......................................................................
Patch Set 3: Code-Review+1
(10 comments)
Patchset:
PS3:
some optional nitpicks..
File include/osmocom/msc/ran_peer.h:
https://gerrit.osmocom.org/c/osmo-msc/+/40630/comment/1576b4ec_fb08873f?usp… :
PS3, Line 94: struct ran_peer *ran_peer_find_by_addr(const struct sccp_ran_inst *sri,
const struct osmo_sccp_addr *peer_addr);
please consider placing orthogonal cosmetic fixes of prior code in a separate patch,
because you should know that it steals time from every reader
File src/libmsc/ran_peer.c:
https://gerrit.osmocom.org/c/osmo-msc/+/40630/comment/252e3ef4_2e26dc29?usp… :
PS3, Line 131: /* N-PCSTATE.ind informs us the peer went down and is no longer reachable:
*/
in this comment i expect to read what the *function* does
File src/libmsc/sccp_ran.c:
https://gerrit.osmocom.org/c/osmo-msc/+/40630/comment/87204a15_e8441d73?usp… :
PS3, Line 63: address
you mean PC? PC is a subset of possible SCCP addresses
https://gerrit.osmocom.org/c/osmo-msc/+/40630/comment/8a2c6af2_0c48aa76?usp… :
PS3, Line 63: n r
typo
https://gerrit.osmocom.org/c/osmo-msc/+/40630/comment/be104396_61685ce3?usp… :
PS3, Line 64: get_ran_
below you call function ran_peer_find_by_addr().
let's use the same naming pattern here: ran_peer_find_by_pc()
secondly, consider putting this function in ran_peer.h/c because static functions hidden
in .c files tend to be duplicated; and this looks generally useful.
https://gerrit.osmocom.org/c/osmo-msc/+/40630/comment/483a5b43_0dbdd2fb?usp… :
PS3, Line 74: LOGP(DMSC, LOGL_DEBUG, "No ran_peer found under remote address:
%s\n", osmo_sccp_addr_name(cs7, &rem_addr));
(if this function becomes public API: error logging should be done at the caller, because
these helpers tend to be called from various places, half of which expect to gracefully
handle missing objects instead of logging)
https://gerrit.osmocom.org/c/osmo-msc/+/40630/comment/dc8ad4bf_dafccf55?usp… :
PS3, Line 85: LOGP(DMSC, LOGL_DEBUG, "N-PCSTATE ind: affected_pc=%u=%s sp_status=%s
remote_sccp_status=%s\n",
(not sure if we should log *all* N-PCSTATE events)
https://gerrit.osmocom.org/c/osmo-msc/+/40630/comment/9cdd08e2_20a07df3?usp… :
PS3, Line 95: /* See if this marks the point code to have become available, or to have
been lost.
hehe i know this comment from somewhere =) high five
https://gerrit.osmocom.org/c/osmo-msc/+/40630/comment/a72fb695_5a1e53f2?usp… :
PS3, Line 146: }
(... this logic exists also in osmo-bsc.git, should we move this to a library and re-use
instead of copying?)
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/40630?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: Ice1b2c163b1b0d134fcaa1c8bf543038a35fabdf
Gerrit-Change-Number: 40630
Gerrit-PatchSet: 3
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-Attention: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Mon, 14 Jul 2025 13:50:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes