Attention is currently required from: laforge, neels, osmith. pespin 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: (10 comments) Patchset: PS3: > some optional nitpicks.. Done File include/osmocom/msc/ran_peer.h: https://gerrit.osmocom.org/c/osmo-msc/+/40630/comment/5dc978db_d565762c?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 sho […] Done File src/libmsc/ran_peer.c: https://gerrit.osmocom.org/c/osmo-msc/+/40630/comment/a51992a2_2c86a264?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 Done File src/libmsc/sccp_ran.c: https://gerrit.osmocom.org/c/osmo-msc/+/40630/comment/96c603d3_97769765?usp=email : PS3, Line 63: n r > typo Done https://gerrit.osmocom.org/c/osmo-msc/+/40630/comment/6c391fe4_c39670cd?usp=email : PS3, Line 63: address > you mean PC? PC is a subset of possible SCCP addresses It doesn't really matter, since the SSN is known/hardcoded in this case. https://gerrit.osmocom.org/c/osmo-msc/+/40630/comment/42ca709f_41e228dd?usp=email : PS3, Line 64: get_ran_ > below you call function ran_peer_find_by_addr(). […] Done https://gerrit.osmocom.org/c/osmo-msc/+/40630/comment/85d4da41_6a02a2b0?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 help […] Done https://gerrit.osmocom.org/c/osmo-msc/+/40630/comment/6b0d894d_0ae1e076?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) It's in debug, so sure we should. https://gerrit.osmocom.org/c/osmo-msc/+/40630/comment/facd68d8_674f01aa?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 Yeah I'm reusing stuff from osmo-bsc which had better support already, https://gerrit.osmocom.org/c/osmo-msc/+/40630/comment/050e2f92_9270ddc7?usp=email : PS3, Line 146: } > (... this logic exists also in osmo-bsc. […] No, I believe this is really logic of apps should take care of, specially since we'll be improving it at different pace in different apps in the future. -- 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: neels <[email protected]> Gerrit-Attention: laforge <[email protected]> Gerrit-Comment-Date: Mon, 14 Jul 2025 15:34:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <[email protected]>
