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/.
dexter gerrit-no-reply at lists.osmocom.org
Patch Set 7:
(44 comments)
> please merge 4375 and 4376 into this patch, as agreed before? If
> the build failure is due to an unmerged patch in libosmocore or
> something, please add a 'Depends:' tag.
Sorry, I forgot about the patches. Thanks for reminding me. I have now integrated the changes, please have another look.
https://gerrit.osmocom.org/#/c/4334/4/include/osmocom/bsc/osmo_bsc_mgcp.h
File include/osmocom/bsc/osmo_bsc_mgcp.h:
PS4, Line 1: Sysmocom s.f.m.c.
> oh ... I (neels) didn't know about the dash, which means that all headers I
Done
PS4, Line 1: Sysmocom s.f.m.c.
> sysmocom awalys lower-case and "-" before s.f.m.c.
Done
Line 28: /* RTP endpoint number */
> does it make sense to have a name inside the fms, and a name here? I would
Done
Line 36: enum gsm48_chan_mode chan_mode;
> I don't think we want to keep an extra pointer to the network around in eve
Done
Line 38: struct gsm_lchan *lchan;
> is this really an integer type, don't we have some enum for this?
Done
PS4, Line 39: c
> could be a bool?
Done
https://gerrit.osmocom.org/#/c/4334/4/src/osmo-bsc/osmo_bsc_bssap.c
File src/osmo-bsc/osmo_bsc_bssap.c:
Line 341: /* NOTE: This is the SCCP-Lite case, since we do not handle
> does this version of osmo-bsc even support SCCPlite anymore? I thought the
When starting to port over to AoIP we agreed on preserving the SCCPLite support. There is not much to it. Since with SCCP-Lite, the MSC controls the MGW, The BSC only has to make the right port allocation on the BTS via RSL.
(Probably SCCPLite is the wrong term here, its more about the behaviour that is applied when SCCPLite is used.)
Line 564: conn->mgcp_ctx = mgcp_assignm_req(msc->network, msc->network->mgw.client, conn, chan_mode, full_rate);
> don't pass a NULL talloc ctx!! We don't want to create rogue root contexts,
Done
Line 582:
> new whitspace error?
Done
Line 767:
> new whitspace error?
Done
Line 768: OSMO_ASSERT(lchan->abis_ip.ass_compl.valid);
> ('== true' is superfluous)
Done
Line 772: LOGP(DMSC, LOGL_DEBUG, "Sending assignment complete message... (id=%i)\n", conn->sccp_con->conn_id);
> sending the assignment complete is an normal event, i.e. LOGL_DEBUG seems a
Done
Line 787: LOGP(DMSC, LOGL_ERROR, "Failed to generate assignment completed message! (id=%i)\n",
> there is no context in this message at all. In a loaded network, we may be
Done
https://gerrit.osmocom.org/#/c/4334/4/src/osmo-bsc/osmo_bsc_main.c
File src/osmo-bsc/osmo_bsc_main.c:
Line 284: LOGP(DNM, LOGL_ERROR, "MGW connect failed at (%s:%u)\n",
> useful to print the connect to *where* has failed (IP/port)
Done
Line 284: LOGP(DNM, LOGL_ERROR, "MGW connect failed at (%s:%u)\n",
> (maybe just "MGCP" without "GW" is better? "MGCPGW" is a term that was used
Done
Line 287: exit(1);
> new whitespace bug
Done
https://gerrit.osmocom.org/#/c/4334/4/src/osmo-bsc/osmo_bsc_mgcp.c
File src/osmo-bsc/osmo_bsc_mgcp.c:
Line 37: #define MGCP_MGW_TIMEOUT 4 /* in seconds */
> in english, it would be "sec", but rather write out "in seconds" to make it
Done
Line 40: #define MGCP_BSS_TIMEOUT_TIMER_NR 2
> ("TIMER_NO" to me reads like "Timer? No!" .. make this "TIMER_NR"?)
Done
Line 46: enum int_cause_code {
> shouldn't below fsm_state_names[] make the comments here unnecessary? At le
Done
Line 71: ST_ASSIGN_PROC,
> "continue", "assignment"
Done
Line 74: ST_ASSIGN_COMPL,
> "assignment"
Done
Line 76: ST_HALT
> nice, we get both the actual state constant name in the log as well as a hu
Done
Line 77: };
> (could omit the comma, nothing should ever follow the end marker)
Done
Line 121: {EV_INIT, "EV_INIT (start state machine (send CRCX for BTS)"},
> (the comment says the identical thing that the function name already does)
Done
Line 122: {EV_ASS_COMPLETE, "EV_ASS_COMPLETE (assignment complete)"},
> this is called from pretty much all the code paths; it might be nice to add
Done
Line 125: {EV_MDCX_BTS_RESP, "EV_MDCX_BTS_RESP (got MDCX reponse for BTS)"},
> If this comment is useful and applies to the entire function, it should go
Done
Line 130:
> (usually it makes sense to assert non-NULL once, early in the code path, in
I am not so sure about this. While developing this the OSMO_ASSERT() helped me a lot. Maybe we can decide to drop some of the OSMO_ASSERT() later, but I would not drop them now.
Line 137: struct osmo_fsm_inst *fi;
> * logging inside fsm's should use LOGPFSM in order to automatically print t
Done
Line 143:
> should this have a timeout?
I think even if it won't change anything, it is logically incorrect. The timeout callback can also call this function. This would create a loop, even if such a situation would never occur because we send an event in the next line.
Line 173: OSMO_ASSERT(mgcp_ctx);
> * all this logging is implemented by the FSM core *and* it is not a NOTICE
Done
Line 185:
> I usually like this notation:
(Note: lindent/indent has problems with that format)
Line 190: mgcp_msg = (struct mgcp_msg) {
> I think this snprintf should move into mgcp_msg_gen() and make mgcp_msg.end
If we do this we will loose the flexibility to support any endpoint naming format. Endpoint names can get quite complex. (e.g. ann/23 at mygateway.whatever.net). See RFC3435 Appendix E. Or we just decide for one format and that our mgcp-client is only compatible with our mgw by definition.
Line 193: MGCP_MSG_PRESENCE_CONN_MODE),
> (would it make sense to add thin API functions like mgcp_msg_set_foo() that
I do not think that we will benefit much from this. The presence field is inspired from the sccp-addresses in libosmo-sigtran. When I remember correctly we do not have anything like that there too.
Line 199: msg = mgcp_msg_gen(mgcp, &mgcp_msg);
> admitted, we have logging in mgcp_client_tx, but would still be nice to do
Done
Line 207: return;
> (is a cast from void* even necessary? I think it's not warned about, is it?
Done
Line 250: /* Callback for ST_ASSIGN_PROC: An mgcp response has been received, proceed
> (also bool according to Harald's earlier comment?)
Done
Line 257: bool full_rate;
> LOGPFSM whenever you have a fsm_inst as context, all over this file.
Done
Line 271: return;
> another NOTICE. Please be more careful when deciding on the log level. Th
Done
Line 282:
> (kind of obsolete comment)
Done
Line 499:
> when we encounter errors in parsing MGCP, shouldn't we always return some k
I am not sure here, but I think this is should be catched by the client (we do not implement resends yet), which should discard the response and resent the original command. The MGW would detect this as a resend and respond with the same response again.
Line 780: .timer_cb = fsm_timeout_cb,
> (I'd drop the "FSM " part, we will see in logging that it is an FSM anyway)
Done
Line 806: fsm_registered = true;
> this is a linear list traversal over all FSMs in the program at each proces
Done
Line 813: snprintf(name, sizeof(name), "MGW_%i", conn->conn_id);
> see my other comment, if we really need the FSM name and another name in th
Done
Line 814: mgcp_ctx->fsm = osmo_fsm_inst_alloc(&fsm_bsc_mgcp, NULL, ctx, LOGL_DEBUG, name);
> this string should also be without spaces, again for ctrl interface.
Probably we should have also some protection that makes it impossible to register names with white-spaces. It was not clear to me that this messes up the control interface. I Just thought that this would be only for logging.
--
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