Attention is currently required from: lynxis lazus.
pespin has posted comments on this change by lynxis lazus. (
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309?usp=email )
Change subject: Add TCAP based loadsharing/routing
......................................................................
Patch Set 8: Code-Review-1
(50 comments)
Patchset:
PS8:
I'd prefer having src/ss7_as_loadshare_tcap.{c,h} renamed to have a "tcap_"
prefix at the start, eg. "tcap_loadshare.c", to have all tcap files together.
Commit Message:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/5466b547_9883224b?usp=email
:
PS8, Line 13: This TCAP session tracking is similar to IP connection tracking.
I guess you mean TCP connection here?
File examples/sccp_demo_user.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/4669b36e_046adb8c?usp=email
:
PS8, Line 30: void *talloc_asn1_ctx;
why is this added?
File src/ipa.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/288b4d18_844341a0?usp=email
:
PS8, Line 51: #include "ss7_as_loadshare_tcap.h"
#ifdef WITH_TCAP_LOADSHARING
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/7f375404_c2cf69c5?usp=email
:
PS8, Line 315: /* update TCAP Transaction Tracking (Rx) */
#ifdef WITH_TCAP_LOADSHARING
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/7a2acee7_1fa70c50?usp=email
:
PS8, Line 316: ss7_asp_tcap_rx_sccp(as, asp, opc, dpc, msg);
This function naming is a bit misleading since it's not even known whether
there's TCAP in the msg...
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/c23f9387_9807ad0d?usp=email
:
PS8, Line 360: case IPAC_PROTO_OSMO:
#ifdef WITH_TCAP_LOADSHARING
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/364ade8c_25d56296?usp=email
:
PS8, Line 368: if (hh_ext->proto == IPAC_PROTO_EXT_TCAP_ROUTING) {
(probably a switch case here will turn out cleaner together with the ifdef.)
File src/m3ua.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/fc0b3dde_fe4349f8?usp=email
:
PS8, Line 50: #include "ss7_as_loadshare_tcap.h"
#ifdef WITH_TCAP_LOADSHARINGActually, I don't see this header being used in
this file. Please have a look...
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/5ef17596_178ebdf8?usp=email
:
PS8, Line 343: if (!xua)
This looks unrelated to this commit? Seems you want to add another commit
describing this.
File src/ss7_as.h:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/4e180de7_8a7159e9?usp=email
:
PS8, Line 6: #include <osmocom/core/hashtable.h>
#ifdef WITH_TCAP_LOADSHARING
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/2aec4c38_576e387f?usp=email
:
PS8, Line 97: /* optimisation: true if tid_ranges contains PCs (not
only wildcards) */
#ifdef WITH_TCAP_LOADSHARING
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/a58f0372_d4b764e0?usp=email
:
PS8, Line 145: /* Should we do load-sharing based on tcap ids?
*/
#ifdef WITH_TCAP_LOADSHARING
File src/ss7_as.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/b16d090f_7d4f90a6?usp=email
:
PS8, Line 40: #include "ss7_as_loadshare_tcap.h"
#ifdef WITH_TCAP_LOADSHARING
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/0bd39052_96eaf541?usp=email
:
PS8, Line 139: hash_init(as->tcap.tid_ranges);
#ifdef WITH_TCAP_LOADSHARING
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/3ee7de6e_d1a7167b?usp=email
:
PS8, Line 227: tcap_asp_down(asp);
I'm not sure this really belongs here. This function (osmo_ss7_as_del_asp) is
to unassign a ASP from an AS, not related to an ASP going down.
It's more an administrative configuration than a state change.
So either change the function name most probably, or move it somewhere else.
And ofc #ifdef WITH_TCAP_LOADSHARING.
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/fa007d58_5f29472f?usp=email
:
PS8, Line 246: tcap_disable(as);
#ifdef WITH_TCAP_LOADSHARING
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/6fe9a695_f0204a96?usp=email
:
PS8, Line 545: case OSMO_SS7_AS_TMOD_LOADSHARE:
#ifdef WITH_TCAP_LOADSHARING
we definetly not want to hit this path for 99% of users every time a packet is
being routed...
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/930ce3fc_6a579a8a?usp=email
:
PS8, Line 634:
extra line not needed.
File src/ss7_as_loadshare_tcap.h:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/e0972887_f89394bb?usp=email
:
PS8, Line 3: #include <complex.h>
I wonder why do we need this here...
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/3b36ee96_ee2e93d5?usp=email
:
PS8, Line 14: #ifdef WITH_TCAP_LOADSHARING
YOu can remove this, simply don't even include it if not requested by
configure...
File src/ss7_as_loadshare_tcap.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/327be0b0_8aec9017?usp=email
:
PS8, Line 61: return osmo_ntohl(*(uint32_t *)src->buf);
is this always properly aligned? maybe use osmo_load_32be or however it's
called?
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/8e582474_9cfaba06?usp=email
:
PS8, Line 72: tcapmsg = talloc_zero(as, struct TCAP_TCMessage);
Can we avoid talloc-allocating a new chunk for every message? Simply "struct
TCAP_TCMessage tcapmsg = {0};" should probably work?
If a talloc context is needed, then probably keeping it as a static variable
and allocating it once is enough.
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/3d5ac30f_dcb8c690?usp=email
:
PS8, Line 138: ssn ^= ((pc >> 24) & 0xff);
A point code is 13 bits in ITU iirc, you seem to be dropping 5 bits here. Is
this expected?
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/b27aa519_90a51406?usp=email
:
PS8, Line 159: static struct osmo_ss7_asp *tcap_hlist_get(struct osmo_ss7_as
*as, uint32_t pc, uint8_t ssn, uint32_t tid)
const as?
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/d1b111de_c44288dc?usp=email
:
PS8, Line 273: /** Traffic from the TCAP ASP -> AS -> osmo-stp, only used to
update transaction tracking
I wonder whether this step is mandatory or should be enabled depending on setup?
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/1ec3f764_9c8a0ae8?usp=email
:
PS8, Line 316: /* TCAP transaction tracking requires point codes */
In theory at some point in the future we can do this also translating from GT
to PC afaiu.
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/d8c3a975_7fc4b37d?usp=email
:
PS8, Line 379: /* FIXME: use UTDS */
This needs to be fixed?
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/cb9b2fd5_78615eaf?usp=email
:
PS8, Line 380: static int sent_back_utds(struct osmo_ss7_as *as,
Please add a description for this function, I'm not getting what's its purpose.
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/3125ceba_07bdf3f3?usp=email
:
PS8, Line 473: * \return
return is not described.
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/428f89d8_eab42cbd?usp=email
:
PS8, Line 475: static int asp_loadshare_tcap_sccp(struct osmo_ss7_asp **rasp,
struct osmo_ss7_as *as, uint32_t opc, uint32_t dpc,
imho the EPROTONOSUPPORT should be dropped, and return rasp instead.
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/fec7012f_f0a9a7ca?usp=email
:
PS8, Line 487: /* decode SCCP and convert to a SUA/xUA representation */
I think you have similar code logic in one of the above functions, you may want
to see if it makes sense to add a helper function to do all checks and fill
data.
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/60f7493c_f5914c55?usp=email
:
PS8, Line 560: sent_back_utds(as, xua, sua,
SCCP_RETURN_CAUSE_SUBSYSTEM_FAILURE);
You may want to look at whether it makes more sense to send a routing failure:
"sccp_rout_fail_enqueue(inst, xua, SCCP_RETURN_CAUSE_MTP_FAILURE,
xua->hdr.msg_class == SUA_MSGC_CO);"
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/3b20b16a_02afe475?usp=email
:
PS8, Line 607: * @return 0 on success
EPROTONOSUPPORT is not documented here. In case, I'd drop EPROTONOSUPPORT and
return asp instead.
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/2043cd0b_7b95e464?usp=email
:
PS8, Line 752: LOGPASP(asp, DLSS7, LOGL_ERROR, "TCAP routing
message is too small\n");
I think we really want to add a new log category "TCAP" here.
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/283d18cb_a977ae72?usp=email
:
PS8, Line 921: /** Called when the ASP is going down or free'd
Then you are currently calling it fromthe wrong place apparently.
File src/ss7_as_vty.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/218178ec_7c44d73a?usp=email
:
PS8, Line 39: #include "ss7_as_loadshare_tcap.h"
#ifdef WITH_TCAP_LOADSHARING
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/10e26bb1_36d7306e?usp=email
:
PS8, Line 158: tcap_disable(as);
#ifdef WITH_TCAP_LOADSHARING
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/3d196d11_0484e1eb?usp=email
:
PS8, Line 165: "traffic-mode (loadshare|loadshare-tcap) [bindings] [sls]
[opc-sls] [opc-shift] [<0-2>]",
IMHO loadshare-tcap feature should have a separate command to enable/disable
it. Leave alone the traffic-mode loadshare one, is already complex enough.
Otherwise you are mixing concepts in different protocol layers.
Eg: "[no] tcap-loadshare"
Then, drop all the tcap_disable/enable() from all "traffic-mode" commands.
File src/ss7_asp.h:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/e722613c_eacce93c?usp=email
:
PS8, Line 96: bool enabled;
is this really needed? Does it make sense to have tcap loadsharing enabled in
only some ASPs of the AS? I doubt so.
File src/ss7_asp.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/0f7958cc_99510fb4?usp=email
:
PS8, Line 53: #include "ss7_as_loadshare_tcap.h"
#ifdef WITH_TCAP_LOADSHARING
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/4f900a11_d2a3669f?usp=email
:
PS8, Line 1216: tcap_asp_down(asp);
nooo way you are calling this here. Put it where it belongs in the FSM.
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/64863fbc_c5846a48?usp=email
:
PS8, Line 1344: tcap_asp_down(asp);
nooo way you are calling this here. Put it where it belongs in the FSM
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/fe6a2bb9_8dddf5fe?usp=email
:
PS8, Line 1435: /*! Received an unknown packet on asp
Unrelated to this commit, please submit another one.
File src/ss7_internal.h:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/23fe8af9_75c77ac6?usp=email
:
PS8, Line 9: #include <osmocom/sigtran/mtp_sap.h>
why is this now needed here? if needed, please submit another commit.
File src/tcap_transaction_tracking.h:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/2e560cc7_c465a318?usp=email
:
PS8, Line 36: struct tcap_trxn_track_entry *tcap_trxn_track_create_entry(
We usually have the struct name and then the action, ie.
tcap_trxn_track_entry_create()
Same applies for all the other APIs.
File src/tcap_transaction_tracking.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/6fcfc093_e8bd2c24?usp=email
:
PS8, Line 122: LOGPASP(entry->asp, DLSS7, LOGL_DEBUG, "Creating tcap cache,
entry (own) pc/ssn/tid %u/%u/%u -> %u/%u/%u\n",
we always want to log PC in both numeric and configured format. See
osmo_ss7_pointcode_print_buf().
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/d88f79ec_58c02549?usp=email
:
PS8, Line 141: if (entry->own_tid.tid_valid)
You can probably simplify this strcut removing these "valid" fields by
initializing entry->own_tid.list and then checking if it is empty.
File src/xua_as_fsm.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/d78c708a_8df17e0e?usp=email
:
PS8, Line 204: asp = ss7_as_select_asp(as, xua);
This could actually go in a preparatory patch to simplify this one a bit.
File stp/stp_main.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/13b34653_b21f4108?usp=email
:
PS8, Line 52: void *talloc_asn1_ctx;
Why is this needed?
--
To view, visit https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: libosmo-sigtran
Gerrit-Branch: master
Gerrit-Change-Id: Ibcb48aa0e515ad346f59ddd84b24c6e2c026144d
Gerrit-Change-Number: 41309
Gerrit-PatchSet: 8
Gerrit-Owner: lynxis lazus <[email protected]>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <[email protected]>
Gerrit-Attention: lynxis lazus <[email protected]>
Gerrit-Comment-Date: Mon, 10 Nov 2025 11:56:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
