Attention is currently required from: laforge, osmith, pespin.
Patch set 3:Code-Review +1
10 comments:
Patchset:
some optional nitpicks..
File include/osmocom/msc/ran_peer.h:
Patch Set #3, 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:
Patch Set #3, 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:
Patch Set #3, Line 63: address
you mean PC? PC is a subset of possible SCCP addresses
typo
Patch Set #3, 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.
Patch Set #3, 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)
Patch Set #3, 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)
Patch Set #3, 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
(... this logic exists also in osmo-bsc.git, should we move this to a library and re-use instead of copying?)
To view, visit change 40630. To unsubscribe, or for help writing mail filters, visit settings.