This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Patch Set 1: Code-Review-1
(19 comments)
https://gerrit.osmocom.org/#/c/3303/1//COMMIT_MSG
Commit Message:
Line 7: sccp: make simple client configurable via VTY
I don't see how this patch relates to the VTY. AFAICT it rather goes to great lengths to allow calling simple_client() multiple times on the same ss7 instance, the reason not being clear. How does a VTY config end up in this function? Why is re-entrant client creation necessary? >90% of this patch are not needed if we disallow multiple simple_client calls per ss7 instance, so we need a good reason.
Line 12: Add additional logic that checks for a still existing, valid
("already existing")
Line 14: assume use the caller supplied parameters as standard configuration.
assume?
https://gerrit.osmocom.org/#/c/3303/1/src/osmo_ss7.c
File src/osmo_ss7.c:
Line 836: /*! \brief Find Application Server by given protocol.
missing explanation:
If an AS has an ASP also matching the given protocol, that AS is preferred.
If there are multiple matches, return the first matching AS.
Line 844: struct osmo_ss7_as *second_best_as = NULL;
(var name bikeshed: 'as_without_asp')
Line 850: * where the proto matches up */
(rather make use of the entire line width, by your choice either 80 or since recently up to 120. But let's not spread out the comments unnecessarily to match the other osmocom code. same below.)
Line 855: * proto, just in cas we will not find any
typo
Line 860: /* Check if the candicate we have here as
typo x2
Line 862: for (i = 0; i < ARRAY_SIZE(as->cfg.asps); i++) {
don't duplicate code, rather re-use below osmo_ss7_asp_find_by_proto()
https://gerrit.osmocom.org/#/c/3303/1/src/sccp_user.c
File src/sccp_user.c:
Line 255: * valid port numbers */
(use line width, same below)
Line 278: * proper value when a cs7 instance is defined via the VTY */
otherwise what happens? can we make the code protect us from adverse effects if the user fails to do it properly, i.e. fail with an error message?
Line 288: if (ss7->sccp) {
If we really need to call this more than once: verify that this matches *all* arguments passed to this function: as->cfg.routing_key, asp->name and ports, asp->cfg.local.host asp->cfg.remote.host, protocol. We should not silently ignore mismatches = highly confusing to a user.
What would be the reaction to mismatching conf? Since there can be only one sccp, it must be a fatal error AFAICT.
Line 315: }
else { check matching function arguments }
Line 320: * that is associated with the application server we have choosen
(chosen)
(please end sentences with proper punctuation, don't rely on line breaks)
Line 326: "%s: No ASP instance found, autocreating one...\n", name);
Rather drop the "No ASP instance found", it looks like an error, but it will always be printed for the first call: just "Creating ASP instance...".
Also add the asp_name as well as the protocol string and ports.
OTOH, isn't osmo_ss7_* already logging this? In that case we could drop this logging.
Line 340: }
else { ensure matching parameters }
Line 343: asp->cfg.is_server = false;
if the asp already existed, you may be changing an ASP intended for a different purpose. It seems more and more appropriate to only allow one simple_client call per ss7 instance and skip all of these complications.
Line 347: osmo_ss7_asp_use_default_lm(asp, LOGL_DEBUG);
it seems osmo_ss7_asp_use_default_lm() does not guard against an asp->lm_priv FSM instance already existing. It simply drops the previous fi on the floor = mem leak.
I guess we need to fix that function separately, but another point against allowing re-use of simple-client.
Line 353: LOGP(DLSCCP, LOGL_NOTICE, "%s: Creating SCCP instance...\n", name);
(the '...' implies that it takes a while, but it's instantaneous)
--
To view, visit https://gerrit.osmocom.org/3303
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I293f3526ce6182dca74a169a23449dbc7af57c7c
Gerrit-PatchSet: 1
Gerrit-Project: libosmo-sccp
Gerrit-Branch: master
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-HasComments: Yes