This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Harald Welte gerrit-no-reply at lists.osmocom.org
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