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

historical

neels 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 7: Code-Review-1

(21 comments)

Nice, a lot of excellent cleanup work!

I hope you see the large number of comments as appreciation of the patch.

May seem ironic from me as Mr. Code Bomb himself, but this patch would be fairly easy to split into smaller parts. I'm thinking the deprecation of the sdp audio name vty commands, some renames, and ideally moving old code to new files first without changing them so that code review can easily see the changes made to the code?

The -1 vote is for the DEFUN_DEPRECATED, and missing items in the commit log (or alternatively splitting up the patch), and the talloc_free of trunk->endpoints.

Anyway, cool to see osmo-mgw being streamlined :)

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7//COMMIT_MSG 
Commit Message:

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7//COMMIT_MSG@21 
PS7, Line 21:    symbol name "tcfg" to "trunk" in order to better match the reality.
(maybe do renames in a separate patch)

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7//COMMIT_MSG@28 
PS7, Line 28:    longer allocate them per trunk. Allocate them globally instead.
I'm wondering whether anyone would miss per-trunk counters in the future. I don't know of anyone using more than one trunk, but if we have multiple trunks supported in the code, maybe per-trunk counters would be nicer to keep than to remove?

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/include/osmocom/mgcp/mgcp.h 
File include/osmocom/mgcp/mgcp.h:

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/include/osmocom/mgcp/mgcp.h@a189 
PS7, Line 189: char *audio_name;
             : 	int audio_payload;
these are no longer present in struct mgcp_trunk, maybe explain why in the commit log?

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/include/osmocom/mgcp/mgcp_trunk.h 
File include/osmocom/mgcp/mgcp_trunk.h:

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/include/osmocom/mgcp/mgcp_trunk.h@38 
PS7, Line 38: 	struct mgcp_endpoint **endpoints;
changing this from 'struct mgcp_endpoint*' to 'struct mgcp_endpoint**'.
What is the reason? If I see this right, we are anyway just allocating a fixed number of endpoints at trunk creation time, and it seems unnecessary to add to this the individual dynamic allocation for every contained endpoint?

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_codec.c 
File src/libosmo-mgcp/mgcp_codec.c:

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_codec.c@a289 
PS7, Line 289: 
this removal is not mentioned in the commit log. I guess it should be a separate patch from the refactoring.

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_conn.c 
File src/libosmo-mgcp/mgcp_conn.c:

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_conn.c@259 
PS7, Line 259: aggregate_rtp_conn_stats(struct mgcp_endpoint *endp, struct mgcp_conn_rtp *conn_rtp)
This function does not access the individual endp, but accesses the single global struct mgcp_ratectr. It would make more sense to pass a struct mgcp_ratectr as argument, to reflect the fact that now there is only one global set of counters (besides the per-conn counters).

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_endp.c 
File src/libosmo-mgcp/mgcp_endp.c:

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_endp.c@90 
PS7, Line 90: /* Check if the endpoint name contains the prefix, and chop it off, if it
(would be nice to include an example string for a prefix to make it easier to understand for uninformed readers)

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_endp.c@140 
PS7, Line 140:  *  \param[out] cause, pointer to store cause code, can be NULL.
(I think doxygen wants no comma after 'cause'?)

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_endp.c@141 
PS7, Line 141:  *  \param[in] epname endpoint name to lookup (may lack trunk prefix and domain name).
wildcard should be explained

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_endp.c@164 
PS7, Line 164: 	if (strncmp(epname_ch, "*", epname_ch_len) == 0) {
If I get this right, with a full name, this function finds an existing (used?) endpoint.
In this condition here, if the endpoint name is exactly "*", this finds the first unused endpoint.
It feels to me like these should be two separate functions, one takes an epname to look up, the other takes no epname and finds an unused one.

EDIT: I see now that this function was just moved to a different file...

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_endp.c@184 
PS7, Line 184: 	/* Find an enspoint by its name (if wildcarded request is not
("enspoint")

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_endp.c@201 
PS7, Line 201: 	     trunk->trunk_nr, epname);
(might be better to leave the logging to callers. There could be cases where a caller expects that some endpoint should be gone, and then an error log of "Not able to find" would confuse readers of the osmo-mgw log to think that something went wrong, even though all is as expected)

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_endp.c@214 
PS7, Line 214: 	domain_to_check = strstr(epname, "@");
(could be nice for the future to keep a single implementation for separating epname from domain, maybe use epname_len(), so we have only one place that does strchr('@')? Just an idea...)

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_network.c 
File src/libosmo-mgcp/mgcp_network.c:

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_network.c@1439 
PS7, Line 1439: 		    struct mgcp_rtp_end *rtp_end, struct mgcp_endpoint *endp)
endp is only used for logging. Isn't rtp_end also part of endp? Maybe I'm wrong, but seems like it should be enough to pass only endp or only rtp_end?

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_trunk.c 
File src/libosmo-mgcp/mgcp_trunk.c:

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_trunk.c@77 
PS7, Line 77: 		talloc_free(trunk->endpoints);
the comment says "(re)allocate", but free+alloc loses all endpoint data.
Is this intended to change during a running osmo-mgw without losing endpoint state?
Then we could use talloc_realloc() instead, which keeps the data (as much of it as is possible).

If not, then talloc_free(trunk->endpoints) might not be necessary, or should maybe also first take care to shut down RTP ports and so on?

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_trunk.c@79 
PS7, Line 79: 					     sizeof(struct mgcp_endpoint *), trunk->vty_number_endpoints, "endpoints");
before this, maybe we should validate vty_number_endpoints > 0 (and maybe smaller than some sane maximum?)

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_vty.c 
File src/libosmo-mgcp/mgcp_vty.c:

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_vty.c@a117 
PS7, Line 117: 		vty_out(vty, "  sdp audio-payload number %d%s",
not mentioned in the commit log?

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_vty.c@597 
PS7, Line 597: DEFUN(cfg_mgcp_sdp_payload_number,
DEFUN_DEPRECATED

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_vty.c@610 
PS7, Line 610: DEFUN(cfg_mgcp_sdp_payload_name,
DEFUN_DEPRECATED

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_vty.c@899 
PS7, Line 899: DEFUN(cfg_trunk_payload_number,
DEFUN_DEPRECATED

https://gerrit.osmocom.org/c/osmo-mgw/+/18372/7/src/libosmo-mgcp/mgcp_vty.c@911 
PS7, Line 911: DEFUN(cfg_trunk_payload_name,
DEFUN_DEPRECATED

-- 
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: 7
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-CC: laforge <laforge at osmocom.org>
Gerrit-Comment-Date: Tue, 02 Jun 2020 13:54:45 +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/20200602/0e8bf0e2/attachment.htm>