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.orgHarald Welte has posted comments on this change. ( https://gerrit.osmocom.org/9671 )
Change subject: large refactoring: use FSMs for lchans; add inter-BSC HO
......................................................................
Patch Set 6:
(26 comments)
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/bsc_vty.c
File src/osmo-bsc/bsc_vty.c:
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/bsc_vty.c@a1240
PS6, Line 1240:
was it moved to libosmgsm?
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/bsc_vty.c@1727
PS6, Line 1727: DEFUN(handover_ext_cgi,
we should probably have a FIXME here
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/gsm_data.c
File src/osmo-bsc/gsm_data.c:
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/gsm_data.c@1536
PS6, Line 1536: static enum gsm0808_permitted_speech audio_support_to_gsm88(const struct gsm_audio_support *audio)
static helper functions here and below also don't seem gsm_data.[ch] related and should be moved wherever their sole caller is moved.
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/gsm_data.c@1653
PS6, Line 1653: int bsc_match_codec_pref(enum gsm48_chan_mode *chan_mode,
this is also probably not really right here in gsm_data.c. IT doesn't work with lchan/ts/trx/bts type structures but purely on other data.
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/gsm_data.c@1708
PS6, Line 1708: bool sockaddr_to_str_and_uint(char *rtp_addr, size_t rtp_addr_len, uint16_t *rtp_port,
this is not something we'd traditionally have in gsm_data.c - also it seems like it's a rather generic function. candidate for libosmocore?
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/gsm_data.c@1715
PS6, Line 1715: osmo_ntohs
we normally only use osmo_ntohs() in situations where it's not IP/socket related and hence htons() may not be available. If you're working on a port, ntohs/htons/etc. are available. But anyway, we can probably ignore it.
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_decision.c
File src/osmo-bsc/handover_decision.c:
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_decision.c@203
PS6, Line 203: {
style-wise, I would generally prefer the variable declaration at the top of the function, rather than those separate blocks. This applies to all of the code. I don't think we've been doing this in osmocom so far (I don't recall it?). Why now?
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_decision_2.c
File src/osmo-bsc/handover_decision_2.c:
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_decision_2.c@722
PS6, Line 722: {
why that separate block?
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_fsm.c
File src/osmo-bsc/handover_fsm.c:
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_fsm.c@141
PS6, Line 141: #define GET_CONN() \
see previous comment about hiding variable declarations in macros
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_fsm.c@249
PS6, Line 249: static bool g_initialized = false;
see previous comment about avoiding this automatic registration
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_fsm.c@644
PS6, Line 644: return result_counter_INTER_BSC_HO_MT(result);
two return statements in one case?
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_logic.c
File src/osmo-bsc/handover_logic.c:
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/handover_logic.c@99
PS6, Line 99: int handover_count(struct gsm_bts *bts, int ho_scopes)
might make sense to prefix symbol name with bts_ ?
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/lchan_fsm.c
File src/osmo-bsc/lchan_fsm.c:
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/lchan_fsm.c@41
PS6, Line 41: #define GET_LCHAN() \
see previous comment about hiding variable declaration in macros
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/lchan_fsm.c@326
PS6, Line 326: static bool g_initialized = false;
see previous comment about automatic vs. explicit registration
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/lchan_select.c
File src/osmo-bsc/lchan_select.c:
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/lchan_select.c@1
PS6, Line 1: /* Select a suitable lchan from a given cell.
I guess this is code we traditionally would have had in chan_alloc.c? Any particular rationale to have a new file and not use chan_alloc.c?
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/mgw_endpoint_fsm.c
File src/osmo-bsc/mgw_endpoint_fsm.c:
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/mgw_endpoint_fsm.c@188
PS6, Line 188: if (!g_initialized) {
see previous code about automatic vs. explicit registration
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/mgw_endpoint_fsm.c@196
PS6, Line 196: #define GET_MGWEP() \
see previous comment about hiding vatiable declarations in macros
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/mgw_endpoint_fsm.c@372
PS6, Line 372: struct state_timeout mgwep_fsm_timeouts[32] = {
do we need those non-static? can they be const?
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/mgw_endpoint_fsm.c@682
PS6, Line 682: int mgwep_fsm_timer_cb(struct osmo_fsm_inst *fi)
do we need to export this symbol? is it used by anyting but the timer_cb below?
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/osmo_bsc_api.c
File src/osmo-bsc/osmo_bsc_api.c:
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/osmo_bsc_api.c@189
PS6, Line 189: no-one is using this
this is odd? any service we don't know / support should be rejected. how does the code do this?
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/osmo_bsc_bssap.c
File src/osmo-bsc/osmo_bsc_bssap.c:
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/osmo_bsc_bssap.c@757
PS6, Line 757: LOGP(DMSC, LOGL_ERROR, "Received Handover Command, but no handover was requested");
might make sense to log "conn" context here?
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/osmo_bsc_bssap.c@767
PS6, Line 767: LOGP(DMSC, LOGL_ERROR, "Mandatory IE not present: Layer 3 Information\n");
might make sense to log "conn" context here?
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/osmo_bsc_bssap.c@772
PS6, Line 772: {
see previous comment about those extra blocks rather than declaring the variable at the top of the function.
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/timeslot_fsm.c
File src/osmo-bsc/timeslot_fsm.c:
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/timeslot_fsm.c@40
PS6, Line 40: #define GET_TS(fi, TS) \
see previous comments about hiding variable declarations in macros
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/timeslot_fsm.c@44
PS6, Line 44: #define GET_BTS_TS(fi, BTS, TS) \
see previous comments about hiding variable declarations in macros
https://gerrit.osmocom.org/#/c/9671/6/src/osmo-bsc/timeslot_fsm.c@57
PS6, Line 57: if (!g_initialized) {
see previous comments about explicit registration
--
To view, visit https://gerrit.osmocom.org/9671
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I82e3f918295daa83274a4cf803f046979f284366
Gerrit-Change-Number: 9671
Gerrit-PatchSet: 6
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-CC: Harald Welte <laforge at gnumonks.org>
Gerrit-Comment-Date: Mon, 25 Jun 2018 19:11:49 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20180625/945f6460/attachment.htm>