pespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/libosmo-sigtran/+/38375?usp=email ) Change subject: ss7 vty: Fix insert order of routes with higher priority ...................................................................... ss7 vty: Fix insert order of routes with higher priority Route lookup ((hmrt_message_for_routing() => osmo_ss7_route_lookup() => osmo_ss7_route_find_dpc_mask())) iterates the list of routes assuming they are ordered by mask length (so it doesn't need to iterate the full list every time). Similary, it implicitly seems to assume that the entries are also sorted by priority. However, priority is not currently taken into account when being inserted in the routing table, so the route ends up in the incorrect place. The VTY "update route" command used to first insert the route and only *after* that tweak some of its fields, like priority or qos-class. This commit adds a new set of osmo_ss7_route API which allows users to *first* allocate a route and configure it completelly, and *second* insert it once it contains all the information. This way, the insert algorithm can take into account the configured route priority and place the route entry in the routing table at the proper place. Related: SYS#7112 Change-Id: I6e7b5e3f06ae2c9468da58c55d2f42cbcd777218 --- M TODO-RELEASE M include/osmocom/sigtran/osmo_ss7.h M src/osmo_ss7.c M src/osmo_ss7_vty.c M tests/vty/osmo_stp_route_prio.vty 5 files changed, 199 insertions(+), 55 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmo-sigtran refs/changes/75/38375/1 diff --git a/TODO-RELEASE b/TODO-RELEASE index 935cda8..410bb81 100644 --- a/TODO-RELEASE +++ b/TODO-RELEASE @@ -8,3 +8,4 @@ # If any interfaces have been removed or changed since the last public release: c:r:0. #library what description / commit summary line libosmo-sigtran API change struct osmo_ss7_instance has new member 'secondary_pc' +libosmo-sigtran new API osmo_ss7_route_alloc,osmo_ss7_route_set_linkset,osmo_ss7_route_insert diff --git a/include/osmocom/sigtran/osmo_ss7.h b/include/osmocom/sigtran/osmo_ss7.h index 049c7d8..d525dd9 100644 --- a/include/osmocom/sigtran/osmo_ss7.h +++ b/include/osmocom/sigtran/osmo_ss7.h @@ -230,6 +230,8 @@ }; struct osmo_ss7_route * +osmo_ss7_route_alloc(struct osmo_ss7_route_table *rtbl, uint32_t pc, uint32_t mask); +struct osmo_ss7_route * osmo_ss7_route_find_dpc(struct osmo_ss7_route_table *rtbl, uint32_t dpc); struct osmo_ss7_route * osmo_ss7_route_find_dpc_mask(struct osmo_ss7_route_table *rtbl, uint32_t dpc, @@ -238,7 +240,9 @@ osmo_ss7_route_lookup(struct osmo_ss7_instance *inst, uint32_t dpc); struct osmo_ss7_route * osmo_ss7_route_create(struct osmo_ss7_route_table *rtbl, uint32_t dpc, - uint32_t mask, const char *linkset_name); + uint32_t mask, const char *linkset_name); +int osmo_ss7_route_set_linkset(struct osmo_ss7_route *rt, const char *linkset_name); +int osmo_ss7_route_insert(struct osmo_ss7_route *rt); void osmo_ss7_route_destroy(struct osmo_ss7_route *rt); const char *osmo_ss7_route_print(const struct osmo_ss7_route *rt); const char *osmo_ss7_route_name(struct osmo_ss7_route *rt, bool list_asps); diff --git a/src/osmo_ss7.c b/src/osmo_ss7.c index 55d3d02..cd479a6 100644 --- a/src/osmo_ss7.c +++ b/src/osmo_ss7.c @@ -657,6 +657,100 @@ * SS7 Routes ***********************************************************************/ +/*! \brief Allocate a route entry + * \param[in] rtbl Routing Table where the route belongs + * \param[in] pc Point Code of the destination of the route + * \param[in] mask Mask of the destination Point Code \ref pc + * \returns Allocated route (noy yet insertd into its rtbl), NULL on error + * + * The returned route has no linkset associated yet, user *must* associate it + * using API osmo_ss7_route_set_linkset() before inserting the route into its + * routing table. + * + * Fields priority and qos_class may be set *before* inserting the route into + * its routing table: + * - A default priority of 0 is configured on the route. + * - A default qos-class of 0 is configured on the route. + * + * Use API osmo_ss7_route_insert() to insert the route into its routing table. + * + * The route entry allocated with this API can be destroyed/freed at any point using API + * osmo_ss7_route_destroy(), regardless of it being already insertd or not in + * its routing table. + */ +struct osmo_ss7_route * +osmo_ss7_route_alloc(struct osmo_ss7_route_table *rtbl, uint32_t pc, uint32_t mask) +{ + struct osmo_ss7_route *rt; + + OSMO_ASSERT(ss7_initialized); + + rt = talloc_zero(rtbl, struct osmo_ss7_route); + if (!rt) + return NULL; + + /* Mark it as not being inserted yet in rtbl */ + INIT_LLIST_HEAD(&rt->list); + rt->rtable = rtbl; + /* truncate mask to maximum. Let's avoid callers specifying arbitrary large + * masks to ensure we don't fail duplicate detection with longer mask lengths */ + rt->cfg.mask = osmo_ss7_pc_normalize(&rtbl->inst->cfg.pc_fmt, mask); + rt->cfg.pc = osmo_ss7_pc_normalize(&rtbl->inst->cfg.pc_fmt, pc); + return rt; +} + +/*! \brief Check whether route has already been inserted into its routing table. + * \returns true if already inserted, false if not. + */ +static bool ss7_route_inserted(const struct osmo_ss7_route *rt) +{ + return !llist_empty(&rt->list); +} + +/*! \brief Set linkset on route entry + * \param[in] rt Route to be configured + * \param[in] linkset_name string name of the linkset to be used + * \returns 0 on success, negative on error. + */ +int +osmo_ss7_route_set_linkset(struct osmo_ss7_route *rt, const char *linkset_name) +{ + struct osmo_ss7_linkset *lset; + struct osmo_ss7_as *as = NULL; + struct osmo_ss7_route_table *rtbl = rt->rtable; + + if (rt->cfg.linkset_name) { + LOGSS7(rtbl->inst, LOGL_ERROR, "Attempt setting linkset on route already configured!\n"); + return -EBUSY; + } + + if (ss7_route_inserted(rt)) { + LOGSS7(rtbl->inst, LOGL_ERROR, "Attempt setting linkset on route already in the routing table!\n"); + return -EALREADY; + } + + lset = osmo_ss7_linkset_find_by_name(rtbl->inst, linkset_name); + if (!lset) { + as = osmo_ss7_as_find_by_name(rtbl->inst, linkset_name); + if (!as) + return -ENODEV; + } + + rt->cfg.linkset_name = talloc_strdup(rt, linkset_name); + if (lset) { + rt->dest.linkset = lset; + LOGSS7(rtbl->inst, LOGL_INFO, "Creating route: pc=%u=%s mask=0x%x via linkset '%s'\n", + rt->cfg.pc, osmo_ss7_pointcode_print(rtbl->inst, rt->cfg.pc), + rt->cfg.mask, lset->cfg.name); + } else { + rt->dest.as = as; + LOGSS7(rtbl->inst, LOGL_INFO, "Creating route: pc=%u=%s mask=0x%x via AS '%s'\n", + rt->cfg.pc, osmo_ss7_pointcode_print(rtbl->inst, rt->cfg.pc), + rt->cfg.mask, as->cfg.name); + } + return 0; +} + /*! \brief Find a SS7 route for given destination point code in given table */ struct osmo_ss7_route * osmo_ss7_route_find_dpc(struct osmo_ss7_route_table *rtbl, uint32_t dpc) @@ -709,6 +803,7 @@ /* insert the route in the ordered list of routes. The list is sorted by * mask length, so that the more specific (longer mask) routes are * first, while the less specific routes with shorter masks are last. + * Within the same mask length, the routes are ordered by priority. * Hence, the first matching route in a linear iteration is the most * specific match. */ static void route_insert_sorted(struct osmo_ss7_route_table *rtbl, @@ -717,73 +812,93 @@ struct osmo_ss7_route *rt; llist_for_each_entry(rt, &rtbl->routes, list) { + if (rt->cfg.mask == cmp->cfg.mask && + rt->cfg.priority > cmp->cfg.priority) { + /* insert before the current entry */ + llist_add(&cmp->list, rt->list.prev); + return; + } if (rt->cfg.mask < cmp->cfg.mask) { /* insert before the current entry */ llist_add(&cmp->list, rt->list.prev); return; } } - /* not added, i.e. no smaller mask length found: we are the - * smallest mask and thus should go last */ + /* not added, i.e. no smaller mask length and priority found: we are the + * smallest mask and priority and thus should go last */ llist_add_tail(&cmp->list, &rtbl->routes); } +/*! \brief Insert route into its routing table + * \param[in] rt Route to be inserted into its routing table + * \returns 0 on success, negative on error + * + * A route is only really used once it has been inserted into its routing table. + */ +int +osmo_ss7_route_insert(struct osmo_ss7_route *rt) +{ + struct osmo_ss7_route *prev_rt; + struct osmo_ss7_route_table *rtbl = rt->rtable; + + if (ss7_route_inserted(rt)) { + LOGSS7(rtbl->inst, LOGL_ERROR, "Attempt insert of route already in the routing table!\n"); + return -EALREADY; + } + + if (!rt->cfg.linkset_name) { + return -EINVAL; + } + + /* check for duplicates */ + prev_rt = osmo_ss7_route_find_dpc_mask(rtbl, rt->cfg.pc, rt->cfg.mask); + if (prev_rt && !strcmp(prev_rt->cfg.linkset_name, rt->cfg.linkset_name)) { + LOGSS7(rtbl->inst, LOGL_ERROR, "Refusing to create duplicate route: " + "pc=%u=%s mask=0x%x via linkset/AS '%s'\n", + rt->cfg.pc, osmo_ss7_pointcode_print(rtbl->inst, rt->cfg.pc), + rt->cfg.mask, rt->cfg.linkset_name); + return -EADDRINUSE; + } + + route_insert_sorted(rtbl, rt); + return 0; +} + /*! \brief Create a new route in the given routing table * \param[in] rtbl Routing Table in which the route is to be created * \param[in] pc Point Code of the destination of the route * \param[in] mask Mask of the destination Point Code \ref pc * \param[in] linkset_name string name of the linkset to be used - * \returns caller-allocated + initialized route, NULL on error + * \returns callee-allocated + initialized route, NULL on error + * + * The route allocated and returned by this API is already inserted into the + * routing table, with priority and qos-class set to 0. + * If you plan to use different values for priority and qos-class, avoid using + * this API and use osmo_ss7_route_alloc() + osmo_ss7_route_set_linkset() + + * osmo_ss7_route_insert() instead. */ struct osmo_ss7_route * osmo_ss7_route_create(struct osmo_ss7_route_table *rtbl, uint32_t pc, uint32_t mask, const char *linkset_name) { struct osmo_ss7_route *rt; - struct osmo_ss7_linkset *lset; - struct osmo_ss7_as *as = NULL; + int rc; - /* truncate mask to maximum. Let's avoid callers specifying arbitrary large - * masks to ensure we don't fail duplicate detection with longer mask lengths */ - mask = osmo_ss7_pc_normalize(&rtbl->inst->cfg.pc_fmt, mask); - pc = osmo_ss7_pc_normalize(&rtbl->inst->cfg.pc_fmt, pc); - - OSMO_ASSERT(ss7_initialized); - lset = osmo_ss7_linkset_find_by_name(rtbl->inst, linkset_name); - if (!lset) { - as = osmo_ss7_as_find_by_name(rtbl->inst, linkset_name); - if (!as) - return NULL; - } - - /* check for duplicates */ - rt = osmo_ss7_route_find_dpc_mask(rtbl, pc, mask); - if (rt && !strcmp(rt->cfg.linkset_name, linkset_name)) { - LOGSS7(rtbl->inst, LOGL_ERROR, "Refusing to create duplicate route: " - "pc=%u=%s mask=0x%x via linkset/AS '%s'\n", - pc, osmo_ss7_pointcode_print(rtbl->inst, pc), mask, linkset_name); - return rt; - } - - rt = talloc_zero(rtbl, struct osmo_ss7_route); + rt = osmo_ss7_route_alloc(rtbl, pc, mask); if (!rt) return NULL; - rt->cfg.pc = pc; - rt->cfg.mask = mask; - rt->cfg.linkset_name = talloc_strdup(rt, linkset_name); - if (lset) { - rt->dest.linkset = lset; - LOGSS7(rtbl->inst, LOGL_INFO, "Creating route: pc=%u=%s mask=0x%x via linkset '%s'\n", - pc, osmo_ss7_pointcode_print(rtbl->inst, pc), mask, lset->cfg.name); - } else { - rt->dest.as = as; - LOGSS7(rtbl->inst, LOGL_INFO, "Creating route: pc=%u=%s mask=0x%x via AS '%s'\n", - pc, osmo_ss7_pointcode_print(rtbl->inst, pc), mask, as->cfg.name); + if (osmo_ss7_route_set_linkset(rt, linkset_name) < 0) { + talloc_free(rt); + return NULL; } - rt->rtable = rtbl; - route_insert_sorted(rtbl, rt); + rc = osmo_ss7_route_insert(rt); + /* Keep old behavior, return already existing route: */ + if (rc == -EADDRINUSE) { + talloc_free(rt); + return osmo_ss7_route_find_dpc_mask(rtbl, rt->cfg.pc, rt->cfg.mask); + } return rt; } @@ -795,10 +910,16 @@ OSMO_ASSERT(ss7_initialized); - LOGSS7(rtbl->inst, LOGL_INFO, "Destroying route: pc=%u=%s mask=0x%x via linkset/ASP '%s'\n", - rt->cfg.pc, osmo_ss7_pointcode_print(rtbl->inst, rt->cfg.pc), rt->cfg.mask, rt->cfg.linkset_name); + if (!rt) + return; - llist_del(&rt->list); + if (ss7_route_inserted(rt)) { + LOGSS7(rtbl->inst, LOGL_INFO, + "Destroying route: pc=%u=%s mask=0x%x via linkset/ASP '%s'\n", + rt->cfg.pc, osmo_ss7_pointcode_print(rtbl->inst, rt->cfg.pc), + rt->cfg.mask, rt->cfg.linkset_name); + llist_del(&rt->list); + } talloc_free(rt); } diff --git a/src/osmo_ss7_vty.c b/src/osmo_ss7_vty.c index 0dd8a58..01eafff 100644 --- a/src/osmo_ss7_vty.c +++ b/src/osmo_ss7_vty.c @@ -361,6 +361,7 @@ int mask = osmo_ss7_pointcode_parse_mask_or_len(rtable->inst, argv[1]); const char *ls_name = argv[2]; unsigned int argind; + int rc; if (dpc < 0) { vty_out(vty, "%% Invalid point code (%s)%s", argv[0], VTY_NEWLINE); @@ -372,16 +373,9 @@ return CMD_WARNING; } - rt = osmo_ss7_route_create(rtable, dpc, mask, ls_name); - if (!rt) { - vty_out(vty, "%% Cannot create route %s/%s to %s%s", - argv[0], argv[1], argv[2], VTY_NEWLINE); - return CMD_WARNING; - } - switch (argc) { case 3: - return CMD_SUCCESS; + break; /* Continue below */ case 5: if (strcmp(argv[3], "priority") != 0 && strcmp(argv[3], "qos-class") != 0) @@ -397,6 +391,17 @@ return CMD_WARNING; } + rt = osmo_ss7_route_alloc(rtable, dpc, mask); + if (!rt) { + vty_out(vty, "%% Cannot allocate new route%s", VTY_NEWLINE); + return CMD_WARNING; + } + + if ((rc = osmo_ss7_route_set_linkset(rt, ls_name)) < 0) { + vty_out(vty, "%% Cannot find linkset %s%s", ls_name, VTY_NEWLINE); + goto destroy_warning; + } + argind = 3; if (argc > argind && !strcmp(argv[argind], "priority")) { argind++; @@ -408,7 +413,20 @@ rt->cfg.qos_class = atoi(argv[argind++]); } + if ((rc = osmo_ss7_route_insert(rt)) < 0) { + char buf_err[128]; + strerror_r(-rc, buf_err, sizeof(buf_err)); + vty_out(vty, "%% Cannot insert route %s/%s to %s: %s (%d)%s", + argv[0], argv[1], argv[2], + buf_err, rc, VTY_NEWLINE); + goto destroy_warning; + } + return CMD_SUCCESS; + +destroy_warning: + osmo_ss7_route_destroy(rt); + return CMD_WARNING; } DEFUN_ATTR(cs7_rt_rem, cs7_rt_rem_cmd, diff --git a/tests/vty/osmo_stp_route_prio.vty b/tests/vty/osmo_stp_route_prio.vty index 178f9be..90f9b2e 100644 --- a/tests/vty/osmo_stp_route_prio.vty +++ b/tests/vty/osmo_stp_route_prio.vty @@ -74,9 +74,9 @@ cs7 instance 0 ... route-table system - update route 3.2.1 7.255.7 linkset as1 priority 5 update route 3.2.1 7.255.7 linkset as3 update route 3.2.1 7.255.7 linkset as2 priority 2 + update route 3.2.1 7.255.7 linkset as1 priority 5 ... OsmoSTP(config-cs7-rt)# do show cs7 instance 0 route Routing table = system @@ -84,6 +84,6 @@ Destination C Q P Linkset Name Linkset Non-adj Route ---------------------- - - - ------------------- ------- ------- ------- -3.2.1/14 5 as1 ? ? ? 3.2.1/14 0 as3 ? ? ? 3.2.1/14 2 as2 ? ? ? +3.2.1/14 5 as1 ? ? ? -- To view, visit https://gerrit.osmocom.org/c/libosmo-sigtran/+/38375?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: libosmo-sigtran Gerrit-Branch: master Gerrit-Change-Id: I6e7b5e3f06ae2c9468da58c55d2f42cbcd777218 Gerrit-Change-Number: 38375 Gerrit-PatchSet: 1 Gerrit-Owner: pespin &lt;pespin(a)sysmocom.de&gt;