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_48ad47... : 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_674e0e... : 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_c6c541... : 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_6af956... : 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_2c112d... : 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_d79ae3... : 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_7cf0a1... : 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_27aaa3... : PS8, Line 39: #include "ss7_as_loadshare_tcap.h"
#ifdef WITH_TCAP_LOADSHARING
No
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/bb387f15_ffbf28... : PS8, Line 158: tcap_disable(as);
#ifdef WITH_TCAP_LOADSHARING
No, not needed.
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/3f6f022b_d1a5be... : 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.
@dwillmann@sysmocom.de @laforge@osmocom.org 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_cd9137... : 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_cfca5b... : 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_918950... : 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_87102a... : 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_999a02... : 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_3d5de1... : PS8, Line 52: void *talloc_asn1_ctx;
Why is this needed?
Need to check it again, if it is still required.