osmo-msc[master]: mgcp: use osmo-mgw to switch rtp streams

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/.

Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Mon Dec 25 06:59:24 UTC 2017


Patch Set 16: Code-Review+1

(14 comments)

I am still finding things (you know me) ... but I think it is becoming not worth it to fix them before merging. We've really had more than enough passes on it, from your comments this patch seems to work well enough, and my comments can all be addressed later. Even if they get dropped on the floor, maybe this will help improving future patches.

I would welcome for the future that you actually go on gerrit and read through your patch like a reviewer, and you should find the typos I keep marking, by yourself, that would be cool.

Here follow the comments, but if another reviewer agrees that none of them are worthwhile another cycle, I'd be ok with merging as-is.

I think the most important one is "Line 903".

https://gerrit.osmocom.org/#/c/4980/16/include/osmocom/msc/msc_mgcp.h
File include/osmocom/msc/msc_mgcp.h:

Line 34: 	/* RTP endpoint number. This number number identifies the endpoint
number number


https://gerrit.osmocom.org/#/c/4980/16/src/libmsc/a_iface.c
File src/libmsc/a_iface.c:

Line 403: 		LOGP(DMSC, LOGL_ERROR,
would be nice to use LOGPCONN as above in line 392


https://gerrit.osmocom.org/#/c/4980/16/src/libmsc/iucs.c
File src/libmsc/iucs.c:

Line 42: extern struct msgb *ranap_new_msg_rab_assign_voice(uint8_t rab_id,
wondering why this is necessary, typically we would #include a header with this definition. extern is not necessary (I'm not really sure why extern exists as a keyword for functions)


https://gerrit.osmocom.org/#/c/4980/16/src/libmsc/msc_mgcp.c
File src/libmsc/msc_mgcp.c:

Line 183: 		osmo_fsm_inst_dispatch(fi, EV_TEARDOWN_ERROR, mgcp_ctx);
I still think these two lines would make a good common static function like mgcp_fsm_error() ... wait, you actually have a handle_error() function, isn't it applicable here?


Line 742: 	LOGPFSML(fi, LOGL_DEBUG, "state machine halted\n");
(the FSM internal logging just logged a state transition to ST_HALT, this is duplicating that, right?)


Line 757: 	osmo_fsm_inst_state_chg(fi, ST_HALT, MGCP_REL_TIMEOUT, MGCP_REL_TIMEOUT_TIMER_NR);
fsm_halt_cb is only invoked from receving events in the ST_HALT state. Why re-enter ST_HALT from ST_HALT?

>From another angle: above comment says "state machine halted", yet there is another state change triggered?


Line 763: 	[ST_CRCX_RAN] = {
Thanks for the improvements, they help to understand.

I still find that in the naming of the states, there is a mixup between the action, state and event. When an action like "sending X" happened, a state is like "waiting for Y", and event then is "received Y". Here the FSM states' naming tend to be like the action that came before entering a state or the event that triggered leaving the previous state. So when we're actually waiting for Y, the state is named "send X".

In this particular state, the FSM init code probably sent the CRCX for RAN, and now this state could be ST_WAIT_CRCX_RAN_RESP. The comment above suggests that entering the state implicitly sends the CRCX (theoretically possible with an on_enter cb, but there is none).

Other example: in handle_error(), we enter the ST_CALL state. 
That reads like: on any error, we have a call established.

So, what I mean is, when reading, I find that questions remain that can only be answered by reading more of the code, and it would be nice to clearly know who did the action (the state doesn't do actions, it's just a number) and what the state is waiting for by the name and comment alone.


Line 770: 	   sending the CRCX for CN side */
*


Line 797: 	 * completed we will check the IP/Port of the RAN connection. If we
as completed


Line 798: 	 * this data is valid we may continue with the MDCX phase for the RAN
If we this


Line 799: 	 * side. If not we wait until the assinment completes on the A or on
assinment


Line 816: 	/* The ran side MDCX phase is complete when the response is received
'ran' the verb or RAN the acronym


Line 817: 	 * from the MGW. The is then active and we change to ST_CALL and wait
The is


Line 903: 	mgcp_ctx->fsm = osmo_fsm_inst_alloc(&fsm_msc_mgcp, NULL, NULL, LOGL_DEBUG, name);
I still think it should be a child fsm of conn->conn_fsm


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ieea9630358b3963261fa1993cf1f3b563ff23538
Gerrit-PatchSet: 16
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>
Gerrit-HasComments: Yes



More information about the gerrit-log mailing list