Patch Set 5: (20 comments)
> New patch set from dexter's branch pmaier/mgw4 and my own > adjustments. For the record, submitted the patch set without asking > dexter first, I hope it's not premature. Thanks for adjusting the patch. My main concern at the moment is how to move on with OSMUX. I think I can not fix the OSMUX related issues on the short run. I would need to learn more about osmox, how it works, how it is used. I would prefer to merge the patch without addressing the OSMUX issues at the moment. https://gerrit.osmocom.org/#/c/4003/4/include/osmocom/mgcp/mgcp.h File include/osmocom/mgcp/mgcp.h: Line 37: > why do we have a default range end of only 10 ports higher than the start p Done 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 separate socket for RTP and RTCP */ > separate Done Line 149: /* Specific connection type */ > the 'rtp_end' structure made sense when we had a bts_end and a net_end. Now I think so too, removing one level would make handling the struct a lot easier. I will keep that in in mind. Line 193: /*!< Backpointer to the endpoint where the conn belongs to */ > should this be some enum? or at least the comment above state what kind of Done Line 223: (e.g mgcp_dispatch_rtp_bridge_cb, see below) */ > if it's an endpoint type, mgcp_endpoint_type might be a better name having enum mgcp_type here type is wrong. This is something that is specific to an RTP connection only. I have moved it. https://gerrit.osmocom.org/#/c/4003/4/src/libosmo-mgcp/mgcp_conn.c File src/libosmo-mgcp/mgcp_conn.c: Line 72: /*! \brief allocate a new connection list entry > as the connection list is per endpoint, it might make sense to pass in the Done Line 82: struct mgcp_conn *conn; > this is something specific to the "rtp bridge/proxy" endpoint type. I sugg Done Line 93: return NULL; > rather than the enum, this would be a pointer to the 'const struct rtp_endp This sets the type of the connection, so its not about the endpoint here. (We mainly use this to determine how to access the union.) However, this brings me to another thought. If we have various possible connection types. rtp_endpoint_type should have some bitfield with allowed connection types. But thats is not in the scope of this patch I think. Line 105: strcpy(conn->name, name); > why does mgpc_rtp_end_reset() not set those -1 values above? Done Line 123: /*! \brief find a connection by its ID > Is there a connection list outside of the context of a struct mgcp_endpoint Done PS4, Line 169: ns.next != NULL && endp > I wonder when do we need this. The below linear iteration seems quite expe Done Line 225: } > What if the list is empty? Done https://gerrit.osmocom.org/#/c/4003/4/src/libosmo-mgcp/mgcp_network.c File src/libosmo-mgcp/mgcp_network.c: Line 704: > using an any of the libosmocore counters might be an idea here, rather than I think its best to solve this in a separate patch. I have created a task, so we do not forget about this. https://osmocom.org/issues/2517 Line 806: ENDPOINT_NUMBER(endp)); > see my other comment, as the socket/fd is part of the mgcp connection, priv Done Line 889: * port and IP-Address make sense at all. If not, we will be unable > see my other comment, this is a highly inefficient lookup, and we appear to Done Line 911: } > This lookup and the code below is again specific to the endpoint type. I wo Done https://gerrit.osmocom.org/#/c/4003/4/src/libosmo-mgcp/mgcp_osmux.c File src/libosmo-mgcp/mgcp_osmux.c: Line 126: h->in->osmux_seq = 0; > (whitespace) Done Line 205: > (whitespace) Done Line 585: return; > whitespace Done Line 613: > whitespace Done -- 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: 5 Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Holger Freyther <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: dexter <[email protected]> Gerrit-HasComments: Yes
