Attention is currently required from: lynxis lazus.
Patch set 8:Code-Review -1
50 comments:
Patchset:
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:
Patch Set #8, 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:
Patch Set #8, Line 30: void *talloc_asn1_ctx;
why is this added?
File src/ipa.c:
Patch Set #8, Line 51: #include "ss7_as_loadshare_tcap.h"
#ifdef WITH_TCAP_LOADSHARING
Patch Set #8, Line 315: /* update TCAP Transaction Tracking (Rx) */
#ifdef WITH_TCAP_LOADSHARING
Patch Set #8, 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...
Patch Set #8, Line 360: case IPAC_PROTO_OSMO:
#ifdef WITH_TCAP_LOADSHARING
Patch Set #8, 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:
Patch Set #8, 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...
Patch Set #8, Line 343: if (!xua)
This looks unrelated to this commit? Seems you want to add another commit describing this.
File src/ss7_as.h:
Patch Set #8, Line 6: #include <osmocom/core/hashtable.h>
#ifdef WITH_TCAP_LOADSHARING
Patch Set #8, Line 97: /* optimisation: true if tid_ranges contains PCs (not only wildcards) */
#ifdef WITH_TCAP_LOADSHARING
Patch Set #8, Line 145: /* Should we do load-sharing based on tcap ids? */
#ifdef WITH_TCAP_LOADSHARING
File src/ss7_as.c:
Patch Set #8, Line 40: #include "ss7_as_loadshare_tcap.h"
#ifdef WITH_TCAP_LOADSHARING
Patch Set #8, Line 139: hash_init(as->tcap.tid_ranges);
#ifdef WITH_TCAP_LOADSHARING
Patch Set #8, 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.
Patch Set #8, Line 246: tcap_disable(as);
#ifdef WITH_TCAP_LOADSHARING
Patch Set #8, 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...
extra line not needed.
File src/ss7_as_loadshare_tcap.h:
Patch Set #8, Line 3: #include <complex.h>
I wonder why do we need this here...
Patch Set #8, 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:
Patch Set #8, Line 61: return osmo_ntohl(*(uint32_t *)src->buf);
is this always properly aligned? maybe use osmo_load_32be or however it's called?
Patch Set #8, 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.
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. Is this expected?
Patch Set #8, 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?
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?
Patch Set #8, 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.
Patch Set #8, Line 379: /* FIXME: use UTDS */
This needs to be fixed?
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.
Patch Set #8, Line 473: * \return
return is not described.
Patch Set #8, 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.
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 sense to add a helper function to do all checks and fill data.
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:
"sccp_rout_fail_enqueue(inst, xua, SCCP_RETURN_CAUSE_MTP_FAILURE, xua->hdr.msg_class == SUA_MSGC_CO);"
Patch Set #8, Line 607: * @return 0 on success
EPROTONOSUPPORT is not documented here. In case, I'd drop EPROTONOSUPPORT and return asp instead.
Patch Set #8, 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.
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.
File src/ss7_as_vty.c:
Patch Set #8, Line 39: #include "ss7_as_loadshare_tcap.h"
#ifdef WITH_TCAP_LOADSHARING
Patch Set #8, Line 158: tcap_disable(as);
#ifdef WITH_TCAP_LOADSHARING
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. 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:
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 AS? I doubt so.
File src/ss7_asp.c:
Patch Set #8, Line 53: #include "ss7_as_loadshare_tcap.h"
#ifdef WITH_TCAP_LOADSHARING
Patch Set #8, Line 1216: tcap_asp_down(asp);
nooo way you are calling this here. Put it where it belongs in the FSM.
Patch Set #8, Line 1344: tcap_asp_down(asp);
nooo way you are calling this here. Put it where it belongs in the FSM
Patch Set #8, Line 1435: /*! Received an unknown packet on asp
Unrelated to this commit, please submit another one.
File src/ss7_internal.h:
Patch Set #8, 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:
Patch Set #8, 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:
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().
Patch Set #8, 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:
Patch Set #8, 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:
Patch Set #8, Line 52: void *talloc_asn1_ctx;
Why is this needed?
To view, visit change 41309. To unsubscribe, or for help writing mail filters, visit settings.