This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
neels gerrit-no-reply at lists.osmocom.orgneels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/18898 )
Change subject: endp: add E1 endpoint interlocking
......................................................................
Patch Set 4:
(24 comments)
oof, sorry for posting so many comments again. It's mainly details, but a lot of them.
Most important to me is the code dup / static-ness of magic-number tables and the logic around blocked (not "available") endpoints.
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c
File src/libosmo-mgcp/mgcp_endp.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@81
PS4, Line 81: { 0, 0, 4, 0, 2, 4, 6, 0, 1, 2, 3, 4, 5, 6, 7 };
(are these the same numbers as below const arrays? if yes please don't dup but use a common global const array)
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@373
PS4, Line 373: (e.g. ds/e1-0/s-30/su16-4), returns 0xff on error. */
(comment convention: start all lines with '*', same below)
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@374
PS4, Line 374: static uint8_t e1_ts_nr_from_epname(char *epname)
const char *epname, same below
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@410
PS4, Line 410: static const uint8_t rates[] = { 64, 32, 16, 16, 8 };
(global? like commented below)
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@468
PS4, Line 468: }
The above e1_xxx_from_epname() each tokenize and parse the same epname, one after the other.
Wouldn't it make more sense to have a single function that extracts all of these items at the same time?
(could also include below _ss_nr_)
If it were me I'd make single point of conversion between a struct and a string:
struct e1_ep {
uint8_t rate;
uint8_t offs;
uint8_t ts;
uint8_t ss_nr;
}
char *e1_ep_to_str_c(void *ctx, const struct e1_rp *e1_ep);
int e1_ep_from_str(struct e1_ep *e1_ep, const char *epname);
Whether the remaining code stores the string or the e1_ep struct is then arbitrary.
Just an idea, I think it would make for cleaner code overall.
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@485
PS4, Line 485: { 0, 0, 4, 0, 2, 4, 6, 0, 1, 2, 3, 4, 5, 6, 7 };
maybe these two const arrays should be defined at the top of the .c file?
(then it is clear that they are static and also available for other functions that might use them in the future)
A const does not need to be static, just leave it to the compiler to optimize the values used.
static only needed if changes to it should survive across re-invocations of the function.
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@487
PS4, Line 487: OSMO_ASSERT(sizeof(rates) == sizeof(offsets));
this should use an osmo_static_assert() (compile time)
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@506
PS4, Line 506: * rate in the form: i:r where i is the subsolt number and r the
(subsolt x2)
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@534
PS4, Line 534: { 0, 3, 4, 7, 8, 9, 10, -1, -1, -1, -1, -1, -1, -1, -1 },
(maybe decide on indent or no indent)
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@548
PS4, Line 548: };
(this array could also be global, further up in the .c file. If not, then there is no need to make it 'static', really)
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@563
PS4, Line 563: if (ts_nr == 0xff || ss_nr == 0xff) {
(here is a place that would look nicer to me if it was just a single conversion function...)
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@571
PS4, Line 571: for (i = 0; i < 15; i++) {
(could use sizeof(interlock_tab[0]) instead of 15?)
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@572
PS4, Line 572: /* Detect table end */
(more like row end?)
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@591
PS4, Line 591: * (This is an exceptional situation, that should not occurr
(occur)
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@592
PS4, Line 592: * in a properly configured environment!) */
IIUC then this function is called by e.g. a CRCX message coming from outside of osmo-mgw. Then that is rather an error situation that osmo-mgw needs to protect against, and properly configuring osmo-mgw is not related?
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@607
PS4, Line 607: bool mgcp_endp_avail(struct mgcp_endpoint *endp)
But a virtual endpoint could also already be in use?
IIUC this function may be described "check whether an endpoint is blocked by another endpoint (for E1)"?
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@611
PS4, Line 611: /* There are no obsticels that may render a virtual trunk
(obstacles)
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_endp.c@614
PS4, Line 614: return true;
but a virtual endpoint could also already be in use?
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_protocol.c
File src/libosmo-mgcp/mgcp_protocol.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_protocol.c@750
PS4, Line 750: if (!mgcp_endp_avail(endp)) {
(...this is the CRCX rx I mentioned elsewhere)
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_protocol.c@1011
PS4, Line 1011: if (!mgcp_endp_avail(endp)) {
an MDCX is by definition sent for an endpoint that is already in use.
wouldn't this check forbid all MDCX for all E1 endpoints?
(same for DLCX)
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_ratectr.c
File src/libosmo-mgcp/mgcp_ratectr.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_ratectr.c@66
PS4, Line 66: [MGCP_CRCX_FAIL_AVAIL] = { "crcx:availability", "endpoint unavailable." },
i'd prefer 'crcx:unavailable' (otherwise sounds like it counts successful CRCX)
or after previous discussion 'crcx:blocked'?
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_ratectr.c@94
PS4, Line 94: [MGCP_MDCX_FAIL_AVAIL] = { "mdcx:availability", "endpoint unavailable." },
MDCX only makes sense for conns that are already active.
So either we should get MGCP_MDCX_FAIL_NO_CONN, or the MDCX should succeed, right?
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_ratectr.c@116
PS4, Line 116: [MGCP_DLCX_FAIL_AVAIL] = { "dlcx:availability", "endpoint unavailable." },
same as for MDCX
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_vty.c
File src/libosmo-mgcp/mgcp_vty.c:
https://gerrit.osmocom.org/c/osmo-mgw/+/18898/4/src/libosmo-mgcp/mgcp_vty.c@203
PS4, Line 203: mgcp_endp_avail(endp) ? "available" : "not in service", VTY_NEWLINE);
how about those endpoints currently in use? We print "available" for all endpoints, which may be interpreted as not being used. Maybe also use the term "blocked" here?
Maybe don't print anything at all for non-blocked endpoints, only for those that are blocked add a "Blocked by: <other-epname>" ?
--
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/18898
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: I18e90b10648a7e504371179ad144645fc82e1c27
Gerrit-Change-Number: 18898
Gerrit-PatchSet: 4
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-CC: neels <nhofmeyr at sysmocom.de>
Gerrit-Comment-Date: Fri, 26 Jun 2020 15:16:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200626/4206f5df/attachment.htm>