Attention is currently required from: laforge, lynxis lazus, pespin.
daniel 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 10:
(20 comments)
File examples/sccp_demo_user.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/ac650df0_151b47... : PS8, Line 30: void *talloc_asn1_ctx;
If it is required, do you mind explaining why? Doesn't seem obvious to me.
It's not required, will remove it
File src/ipa.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/f2b47446_acc579... : PS8, Line 360: case IPAC_PROTO_OSMO:
I could move it further upwards.
Done
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/d30c31ae_973fce... : PS8, Line 368: if (hh_ext->proto == IPAC_PROTO_EXT_TCAP_ROUTING) {
yes, I can use switch/case there.
Should be ok with the ifdef encompassing the whole case.
File src/m3ua.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/7238899a_81ed7d... : PS8, Line 50: #include "ss7_as_loadshare_tcap.h"
I prefer the style how it is currently. It reduces the usage of #ifdef in the common code.
Not needed here, removed
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/5a5b3d3f_9a5975... : PS8, Line 343: if (!xua)
Ack.
Done
File src/ss7_as.h:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/e34133a1_15cd0a... : PS8, Line 97: /* optimisation: true if tid_ranges contains PCs (not only wildcards) */
#ifdef WITH_TCAP_LOADSHARING
Done
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/feae522b_d41b71... : PS8, Line 145: /* Should we do load-sharing based on tcap ids? */
#ifdef WITH_TCAP_LOADSHARING
Done
File src/ss7_as.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/a6382c8a_30e898... : PS8, Line 139: hash_init(as->tcap.tid_ranges);
#ifdef WITH_TCAP_LOADSHARING
Done
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/757b4052_aea76d... : PS8, Line 545: case OSMO_SS7_AS_TMOD_LOADSHARE:
This is fine.
Done
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/779e6bed_269603... : PS8, Line 634:
Acknowledged
Done
File src/ss7_as_loadshare_tcap.h:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/ee27e2c7_e8d9d1... : PS8, Line 3: #include <complex.h>
I wonder why do we need this here...
Acknowledged
File src/ss7_as_loadshare_tcap.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/40b30e96_916a36... : 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?
Done
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/e4fc72e4_d93f9b... : 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 […]
Done
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/fcd4dd97_98e33a... : 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?
Acknowledged
File src/ss7_as_vty.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/6d99e087_6b46ce... : PS8, Line 165: "traffic-mode (loadshare|loadshare-tcap) [bindings] [sls] [opc-sls] [opc-shift] [<0-2>]",
traffic-mode loadshare-tcap is an extension to the general loadshare. […]
I'm not 100% familiar with the tcap_enable/disable hooks, but wouldn't you need to call tcap_disable in other traffic modes even with the separate command?
It's always possible to ``` traffic-mode loadshare
tcap-loadshare
[...later] traffic-mode roundrobin ``` and then you'd end up in a bad state (if calling tcap_disable is required).
File src/ss7_asp.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/87fc4c21_6f0474... : PS8, Line 1435: /*! Received an unknown packet on asp
Will look into it.
Done
File src/ss7_internal.h:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/90db1725_095055... : PS8, Line 9: #include <osmocom/sigtran/mtp_sap.h>
why is this now needed here? if needed, please submit another commit.
Done
File src/tcap_transaction_tracking.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/87ba06d1_706ae0... : PS8, Line 122: LOGPASP(entry->asp, DLSS7, LOGL_DEBUG, "Creating tcap cache, entry (own) pc/ssn/tid %u/%u/%u -> %u/%u/%u\n",
Ack
Done
File src/xua_as_fsm.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/41902dc8_f40ba4... : PS8, Line 204: asp = ss7_as_select_asp(as, xua);
This could actually go in a preparatory patch to simplify this one a bit.
Done
File stp/stp_main.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/1cba7d0b_e3f6b9... : PS8, Line 52: void *talloc_asn1_ctx;
Need to check it again, if it is still required.
It's not needed, removed
-
daniel