Change in osmo-mgw[master]: osmo-mgw: refactor endpoint and trunk handling

historical

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 <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Wed, 27 May 2020 07:40:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200527/eab1c86e/attachment.htm>