Attention is currently required from: daniel, laforge, lynxis lazus.
pespin 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 9: Code-Review-1
(11 comments)
Patchset:
PS8:
@pespin: I like to keep most of the #ifdef / #endif out of the code. […]
I don't want to be looking at functions which turn to be noops 99% of the setups while reading at the code. And the amount of places where the tcap functionaltities need to be hooked is so small I don't see a problem with having some extra ifdefs in the few places where the calls are done.
that also helps by a simple grep ENABLE_TCAP figuring out all the places where tcap related features are hooked.
File examples/sccp_demo_user.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/8ec02608_e29c87... : PS8, Line 30: void *talloc_asn1_ctx;
Will check it again, if it is still required. The asn1 library changed.
If it is required, do you mind explaining why? Doesn't seem obvious to me.
File src/ipa.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/f08d589b_c59640... : PS8, Line 316: ss7_asp_tcap_rx_sccp(as, asp, opc, dpc, msg);
I want to keep the code changes to the other files minimal. […]
To start with, I'd prefix it with tcap_* tbh. I can also have a look whether this is the best place to lookup the content.
File src/ss7_as_loadshare_tcap.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/02c34788_99b16e... : PS8, Line 138: ssn ^= ((pc >> 24) & 0xff);
Wikipedia says it's 14 bits for ITU. […]
3+8+3=14 indeed, I counted wrong from the top of my head :D
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/349aace2_4a47ab... : PS8, Line 273: /** Traffic from the TCAP ASP -> AS -> osmo-stp, only used to update transaction tracking
Yes it is. If TCAP is enabled on the ASP, it must track both, Rx and Tx.
Done
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/f5e0cd0c_b7bd1f... : PS8, Line 380: static int sent_back_utds(struct osmo_ss7_as *as,
It sends back an SCCP/UTDS back to the origin
I need to investigate about this: * The meanding of sending that UTDS thing I don't know about * Whether ROUTE-FAIL.ind would be enough
* Whether the sending back up the stack would require some sort of async queue like ROUTE-FAIL.ind to avoid sending a primtiive to the user while it's busy sending the previous request primitive.
Also, some spec reference here would be nice.
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/a1dbbab8_4ca39f... : PS8, Line 487: /* decode SCCP and convert to a SUA/xUA representation */
I don't really see the gain from move the code back into an own function with a lot of pointers. […]
Done
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/0b32957c_ff879f... : PS8, Line 560: sent_back_utds(as, xua, sua, SCCP_RETURN_CAUSE_SUBSYSTEM_FAILURE);
An UTDS was requested. I don't know enough of SCCP. […]
I need to investigate about this: * The meanding of sending that UTDS thing I don't know about * Whether ROUTE-FAIL.ind would be enough
* Whether the sending back up the stack would require some sort of async queue like ROUTE-FAIL.ind to avoid sending a primtiive to the user while it's busy sending the previous request primitive.
Also, some spec reference here would be nice.
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/bff75e23_5ec772... : PS8, Line 607: * @return 0 on success
There is a difference between (rc = EPROTONOSUPPORT) and (rc = 0 && asp == NULL.) […]
afaiu this should only return the asp, no need to return an rc?
File src/ss7_asp.h:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/9b30ad68_34e55c... : PS8, Line 96: bool enabled;
Not all ASP of an AS has it enabled. […]
Does that make sense? why would somebody want to have TCAP routing enabled on some and not all of the ASPs serving an AS?
File src/ss7_asp.c:
https://gerrit.osmocom.org/c/libosmo-sigtran/+/41309/comment/04268b90_8a906d... : PS8, Line 1344: tcap_asp_down(asp);
Where do you think it belongs exactly? I'm not used to this code base.
I can have a look.