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/+/18644 )

Change subject: osmo-mgw: refactor endpoint and trunk handling
......................................................................

Patch Set 6: Code-Review+1

(13 comments)

Take a look if any of my remarks make sense, but none of this blocks.

I agree with laforge that keeping stats per-trunk is more desirable and also seems to be very easy here, just put a struct mgcp_ratectr in the trunk instead of the cfg? What's the rationale for having only global stats?

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

https://gerrit.osmocom.org/c/osmo-mgw/+/18644/6/include/osmocom/mgcp/mgcp.h@251 
PS6, Line 251: 	 * health */
(it seems to be your personal style to prefer vertical comments, so I won't complain.
But let me say here once, me personally, I'd prefer using the 120 char width we have agreed on -- especially if it's a new line for only one word.)

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

https://gerrit.osmocom.org/c/osmo-mgw/+/18644/6/src/libosmo-mgcp/mgcp_endp.c@49 
PS6, Line 49: 	endp->cfg = trunk->cfg;
> Not sure if whitespace at start of this line is correct.
(I don't see anything wrong)

https://gerrit.osmocom.org/c/osmo-mgw/+/18644/6/src/libosmo-mgcp/mgcp_endp.c@93 
PS6, Line 93:  * the same for all endpoints, so no ambiguity is introduced) */
would be nice to define "chop it off": write the epname without the prefix back to the memory pointed at by epname.

https://gerrit.osmocom.org/c/osmo-mgw/+/18644/6/src/libosmo-mgcp/mgcp_endp.c@113 
PS6, Line 113: 		return;
maybe

 default:
       OSMO_ASSERT(false);

https://gerrit.osmocom.org/c/osmo-mgw/+/18644/6/src/libosmo-mgcp/mgcp_endp.c@118 
PS6, Line 118:  * it off */
also would be nice to define "chop it off" here: truncate epname by writing a '\0' char where the suffix starts.

https://gerrit.osmocom.org/c/osmo-mgw/+/18644/6/src/libosmo-mgcp/mgcp_endp.c@136 
PS6, Line 136:  * and not needed to identify the endpoint on the trunk */
I don't understand "all information that is already evaluated".
Maybe "return the epname stripped of prefix and suffix and converted to lower case"?

Also would be nice to indicate the expected buf size of epname_stripped.

https://gerrit.osmocom.org/c/osmo-mgw/+/18644/6/src/libosmo-mgcp/mgcp_endp.c@140 
PS6, Line 140: 	osmo_str_tolower_buf(epname_stripped, strlen(epname), epname);
the second argument is the memory length of epname_stripped to safeguard against writing past its end, regardless of the input strlen. Passing strlen(epname) is breaking the purpose of that len. Since there is no explicit epname_stripped_buflen arg, IIUC this should be MGCP_ENDPOINT_MAXLEN?

https://gerrit.osmocom.org/c/osmo-mgw/+/18644/6/src/libosmo-mgcp/mgcp_endp.c@181 
PS6, Line 181: 			endp->wildcarded_req = false;
this function is called "find_...", could be a bad idea to also modify the endp here?
Though I must admit that I'm not sure what wildcarded_req does.

(then you could also add const to the 'trunk' arg)

https://gerrit.osmocom.org/c/osmo-mgw/+/18644/6/src/libosmo-mgcp/mgcp_endp.c@195 
PS6, Line 195: 					      struct mgcp_trunk *trunk)
const struct mgcp_trunk ?

https://gerrit.osmocom.org/c/osmo-mgw/+/18644/6/src/libosmo-mgcp/mgcp_endp.c@210 
PS6, Line 210: 				 trunk->trunk_nr, endp->name);
I think set endp->wildcarded = true here?

https://gerrit.osmocom.org/c/osmo-mgw/+/18644/6/src/libosmo-mgcp/mgcp_endp.c@225 
PS6, Line 225: 	if (endp) {
and wildcarded = false here?

https://gerrit.osmocom.org/c/osmo-mgw/+/18644/6/src/libosmo-mgcp/mgcp_msg.c 
File src/libosmo-mgcp/mgcp_msg.c:

https://gerrit.osmocom.org/c/osmo-mgw/+/18644/6/src/libosmo-mgcp/mgcp_msg.c@234 
PS6, Line 234: 		     line, endp->name);
very good to see the "0x1" endp naming go away!

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

https://gerrit.osmocom.org/c/osmo-mgw/+/18644/6/src/libosmo-mgcp/mgcp_trunk.c@143 
PS6, Line 143: struct mgcp_trunk *mgcp_trunk_by_name(const char *epname, struct mgcp_config *cfg)
if this is a new function: the mgcp_trunk_by_num() has cfg as the first argument, better also do that here.
(If this is just moving an old function, then rather keep it inconsistent I guess.)

-- 
To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/18644
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Change-Id: Ia8cf4d6caf05a4e13f1f507dc68cbabb7e6239aa
Gerrit-Change-Number: 18644
Gerrit-PatchSet: 6
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Wed, 10 Jun 2020 19:41:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200610/7d675761/attachment.htm>