Patch Set 1: Code-Review-1

(4 comments)

https://gerrit.osmocom.org/#/c/7732/1/src/libbsc/acc_ramp.c
File src/libbsc/acc_ramp.c:

Line 162:       struct gsm_nm_state *new_state = nsd->new_state;
You are deferring an expected nm_statechg_signal_data pointer before knowing if 
that's the correct signal (check for S_NM_STATECHG_ADM done below).


Line 166:       if (subsys != SS_NM)
Is this really needed? we are only attaching the cb to the SS_NM subsystem 
right?

either move to an ASSERT or drop it (preferred imho).


Line 194:               break;
What about NM_STATE_SHUTDOWN and NM_STATE_NULL? Would be nice at least adding 
an explicit case for NM_STATE_SHUTDOWN, and maybe ASSERT on default.


https://gerrit.osmocom.org/#/c/7732/1/src/libbsc/bsc_vty.c
File src/libbsc/bsc_vty.c:

Line 3280:      /* ACC ramping takes effect when the BTS reconnects or RF 
administrative state changes to 'unlocked'. */
You can take the chance to improve the "BTS reconnects" part by adding 
reference to RSL link.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4124f1da3dadec003de45c1da8435506ee8f0a34
Gerrit-PatchSet: 1
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Owner: Stefan Sperling <ssperl...@sysmocom.de>
Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pau Espin Pedrol <pes...@sysmocom.de>
Gerrit-HasComments: Yes

Reply via email to