pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/18372 )
Change subject: osmo-mgw: refactor endpoint and trunk handling ...................................................................... Patch Set 2: Code-Review-1 (17 comments) https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2//COMMIT_MSG Commit Message: https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2//COMMIT_MSG@10 PS2, Line 10: implemented in various placed (mostly mgcp_protocol.c). Also we use places https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2//COMMIT_MSG@14 PS2, Line 14: The trunk and endpoint handling in osmo-mgw is still very complex and This paragraph is repeated. https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2//COMMIT_MSG@25 PS2, Line 25: - rename struct mgcp_trunk_config to mgcp_trunk and "tcfg" to "trunk" Having an "mgcp_trunk" structure and a "trunk" one looks confusing to me. https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/include/osmocom/mgcp/mgcp_endp.h File include/osmocom/mgcp/mgcp_endp.h: https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/include/osmocom/mgcp/mgcp_endp.h@81 PS2, Line 81: /*! Backpointer to the related trunk */ "Backpointer to the trunk this endpoint belongs to" would probably be more clear for newcomers to understand relation between objects. https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_endp.c File src/libosmo-mgcp/mgcp_endp.c: https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_endp.c@101 PS2, Line 101: (epname, MGCP_ENDPOINT_PREFIX_VIRTUAL_TRUNK, what if len(epname) < prefix_len? Is strncmp safe here? https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_endp.c@158 PS2, Line 158: * wildarded endpoint searches that picks the next free endpoint on wildcarded https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_endp.c@183 PS2, Line 183: endp = trunk->endpoints[i]; (We should move to a hashtable with key=str and val=ptr at some point now that we use strings.) https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_endp.c@188 PS2, Line 188: "(trunk:%i) found endpoint: %s\n", %d https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_endp.c@196 PS2, Line 196: "(trunk:%i) Not able to find specified endpoint: %s\n", %d https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_endp.c@254 PS2, Line 254: if (trunk->trunk_type == MGCP_TRUNK_VIRTUAL) { (At some point we should have function pointers to do type-specific stuff. Like in here trunk->check_domain_name().) https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_protocol.c File src/libosmo-mgcp/mgcp_protocol.c: https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_protocol.c@285 PS2, Line 285: /* FIXME: We hardcode to pick cfg->virt_trunkl, but trailing whitespace https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_protocol.c@288 PS2, Line 288: struct rate_ctr_group *rate_ctrs = trunk->mgcp_general_ctr_group; The rate_ctrs used below look general and not related to a trunk, so they should mbe moved to be under "mgcp_config cfg" instead of "cfg->virt_trunk". https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_protocol.c@996 PS2, Line 996: struct mgcp_endpoint *endp = p->endp; Swap these two lines: struct mgcp_endpoint *endp = p->endp; struct mgcp_trunk *trunk = endp->trunk; https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_protocol.c@1220 PS2, Line 1220: struct mgcp_endpoint *endp = p->endp; Same, swap these two lines. https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_sdp.c File src/libosmo-mgcp/mgcp_sdp.c: https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_sdp.c@386 PS2, Line 386: LOGP(DLMGCP, LOGL_NOTICE, "endpoint:%s, failed to add codec\n", endp->name); I see a lot of endpoint:%s logging, what about introducing LOGPENDP(endp, CAT, "...") at some point? https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_trunk.c File src/libosmo-mgcp/mgcp_trunk.c: https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/src/libosmo-mgcp/mgcp_trunk.c@30 PS2, Line 30: static const struct rate_ctr_desc mgcp_general_ctr_desc[] = { As I said, to me lots of these counters are not per-trunk, but global, so they shouldn't be here. https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/tests/mgcp/mgcp_test.c File tests/mgcp/mgcp_test.c: https://gerrit.osmocom.org/c/osmo-mgw/+/18372/2/tests/mgcp/mgcp_test.c@74 PS2, Line 74: #define AUEP1_RET "500 158663169 FAIL\r\n" What about these changes in behavior? expected? I don't recall seeing any comment in commit deescription. -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/18372 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: Ice8aaf03faa2fd99074f8665eea3a696d30c5eb3 Gerrit-Change-Number: 18372 Gerrit-PatchSet: 2 Gerrit-Owner: dexter <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <[email protected]> Gerrit-Comment-Date: Wed, 27 May 2020 07:40:36 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
