Patch Set 4: (20 comments)
https://gerrit.osmocom.org/#/c/4003/4/include/osmocom/mgcp/mgcp.h File include/osmocom/mgcp/mgcp.h: Line 37: #define RTP_PORT_DEFAULT_RANGE_END RTP_PORT_DEFAULT_RANGE_START + 10 why do we have a default range end of only 10 ports higher than the start port? given that each RTP connection needs two ports (even for RTP, odd for RTCP), that means we only have five connections by default? https://gerrit.osmocom.org/#/c/4003/4/include/osmocom/mgcp/mgcp_internal.h File include/osmocom/mgcp/mgcp_internal.h: Line 114: /* Each end has a separete socket for RTP and RTCP */ separate Line 149: struct mgcp_rtp_end end; the 'rtp_end' structure made sense when we had a bts_end and a net_end. Now it _might_ make sense to migrate it into mgcp_conn_rtp, as it is only used once in one place (here). Not a requirement, just a thought. If at all, should be a follow-up patch. Line 193: int mode; should this be some enum? or at least the comment above state what kind of values/bitmask/enum/#defines apply here? Line 223: enum mgcp_type type; if it's an endpoint type, mgcp_endpoint_type might be a better name https://gerrit.osmocom.org/#/c/4003/4/src/libosmo-mgcp/mgcp_conn.c File src/libosmo-mgcp/mgcp_conn.c: Line 72: struct mgcp_conn *mgcp_conn_alloc(void *ctx, struct llist_head *conns, as the connection list is per endpoint, it might make sense to pass in the mgcp_endpoint here. the endpoint then is used to derive context + llist_head. Seems more natural to me: pass in the endpoint and state "please alloc a new connection for this endpoint". Line 82: if (llist_count(conns) >= 2) this is something specific to the "rtp bridge/proxy" endpoint type. I suggest introducing something like a 'const struct rtp_endpoint_type' which has a max_conns member. (currently all) mgcp_endpoint would then point to that one struct rtp_endpoint_type, and the comparison here would compare with endpoint->max_conns. This way we're well prepared to introduce other endpoint types in the future. Line 93: conn->type = type; rather than the enum, this would be a pointer to the 'const struct rtp_endpoint_type' which we would initially have for 'rtp_bridge' but which we would in the future have multiple. Line 105: mgcp_rtp_end_reset(&conn->u.rtp.end); why does mgpc_rtp_end_reset() not set those -1 values above? Line 123: struct mgcp_conn *mgcp_conn_get(struct llist_head *conns, uint32_t id) Is there a connection list outside of the context of a struct mgcp_endpoint? If no, I would suggest to pass in the struct mgcp_endpoint here rather than the llist_head pointer. This is what we do in many other places of osmocom code, too. PS4, Line 169: mgcp_conn_get_rtp_by_fd I wonder when do we need this. The below linear iteration seems quite expensive, if you consider it is done for each received RTP packet (every 20ms) on each RTP socket. Normally I would expect osmo_fd.priv to point to the mgcp_conn_rtp, and this function would be just a pointer de-reference. https://gerrit.osmocom.org/#/c/4003/4/src/libosmo-mgcp/mgcp_network.c File src/libosmo-mgcp/mgcp_network.c: Line 704: conn_dst->end.packets_tx += 1; using an any of the libosmocore counters might be an idea here, rather than having hand-coded counters. A rate_counter is probably overkill, but then it would come with free CTRL interface access to the counters, as well as VTY dumping? It is what we e.g. use in the SGSN to count per-direction packets and bytes for a PDP context. Line 806: endp = (struct mgcp_endpoint *)fd->data; see my other comment, as the socket/fd is part of the mgcp connection, priv should probably point to that, and not directly to the endpoint? Would you agree? Line 889: conn_src = mgcp_conn_get_rtp_by_fd(&endp->conns, fd); see my other comment, this is a highly inefficient lookup, and we appear to perform it in the most frequently used code path. Line 911: llist_for_each_entry(conn, &endp->conns, entry) { This lookup and the code below is again specific to the endpoint type. I would envision the rtp_endpoint_type should have a callback function member that "consumes" the received RTP and then does whatever is specific for this endpoint type. For the "rtp-relay/proxy" endpoint type, that function then will perform the lookup + sending on the "peer" connection. For future other endpoints such as announcement playback, E1, ... the "consume/receive" function will be different and perform the specific operation. https://gerrit.osmocom.org/#/c/4003/4/src/libosmo-mgcp/mgcp_osmux.c File src/libosmo-mgcp/mgcp_osmux.c: Line 443: /* FIXME: Get rid of CONN_ID_XXX! */ whitspace, but then thos FIXME should be resolved before merge anyway. Line 585: return; whitespace Line 613: whitespace https://gerrit.osmocom.org/#/c/4003/4/src/libosmo-mgcp/mgcp_protocol.c File src/libosmo-mgcp/mgcp_protocol.c: Line 587: /* Check if we are able to accept the creation of another connection */ this kind of handling (probably pretty much all of the CRCX/MDCX processing after the parsing and some general syntax validation) is again specific to the endpoint type and should be performed in a callback function that is a member of 'struct rtp_endpoint_type'. The specific implementation here only applies to the 'RTP proxy/relay' type. Line 984: LOGP(DLMGCP, LOGL_ERROR, we should probably have some kind of LOGP/DEBUGP macro to which we pass the mgcp_endpoint or mgcp-connection, and which then prints the connection/endpoint identity. Just like in most other osmocom programs. -- To view, visit https://gerrit.osmocom.org/4003 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie008599136c7ed8a0dfbb0cf803188975a499fc5 Gerrit-PatchSet: 4 Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter <[email protected]> Gerrit-HasComments: Yes
