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=email : 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=email : 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=email : 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=email : PS3, Line 63: n r typo https://gerrit.osmocom.org/c/osmo-msc/+/40630/comment/be104396_61685ce3?usp=email : 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=email : 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=email : 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=email : 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=email : 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 <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <[email protected]> Gerrit-Reviewer: neels <[email protected]> Gerrit-Reviewer: osmith <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-CC: laforge <[email protected]> Gerrit-Attention: osmith <[email protected]> Gerrit-Attention: laforge <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Comment-Date: Mon, 14 Jul 2025 13:50:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes
