dexter 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: (21 comments) @neels: Thanks for commenting on this patch. 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) Done 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. […] up to now osmo-mgw was only used with the virtual trunk since E1 trunks never worked. There is already a discussion going on whether we should have the counters per trunk or not. See also: https://gerrit.osmocom.org/c/osmo-mgw/+/18372/6//COMMIT_MSG#28 We probably might have a closer look and see for which counters it would make sense to have them per trunk. 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? Done 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**'. […] I just changed the array to a pointer array and allocate the the endpoints dynamically. This allows me to have a mgcp_endp_alloc() which can allocate everything an endpoint needs. I find it more logical when there is an alloc function for the endp that takes care of everything, including the memory. However, the alternative would be to have some mgcp_endp_init() and call that for each array element but this approach looks less clean to me. 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. […] Done 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_ratect […] Done https://gerrit.osmocom.org/c/osmo-mgw/+/18372/3/src/libosmo-mgcp/mgcp_endp.c File src/libosmo-mgcp/mgcp_endp.c: https://gerrit.osmocom.org/c/osmo-mgw/+/18372/3/src/libosmo-mgcp/mgcp_endp.c@100 PS3, Line 100: if (strlen(epname) <= prefix_len) > so why not simply using strcmp() if they are both guaranteed to be null > terminated? I am not sure, but I think it was correct in the beginning. What the function does is to return a pointer advanced to the position where the endpoint name begins. The check strlen(epname) <= prefix_len is not necessary since strncmp will also return a value other than 0 when the strings have different length. I have to use strncmp because I only want to check the prefix, the epname sting usually contains the prefix and then some endpoint name. The endpoint name has to be omitted for the check of course. Since I am confused now I have done an experiment, AAA is the prefix, AAABBB is the endpoint name with the prefix in front: This prints 1: printf("%u\n",strcmp("AAABBB", "AAA")); This prints 0: printf("%u\n",strncmp("AAABBB", "AAA", 3)); (also the tests fail when I use strcmp ;-) 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 uninfor […] Done 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'?) Done 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 Done 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. […] When we get a full endpoint name (one that is not wildcarded such as rtpbridge/1@mgw) we search for that specifc endpoint. If we do not find it we return NULL If the endpoint is part of a wildcarded request (e.g. rtpbridge/*@mgw) then we search for the next free endpoint and return that one. I have now cleaned up the function a bit and also splitted the wildcarded part and the non wildcarded part. However, it still needs to come together in one function. The idea behind this to agregate the evaluation of the endpoint name at one central point. 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") Done 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. […] I do not think that this would change much. I think its ok to log here, this way it is centralized and the caller does not have to care about it. 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, may […] I think this is ok, I also removed epname_len() now. 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. […] Unfortunately there is no way to determine the endp with the rtp_end, there is no backpointer or something. I see also that endp is only used for logging, but if we get rid of it we have no logging reference. 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. […] mgcp_trunk_alloc_endpts() is called once on startup when the VTY parses the config file. It is not intended to be called multiple times. I have now added an OSMO_ASSERT to ensure one can not call it multiple times. 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 max […] Done 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@597 PS7, Line 597: DEFUN(cfg_mgcp_sdp_payload_number, > DEFUN_DEPRECATED Done 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 Done 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 Done 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 Done -- 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 <pma...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter <pma...@sysmocom.de> Gerrit-Reviewer: neels <nhofm...@sysmocom.de> Gerrit-Reviewer: pespin <pes...@sysmocom.de> Gerrit-CC: laforge <lafo...@osmocom.org> Gerrit-Comment-Date: Wed, 03 Jun 2020 14:11:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: dexter <pma...@sysmocom.de> Comment-In-Reply-To: pespin <pes...@sysmocom.de> Comment-In-Reply-To: neels <nhofm...@sysmocom.de> Gerrit-MessageType: comment
