Attention is currently required from: daniel, laforge, pespin.
16 comments:
File src/ss7_as_loadshare_tcap.c:
Patch Set #8, 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.
Patch Set #8, 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.
Patch Set #8, 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
Patch Set #8, 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.
Patch Set #8, 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.
Patch Set #8, 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).
Patch Set #8, 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:
Patch Set #8, Line 39: #include "ss7_as_loadshare_tcap.h"
#ifdef WITH_TCAP_LOADSHARING
No
Patch Set #8, Line 158: tcap_disable(as);
#ifdef WITH_TCAP_LOADSHARING
No, not needed.
Patch Set #8, 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.
Patch Set #8, 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:
Patch Set #8, 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:
Patch Set #8, 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.
Patch Set #8, 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:
Patch Set #8, 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:
Patch Set #8, Line 52: void *talloc_asn1_ctx;
Why is this needed?
Need to check it again, if it is still required.
To view, visit change 41309. To unsubscribe, or for help writing mail filters, visit settings.