Change in osmo-mgw[master]: endp: add E1 endpoint interlocking

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/.

dexter gerrit-no-reply at lists.osmocom.org
Mon Jun 29 15:31:34 UTC 2020


dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/18898 )

Change subject: endp: add E1 endpoint interlocking
......................................................................


Patch Set 5:

(19 comments)

(ensure all comments are sent)

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 c […]
Done


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)
Done


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
Done


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)
Done


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. […]
I thought about doing that while I was doing the implementation, but I prefer to have a single function for each parameter.


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? […]
Done


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)
Done


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)
Done


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)
Done


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... […]
Done


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?)
Done


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?)
Done


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)
Done


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. […]
If this happens its not a problem at the mgw side. osmo-mgw will reject the request with an error code. Its the requestor that has a problematic configuration or malfunction then.


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)
Done


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?
In "in use" means something different here. "Available" just means that the client can do something with the endpoint (CRCX, DLCX, MDCX etc.). Its more like an "in service" or "out of service" thing.


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) […]
Done


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. […]
It does not matter if MDCX, CRCX etc would succeed or not, this is checked much later. The question is, does the client try to something with an endpoint that is currently not in service. (e.g. The linecard for this endpoint is not installed in the rack.). If there is an active call on an endpoint and if it is not allowed to create another connection, than the endpoint is "available" and the operation will fail for a different reason.

I think it fails earlier and the _NO_CONN counter does not tick. I added the availablity as a separate counter because especially with the E1 the problem could be difficult to diagnose. Its an unlikely situation that the bsc will mess up the endpoints, but if it ever happens we need to be able spot this immediately.


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
Done



-- 
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: 5
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: Mon, 29 Jun 2020 15:31:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200629/d642ad2e/attachment.htm>


More information about the gerrit-log mailing list