14 Jul
2025
14 Jul
'25
3:34 p.m.
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… :
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… :
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… :
PS3, Line 63: n r
typo
Done
https://gerrit.osmocom.org/c/osmo-msc/+/40630/comment/6c391fe4_c39670cd?usp… :
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… :
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… :
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… :
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… :
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… :
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 <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: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Mon, 14 Jul 2025 15:34:08 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>