pespin has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-sigtran/+/41411?usp=email )
Change subject: mtp: Improve mtp_sap/ss7_user APIs ......................................................................
mtp: Improve mtp_sap/ss7_user APIs
* The ss7_user API was so far never used externally, and internally only by SCCP and a unit test. It has several bad practices like first passing a inst parent ptr to allocate it, and then require a inst pointer again to operate on it (eg. register/unregister, send a primitive, etc.). This only makes the API more complex than needed and creates potential problems where a isnt differnet than the one ss7_user was allocated on is passed. Instead, the ss7_user is pinned to its parent inst during alloction, and all MTP-SAP always happens uniquely through the ss7_user. This follows much more closely the already sanitized and much more used sccp_sap/sccp_user APIs.
* Move more primitive/SAP related code to mtp_sap.c, to make it easier to spot where the inter-layer communication happens. This way we also have the same disposition as with sccp_sap/sccp_user.
* Clean up some primitive related APIs to unify naming with other usual SAP/primitive osmocom code.
Change-Id: If320db2bcec7ccbbc2cdac0cbf018fd8be7bde22 --- M TODO-RELEASE M include/osmocom/sigtran/osmo_ss7.h M src/mtp_sap.c M src/sccp_instance.c M src/sccp_scrc.c M src/ss7_hmrt.c M src/ss7_instance.h M src/ss7_user.c M src/ss7_user.h M src/xua_internal.h M tests/ss7/ss7_test.c 11 files changed, 133 insertions(+), 115 deletions(-)
Approvals: osmith: Looks good to me, but someone else must approve pespin: Looks good to me, approved laforge: Looks good to me, but someone else must approve Jenkins Builder: Verified
diff --git a/TODO-RELEASE b/TODO-RELEASE index 2ecfd57..dd3b333 100644 --- a/TODO-RELEASE +++ b/TODO-RELEASE @@ -10,3 +10,4 @@ libosmo-netif >1.6.0 stream OSMO_STREAM_{CLI,SRV}_TCP_SOCKOPT_KEEP* libosmo-sigtran add enum mtp_network_indicator, mtp_network_indicator_vals libosmo-sigtran add osmo_ss7_instance_get_network_indicator() +libosmo-sigtran add osmo_ss7_user_mtp_sap_prim_down() diff --git a/include/osmocom/sigtran/osmo_ss7.h b/include/osmocom/sigtran/osmo_ss7.h index 5a51ba3..b560caf 100644 --- a/include/osmocom/sigtran/osmo_ss7.h +++ b/include/osmocom/sigtran/osmo_ss7.h @@ -92,15 +92,13 @@ void osmo_ss7_user_set_priv(struct osmo_ss7_user *user, void *priv); void *osmo_ss7_user_get_priv(const struct osmo_ss7_user *user);
-int osmo_ss7_user_register(struct osmo_ss7_instance *inst, uint8_t service_ind, - struct osmo_ss7_user *user); -int osmo_ss7_user_unregister(struct osmo_ss7_instance *inst, uint8_t service_ind, - struct osmo_ss7_user *user); +int osmo_ss7_user_register(struct osmo_ss7_user *user, uint8_t service_ind); +int osmo_ss7_user_unregister(struct osmo_ss7_user *user, uint8_t service_ind);
-/* SS7 User wants to issue MTP-TRANSFER.req */ -int osmo_ss7_user_mtp_xfer_req(struct osmo_ss7_instance *inst, - struct osmo_mtp_prim *omp);
+/* MTP User wants to submit a primitive down to MTP SAP */ +int osmo_ss7_user_mtp_sap_prim_down(struct osmo_ss7_user *user, + struct osmo_mtp_prim *omp);
/*********************************************************************** * SCCP Instance diff --git a/src/mtp_sap.c b/src/mtp_sap.c index b6b6eeb..cef0497 100644 --- a/src/mtp_sap.c +++ b/src/mtp_sap.c @@ -27,6 +27,10 @@ #include <osmocom/sigtran/osmo_ss7.h> #include <osmocom/sigtran/mtp_sap.h>
+#include "ss7_user.h" +#include "ss7_internal.h" +#include "xua_internal.h" + const struct value_string osmo_mtp_prim_type_names[] = { { OSMO_MTP_PRIM_TRANSFER, "MTP-TRANSFER" }, { OSMO_MTP_PRIM_PAUSE, "MTP-PAUSE" }, @@ -57,3 +61,46 @@ mtp_prim_hdr_name_buf(prim_name_buf, sizeof(prim_name_buf), oph); return prim_name_buf; } + +/*! \brief Send a MTP SAP Primitive up to the MTP User + * \param[in] osu MTP User to whom to send the primitive + * \param[in] prim Primitive to send to the user + * \returns return value of the MTP User's prim_cb() function + * + * Ownership of prim->oph->msg is passed to the user of the registered callback + */ +int ss7_user_mtp_sap_prim_up(const struct osmo_ss7_user *osu, struct osmo_mtp_prim *omp) +{ + LOGPSS7U(osu, LOGL_DEBUG, "Delivering to MTP User: %s\n", + osmo_mtp_prim_name(&omp->oph)); + return osu->prim_cb(&omp->oph, (void *) osu->priv); +} + +/* MTP-User requests to send a MTP-TRANSFER.req via the stack + * \param[in] osu MTP User sending us the primitive + * \param[in] oph Osmocom primitive sent by the user + * + * The oph->msg ownership is transferred to this function, which will free it. + */ +int osmo_ss7_user_mtp_sap_prim_down(struct osmo_ss7_user *osu, struct osmo_mtp_prim *omp) +{ + struct msgb *msg = omp->oph.msg; + int rc; + + OSMO_ASSERT(omp->oph.sap == MTP_SAP_USER); + + switch (OSMO_PRIM_HDR(&omp->oph)) { + case OSMO_PRIM(OSMO_MTP_PRIM_TRANSFER, PRIM_OP_REQUEST): + rc = hmrt_mtp_xfer_request_l4_to_l3(osu->inst, &omp->u.transfer, + msgb_l2(msg), msgb_l2len(msg)); + break; + default: + LOGPSS7U(osu, LOGL_ERROR, "Ignoring unknown primitive %u:%u\n", + omp->oph.primitive, omp->oph.operation); + rc = -1; + break; + } + + msgb_free(msg); + return rc; +} diff --git a/src/sccp_instance.c b/src/sccp_instance.c index f5b056f..0d67e4d 100644 --- a/src/sccp_instance.c +++ b/src/sccp_instance.c @@ -326,7 +326,7 @@ inst->ss7_user = osmo_ss7_user_create(ss7, "SCCP"); osmo_ss7_user_set_prim_cb(inst->ss7_user, mtp_user_prim_cb); osmo_ss7_user_set_priv(inst->ss7_user, inst); - osmo_ss7_user_register(ss7, MTP_SI_SCCP, inst->ss7_user); + osmo_ss7_user_register(inst->ss7_user, MTP_SI_SCCP);
llist_add_tail(&inst->list, &sccp_instances);
@@ -341,7 +341,7 @@ return;
inst->ss7->sccp = NULL; - osmo_ss7_user_unregister(inst->ss7, MTP_SI_SCCP, inst->ss7_user); + osmo_ss7_user_unregister(inst->ss7_user, MTP_SI_SCCP); osmo_ss7_user_destroy(inst->ss7_user); inst->ss7_user = NULL;
diff --git a/src/sccp_scrc.c b/src/sccp_scrc.c index ad474aa..9cf71a3 100644 --- a/src/sccp_scrc.c +++ b/src/sccp_scrc.c @@ -134,7 +134,7 @@ param->sio = MTP_SIO(MTP_SI_SCCP, s7i->cfg.network_indicator);
/* 3) send via MTP-SAP (osmo_ss7_instance) */ - return osmo_ss7_user_mtp_xfer_req(s7i, omp); + return osmo_ss7_user_mtp_sap_prim_down(inst->ss7_user, omp); }
/* Generate MTP-TRANSFER.req from xUA message */ diff --git a/src/ss7_hmrt.c b/src/ss7_hmrt.c index bd5d480..b915481 100644 --- a/src/ss7_hmrt.c +++ b/src/ss7_hmrt.c @@ -44,7 +44,7 @@ #include "ss7_user.h"
/* convert from M3UA message to MTP-TRANSFER.ind osmo_mtp_prim */ -struct osmo_mtp_prim *m3ua_to_xfer_ind(struct xua_msg *xua) +static struct osmo_mtp_prim *m3ua_to_xfer_ind(struct xua_msg *xua) { struct osmo_mtp_prim *prim; struct osmo_mtp_transfer_param *param; @@ -74,23 +74,10 @@ return prim; }
-/* convert from MTP-TRANSFER.req to osmo_mtp_prim */ -static struct xua_msg *mtp_prim_to_m3ua(struct osmo_mtp_prim *prim) -{ - struct msgb *msg = prim->oph.msg; - struct osmo_mtp_transfer_param *param = &prim->u.transfer; - struct m3ua_data_hdr data_hdr; - - mtp_xfer_param_to_m3ua_dh(&data_hdr, param); - - return m3ua_xfer_from_data(&data_hdr, msgb_l2(msg), msgb_l2len(msg)); -} - /* delivery given XUA message to given SS7 user * Ownership of xua_msg passed is transferred to this function. */ -static int deliver_to_mtp_user(const struct osmo_ss7_user *osu, - struct xua_msg *xua) +static int deliver_to_mtp_user(const struct osmo_ss7_user *osu, struct xua_msg *xua) { struct osmo_mtp_prim *prim; int rc; @@ -103,7 +90,7 @@ } prim->u.transfer = xua->mtp;
- rc = osu->prim_cb(&prim->oph, (void *) osu->priv); + rc = ss7_user_mtp_sap_prim_up(osu, prim);
ret_free: xua_msg_free(xua); @@ -145,17 +132,18 @@ return -1; }
- /* Check for local SSN registered for this DPC/SSN */ - osu = inst->user[service_ind]; - if (osu) { - return deliver_to_mtp_user(osu, xua); - } else { + /* "User Part Available?" */ + osu = ss7_user_find(inst, service_ind); + if (!osu) { + /* "Discard Message" */ LOGSS7(inst, LOGL_NOTICE, "No MTP-User for SI %u\n", service_ind); - /* Discard Message */ /* FIXME: User Part Unavailable HMDT -> HMRT */ xua_msg_free(xua); return -1; } + + /* "MTP Transfer indication HMDT→L4" */ + return deliver_to_mtp_user(osu, xua); }
/* HMDC->HMRT Msg For Routing; Figure 26/Q.704 */ @@ -251,35 +239,22 @@ } }
-/* MTP-User requests to send a MTP-TRANSFER.req via the stack */ -int osmo_ss7_user_mtp_xfer_req(struct osmo_ss7_instance *inst, - struct osmo_mtp_prim *omp) +/* Figure 26/Q.704 (sheet 1 of 5) "MTP Transfer request L4→L3" */ +int hmrt_mtp_xfer_request_l4_to_l3(struct osmo_ss7_instance *inst, const struct osmo_mtp_transfer_param *param, uint8_t *user_data, size_t user_data_len) { + struct m3ua_data_hdr data_hdr; struct xua_msg *xua; - int rc;
- OSMO_ASSERT(omp->oph.sap == MTP_SAP_USER); + /* convert from osmo_mtp_prim MTP-TRANSFER.req to xua_msg */ + mtp_xfer_param_to_m3ua_dh(&data_hdr, param); + xua = m3ua_xfer_from_data(&data_hdr, user_data, user_data_len); + OSMO_ASSERT(xua); + xua->mtp = *param;
- switch (OSMO_PRIM_HDR(&omp->oph)) { - case OSMO_PRIM(OSMO_MTP_PRIM_TRANSFER, PRIM_OP_REQUEST): - xua = mtp_prim_to_m3ua(omp); - xua->mtp = omp->u.transfer; - /* normally we would call hmrt_message_for_routing() - * here, if we were to follow the state diagrams of the - * ITU-T Q.70x specifications. However, what if a local - * MTP user sends a MTP-TRANSFER.req to a local SSN? - * This wouldn't work as per the spec, but I believe it - * is a very useful feature (aka "loopback device" in - * IPv4). So we call m3ua_hmdc_rx_from_l2() just like - * the MTP-TRANSFER had been received from L2. */ - rc = m3ua_hmdc_rx_from_l2(inst, xua); - break; - default: - LOGSS7(inst, LOGL_ERROR, "Ignoring unknown primitive %u:%u\n", - omp->oph.primitive, omp->oph.operation); - rc = -1; - } - - msgb_free(omp->oph.msg); - return rc; + /* normally we would call hmrt_message_for_routing() here, if we were to follow the state + * diagrams of the ITU-T Q.70x specifications. However, what if a local MTP user sends a + * MTP-TRANSFER.req to a local SSN? This wouldn't work as per the spec, but I believe it + * is a very useful feature (aka "loopback device" in IPv4). So we call + * m3ua_hmdc_rx_from_l2() just like the MTP-TRANSFER had been received from L2. */ + return m3ua_hmdc_rx_from_l2(inst, xua); } diff --git a/src/ss7_instance.h b/src/ss7_instance.h index d4d0b3d..3045403 100644 --- a/src/ss7_instance.h +++ b/src/ss7_instance.h @@ -43,7 +43,7 @@ struct llist_head xua_servers; /* array for faster lookup of user (indexed by service * indicator) */ - const struct osmo_ss7_user *user[16]; + struct osmo_ss7_user *user[16];
struct osmo_ss7_route_table *rtable_system;
diff --git a/src/ss7_user.c b/src/ss7_user.c index e7d0483..0ec42ff 100644 --- a/src/ss7_user.c +++ b/src/ss7_user.c @@ -26,9 +26,11 @@ #include <osmocom/core/talloc.h> #include <osmocom/core/prim.h> #include <osmocom/sigtran/osmo_ss7.h> +#include <osmocom/sigtran/mtp_sap.h>
#include "ss7_user.h" #include "ss7_internal.h" +#include "xua_internal.h"
/*********************************************************************** * MTP Users (Users of MTP, such as SCCP or ISUP) @@ -48,9 +50,17 @@
void osmo_ss7_user_destroy(struct osmo_ss7_user *user) { + ss7_user_unregister_all(user); talloc_free(user); }
+struct osmo_ss7_user *ss7_user_find(struct osmo_ss7_instance *inst, uint8_t service_indicator) +{ + if (service_indicator >= ARRAY_SIZE(inst->user)) + return NULL; + return inst->user[service_indicator]; +} + struct osmo_ss7_instance *osmo_ss7_user_get_instance(const struct osmo_ss7_user *user) { return user->inst; @@ -72,75 +82,57 @@ }
/*! \brief Register a MTP user for a given service indicator - * \param[in] inst SS7 instance for which we register the user + * \param[in] user SS7 user to register (including primitive call-back) * \param[in] service_ind Service (ISUP, SCCP, ...) - * \param[in] user SS7 user (including primitive call-back) * \returns 0 on success; negative on error */ -int osmo_ss7_user_register(struct osmo_ss7_instance *inst, uint8_t service_ind, - struct osmo_ss7_user *user) +int osmo_ss7_user_register(struct osmo_ss7_user *user, uint8_t service_ind) { + struct osmo_ss7_instance *inst = user->inst; + if (service_ind >= ARRAY_SIZE(inst->user)) return -EINVAL;
if (inst->user[service_ind]) return -EBUSY;
- DEBUGP(DLSS7, "registering user=%s for SI %u with priv %p\n", - user->name, service_ind, user->priv); + LOGPSS7U(user, LOGL_DEBUG, "registering for SI %u with priv %p\n", + service_ind, user->priv);
- user->inst = inst; inst->user[service_ind] = user;
return 0; }
/*! \brief Unregister a MTP user for a given service indicator - * \param[in] inst SS7 instance for which we register the user + * \param[in] user SS7 user to unregister. * \param[in] service_ind Service (ISUP, SCCP, ...) - * \param[in] user (optional) SS7 user. If present, we will not - * unregister other users * \returns 0 on success; negative on error */ -int osmo_ss7_user_unregister(struct osmo_ss7_instance *inst, uint8_t service_ind, - struct osmo_ss7_user *user) +int osmo_ss7_user_unregister(struct osmo_ss7_user *user, uint8_t service_ind) { + struct osmo_ss7_instance *inst = user->inst; + if (service_ind >= ARRAY_SIZE(inst->user)) return -EINVAL;
if (!inst->user[service_ind]) return -ENODEV;
- if (user && (inst->user[service_ind] != user)) + if (inst->user[service_ind] != user) return -EINVAL;
- if (user) - user->inst = NULL; + LOGPSS7U(user, LOGL_DEBUG, "unregistering from SI %u with priv %p\n", + service_ind, user->priv); + inst->user[service_ind] = NULL;
return 0; }
-/* deliver to a local MTP user */ -int ss7_mtp_to_user(struct osmo_ss7_instance *inst, struct osmo_mtp_prim *omp) +void ss7_user_unregister_all(struct osmo_ss7_user *user) { - uint32_t service_ind; - const struct osmo_ss7_user *osu; - - if (omp->oph.sap != MTP_SAP_USER || - omp->oph.primitive != OSMO_MTP_PRIM_TRANSFER || - omp->oph.operation != PRIM_OP_INDICATION) { - LOGP(DLSS7, LOGL_ERROR, "Unsupported Primitive\n"); - return -EINVAL; + struct osmo_ss7_instance *inst = user->inst; + for (unsigned int i = 0; i < ARRAY_SIZE(inst->user); i++) { + if (inst->user[i] == user) + inst->user[i] = NULL; } - - service_ind = omp->u.transfer.sio & 0xF; - osu = inst->user[service_ind]; - - if (!osu) { - LOGP(DLSS7, LOGL_NOTICE, "No MTP-User for SI %u\n", service_ind); - return -ENODEV; - } - - DEBUGP(DLSS7, "delivering MTP-TRANSFER.ind to user %s, priv=%p\n", - osu->name, osu->priv); - return osu->prim_cb(&omp->oph, (void *) osu->priv); } diff --git a/src/ss7_user.h b/src/ss7_user.h index 6021330..280fc82 100644 --- a/src/ss7_user.h +++ b/src/ss7_user.h @@ -5,7 +5,7 @@ #include <osmocom/sigtran/mtp_sap.h>
/*********************************************************************** - * SS7 Linksets + * SS7 User (MTP SAP) ***********************************************************************/
struct osmo_ss7_instance; @@ -21,4 +21,11 @@ void *priv; };
-int ss7_mtp_to_user(struct osmo_ss7_instance *inst, struct osmo_mtp_prim *omp); +struct osmo_ss7_user *ss7_user_find(struct osmo_ss7_instance *inst, uint8_t service_indicator); +void ss7_user_unregister_all(struct osmo_ss7_user *user); +int ss7_user_mtp_sap_prim_up(const struct osmo_ss7_user *osu, struct osmo_mtp_prim *omp); + +#define _LOGPSS7U(osu, subsys, level, fmt, args ...) \ + _LOGSS7((osu)->inst, subsys, level, "ss7_user(%s) " fmt, osu->name, ## args) +#define LOGPSS7U(osu, level, fmt, args ...) \ + _LOGPSS7U(osu, DLSS7, level, fmt, ## args) diff --git a/src/xua_internal.h b/src/xua_internal.h index 36393f3..90a68be 100644 --- a/src/xua_internal.h +++ b/src/xua_internal.h @@ -1,5 +1,8 @@ #pragma once
+#include <unistd.h> +#include <stdint.h> + #include <osmocom/core/tdef.h> #include <osmocom/sigtran/osmo_ss7.h> #include "xua_msg.h" @@ -34,7 +37,6 @@ void sua_tx_dupu(struct osmo_ss7_asp *asp, const uint32_t *rctx, unsigned int num_rctx, uint32_t dpc, uint16_t user, uint16_t cause, const char *info_str);
-struct osmo_mtp_prim *m3ua_to_xfer_ind(struct xua_msg *xua); int m3ua_hmdc_rx_from_l2(struct osmo_ss7_instance *inst, struct xua_msg *xua); struct msgb *m3ua_to_msg(struct xua_msg *xua); int m3ua_tx_xua_as(struct osmo_ss7_as *as, struct xua_msg *xua); @@ -69,6 +71,9 @@ const struct m3ua_data_hdr *mdh); void mtp_xfer_param_to_m3ua_dh(struct m3ua_data_hdr *mdh, const struct osmo_mtp_transfer_param *param); +int hmrt_mtp_xfer_request_l4_to_l3(struct osmo_ss7_instance *inst, + const struct osmo_mtp_transfer_param *param, + uint8_t *user_data, size_t user_data_len);
extern const struct xua_msg_class m3ua_msg_class_mgmt; diff --git a/tests/ss7/ss7_test.c b/tests/ss7/ss7_test.c index c8e1c67..739d786 100644 --- a/tests/ss7/ss7_test.c +++ b/tests/ss7/ss7_test.c @@ -124,25 +124,18 @@ osmo_ss7_user_set_priv(user, (void *) 0x1234);
/* registration */ - OSMO_ASSERT(osmo_ss7_user_register(s7i, 1, user) == 0); - OSMO_ASSERT(osmo_ss7_user_register(s7i, 1, NULL) == -EBUSY); - OSMO_ASSERT(osmo_ss7_user_register(s7i, 255, NULL) == -EINVAL); + OSMO_ASSERT(osmo_ss7_user_register(user, 1) == 0); + OSMO_ASSERT(osmo_ss7_user_register(user, 1) == -EBUSY); + OSMO_ASSERT(osmo_ss7_user_register(user, 255) == -EINVAL);
/* primitive delivery */ - OSMO_ASSERT(ss7_mtp_to_user(s7i, &omp) == 23); + OSMO_ASSERT(ss7_user_mtp_sap_prim_up(user, &omp) == 23);
/* cleanup */ - OSMO_ASSERT(osmo_ss7_user_unregister(s7i, 255, NULL) == -EINVAL); - OSMO_ASSERT(osmo_ss7_user_unregister(s7i, 10, NULL) == -ENODEV); - OSMO_ASSERT(osmo_ss7_user_unregister(s7i, 1, user2) == -EINVAL); - OSMO_ASSERT(osmo_ss7_user_unregister(s7i, 1, user) == 0); - - /* primitive delivery should fail now */ - OSMO_ASSERT(ss7_mtp_to_user(s7i, &omp) == -ENODEV); - - /* wrong primitive delivery should also fail */ - omp.oph.primitive = OSMO_MTP_PRIM_PAUSE; - OSMO_ASSERT(ss7_mtp_to_user(s7i, &omp) == -EINVAL); + OSMO_ASSERT(osmo_ss7_user_unregister(user, 255) == -EINVAL); + OSMO_ASSERT(osmo_ss7_user_unregister(user, 10) == -ENODEV); + OSMO_ASSERT(osmo_ss7_user_unregister(user2, 1) == -EINVAL); + OSMO_ASSERT(osmo_ss7_user_unregister(user, 1) == 0);
osmo_ss7_user_destroy(user); osmo_ss7_user_destroy(user2);
-
pespin