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 <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: neels <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-Comment-Date: Wed, 10 Jun 2020 19:41:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: pespin <[email protected]> Gerrit-MessageType: comment
