Patch Set 9: Code-Review-1

(9 comments)

https://gerrit.osmocom.org/#/c/5881/9/include/osmocom/mgcp_client/mgcp_client_fsm.h
File include/osmocom/mgcp_client/mgcp_client_fsm.h:

Line 11: struct mgcp_conn_info {
naming is not clear enough "conn_info" sounds like some generic information 
about a connection.  Rather, it's information about a "peer", or an "end".  
Good naming is important to make sure people understand an API.  My initial 
idea wouldbe mgcp_conn_peer but feel free to use anything else that implies 
it's only one side of the connection.

But then, it seems somehow unclear.  addr/port for sure are the "side".  But 
"call_id" and "endpoint" are actually  parameters of the connection, not just 
of one end.  I think this is unclear.  Why would each side of a connection 
contain endpoint/call_id?


https://gerrit.osmocom.org/#/c/5881/9/include/osmocom/mgcp_client/mgcp_client_internal.h
File include/osmocom/mgcp_client/mgcp_client_internal.h:

Line 32:                                                      mgcp_trans_id_t 
trans_id,
unrelated whitespace change?


https://gerrit.osmocom.org/#/c/5881/9/src/libosmo-mgcp-client/mgcp_client_fsm.c
File src/libosmo-mgcp-client/mgcp_client_fsm.c:

Line 7:  * it under the terms of the GNU Affero General Public License as 
published by
no AGPL in those libraries.  the old libosmo-{legacy} mgcp was plain 
GPLv2-or-later, and I don't see why it would change now with the new library.


Line 97: struct msgb *make_crcx_msg_bind(struct mgcp_ctx *mgcp_ctx)
static?


Line 112: struct msgb *make_crcx_msg_bind_connect(struct mgcp_ctx *mgcp_ctx)
static?


Line 130: struct msgb *make_mdcx_msg(struct mgcp_ctx *mgcp_ctx)
static?


Line 153: struct msgb *make_dlcx_msg(struct mgcp_ctx *mgcp_ctx)
static?


Line 443: static void fsm_cleanup_cb(struct osmo_fsm_inst *fi, enum 
osmo_fsm_term_cause cause)
what about the cleanup issuing an unconditional DLCX to the MGW, just incase 
there might still be some state left?  I think we cannot wait/delay in the 
cleanup_cb, but we could send a DLCX?  I guess the best is to never terminate 
*only* through the cleanup_cb but always cleanly


Line 511:       .name = "MGCP_CLIENT",
I think it's not a client FSM, but a client connection (or mgcp connection 
client) FSM. IF you agree, this should be reflectd in the name.


-- 
To view, visit https://gerrit.osmocom.org/5881
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I887ce0c15a831dffeb6251a975337b83942af566
Gerrit-PatchSet: 9
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Owner: dexter <pma...@sysmocom.de>
Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pma...@sysmocom.de>
Gerrit-HasComments: Yes

Reply via email to