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