osmo-mgw[master]: client: add an optional FSM interface

Harald Welte gerrit-no-reply at lists.osmocom.org
Sun Feb 4 08:10:00 UTC 2018


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 <pmaier at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-HasComments: Yes


More information about the gerrit-log mailing list