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/+/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 at 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 <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: Wed, 03 Jun 2020 14:11:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: dexter <pmaier at sysmocom.de>
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
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/20200603/1aa672b0/attachment.htm>