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.