Patch Set 3: Code-Review-1

(5 comments)

https://gerrit.osmocom.org/#/c/7784/3/src/libbsc/acc_ramp.c
File src/libbsc/acc_ramp.c:

Line 182:                       if (trx_is_usable(trx)) /* cross-check with 
operational state */
As discussed a few mins ago, when coming from S_NM_STATECHG_OPER signal in 
abis_nm_rx_statechg_rep, it can be that both operational and administrative 
status change at the same time (same call to this function).

Case: admin LOCKED->UNLOCKED + oper DISABLED->ENABLED, we are not enabling 
ramping here because trx_is_usable() probably checks the current/old status 
which is still DISABLED.


Line 190:                       break;
we can probably removed this break to print an error too, unless NULL is really 
a valid state from the spec point of view (one to which we can change to).


Line 201:                       if (trx->mo.nm_state.administrative == 
NM_STATE_UNLOCKED)
Again, we need to check after the new one as it could have be different than 
the old/current one.


Line 208:                       break;
same, remove break?


Line 222:       if (trigger_ramping && 
!osmo_timer_pending(&acc_ramp->step_timer))
do we really need this osmo_timer_pending check here? This should never happen 
right? better move it inside the if() and add an OSMO_ASSERT() with it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3291439655598fb5ddc891a3e4cc35b0bad250f
Gerrit-PatchSet: 3
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Owner: Stefan Sperling <ssperl...@sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pau Espin Pedrol <pes...@sysmocom.de>
Gerrit-HasComments: Yes

Reply via email to