osmo-bsc[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
Tue Oct 31 01:37:08 UTC 2017


Patch Set 7: Code-Review-1

(11 comments)

https://gerrit.osmocom.org/#/c/4334/4/src/osmo-bsc/osmo_bsc_mgcp.c
File src/osmo-bsc/osmo_bsc_mgcp.c:

Line 130: 
> I am not so sure about this. While developing this the OSMO_ASSERT() helped
yes, that's fine. Just consider that for the future. I remember myself using a lot of non-null assertions all over the place, until I realized one day that most are just bloat...


Line 143: 
> I think even if it won't change anything, it is logically incorrect. The ti
erm, not sure I understand. AFAIK when you change an FSM's state with 0 for timeout, there will not be any timeout, the fsm will happily remain in that state forever. Is there another timeout function I'm not aware of? If yes it should probably be incorporated into the fsm?

...hold on, this waits for the subscriber to end the call, right? Does it make sense to at least give it a ridiculous timeout, like 24 hours?


Line 147: 	LOGPFSML(mgcp_ctx->fsm, LOGL_DEBUG, "fsm-state: %s\n", get_value_string(fsm_bsc_mgcp_state_names, fi->state));
ah damn, it appears all of my comments here are wrong, because this is the error handling. You go to the DLCX state and then dispatch an event to it to take an action. It is a bit unintuitive to me because in the VLR FSMs I think we call a static function to tear down and put the fsm directly in the final ending state without dispatching events ... I guess either way is fine.

Just tell me now that I got it right this time, and all is fine.


Line 185: 
> (Note: lindent/indent has problems with that format)
hmm, then I'm not going to use lindent :P


Line 190: 	mgcp_msg = (struct mgcp_msg) {
> If we do this we will loose the flexibility to support any endpoint naming 
right, so we need flexible endpoint strings.

-1: However, snprintf can fail, shouldn't this have error handling of sorts?


Line 193: 			     MGCP_MSG_PRESENCE_CONN_MODE),
> I do not think that we will benefit much from this. The presence field is i
we have osmo_sccp_make_addr_pc_ssn(), osmo_sccp_addr_set_ssn() :)


Line 499: 
> I am not sure here, but I think this is should be catched by the client (we
...which would mean the client needs to cache all the sent messages. I don't think we want that. If the MGW sends a CRCX OK message and we can't parse that, I think going to DLCX is the right move here. It shouldn't actually happen in a real setup, so this is just waterproofing.


Line 814: 	mgcp_ctx->fsm = osmo_fsm_inst_alloc(&fsm_bsc_mgcp, NULL, ctx, LOGL_DEBUG, name);
> Probably we should have also some protection that makes it impossible to re
harald has already merged such protection, which is why some builds were failing and we had to fix some names that are already in master. Remember?


https://gerrit.osmocom.org/#/c/4334/7/src/osmo-bsc/osmo_bsc_vty.c
File src/osmo-bsc/osmo_bsc_vty.c:

Line 977: 	struct gsm_network *net = bsc_gsmnet;
and earlier comment: why create a local var if it is used only once and also is just an alias for another short enough name?


Line 978: 	
-1: ws


Line 1044: 	
-1: ws


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2882b7ca31a3219c676986e85045fa08a425d7a
Gerrit-PatchSet: 7
Gerrit-Project: osmo-bsc
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