Attention is currently required from: daniel, laforge, pespin. lynxis lazus 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: (16 comments) File src/ss7_as_loadshare_tcap.c: https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/4c39a77e_48ad4773?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. […] Wikipedia says it's 14 bits for ITU. I've reserved 24 bits here for the point code (even we don't do ANSI). We don't drop any bits here. XOR the high 8 bits of PC with the SSN. In case this is a WILDCARD. https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/80b1e111_674e0ea2?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? Yes it is. If TCAP is enabled on the ASP, it must track both, Rx and Tx. https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/5f0d23f4_c6c54128?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. It sends back an SCCP/UTDS back to the origin https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/90047b54_6af956d4?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 s […] I don't really see the gain from move the code back into an own function with a lot of pointers. It complicates the code for a minimal gain of code de-duplication. https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/d94640e6_2c112da0?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: […] An UTDS was requested. I don't know enough of SCCP. But the feature request explicit requeseted an UTDS. I don't know why we never sent back an UTDS before. https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/6e8e96e1_d79ae3b4?usp=email : PS8, Line 607: * @return 0 on success > EPROTONOSUPPORT is not documented here. In case, I'd drop EPROTONOSUPPORT and > return asp instead. There is a difference between (rc = EPROTONOSUPPORT) and (rc = 0 && asp == NULL.) PROTONOSUPPORTED will do a regular AS loadsharing. While rc = 0; asp = NULL will drop the message. (Because an UTDS is already sent to the origin). https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/ed73b488_7cf0a1c6?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. Why? File src/ss7_as_vty.c: https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/6244559b_27aaa383?usp=email : PS8, Line 39: #include "ss7_as_loadshare_tcap.h" > #ifdef WITH_TCAP_LOADSHARING No https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/bb387f15_ffbf28b6?usp=email : PS8, Line 158: tcap_disable(as); > #ifdef WITH_TCAP_LOADSHARING No, not needed. https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/3f6f022b_d1a5be0e?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. […] traffic-mode loadshare-tcap is an extension to the general loadshare. @[email protected] @[email protected] I took this over from your origin work. Maybe you have an opinion on it. https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/3cf2e9b7_cd913748?usp=email : PS8, Line 186: vty_out(vty, "tcap loadsharing is not supported in this build.\n"); This is wrong, will fix it in v2 File src/ss7_asp.h: https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/08f4fd96_cfca5b03?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 […] Not all ASP of an AS has it enabled. Only when the IPA/TCAP is called, it is enabled for this particular ASP. File src/ss7_asp.c: https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/c71f3567_91895014?usp=email : PS8, Line 1344: tcap_asp_down(asp); > nooo way you are calling this here. […] Where do you think it belongs exactly? I'm not used to this code base. https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/bbfce463_87102a37?usp=email : PS8, Line 1435: /*! Received an unknown packet on asp > Unrelated to this commit, please submit another one. Will look into it. File src/tcap_transaction_tracking.c: https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/9436428f_999a025c?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(). Ack File stp/stp_main.c: https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/d28158a7_3d5de1af?usp=email : PS8, Line 52: void *talloc_asn1_ctx; > Why is this needed? Need to check it again, if it is still required. -- 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-CC: daniel <[email protected]> Gerrit-CC: laforge <[email protected]> Gerrit-Attention: laforge <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Attention: daniel <[email protected]> Gerrit-Comment-Date: Mon, 10 Nov 2025 17:29:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <[email protected]>
