Patch Set 1:

(5 comments)

https://gerrit.osmocom.org/#/c/7723/1/include/osmocom/bsc/acc_ramp.h
File include/osmocom/bsc/acc_ramp.h:

Line 145: void acc_ramp_disable(struct acc_ramp *acc_ramp);
I think it makes more sense to have it as one function, no need for two:
acc_ramp_enabled_set(acc_ramp, bool enabled)

This way it's easier to grep in code with 1 token to see where it is enabled or 
not,but not really required of course.
In any case, the enable/disable could then be implemented as defines or static 
inline on top of acc_ramp_enabled_set, but I still prefer having only 1 
function and use it everywhere.

Even better, it can be set as a static inline as I see it's already done for 
similar stuff in here.


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

Line 34: static bool acc_is_enabled(struct gsm_bts *bts, unsigned int acc)
Not directly related to this patch but:
Use of same terminology (acc enabled/disabled) for different structs containing 
different information (permanent vs temporary due to ramping activation) is 
quite confusing.


Line 120:       if (acc_ramp->step_size == ACC_RAMP_STEP_SIZE_MAX) {
I think we can drop this if and its contents completely in a follow up patch. 
It is not needed, it's handled fine by the generic code path, so no need to add 
code complexity for a really corner case scenario (since usually in that case 
you would actually just disable ramping).


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

Line 1918:              acc_ramp_init(&bts->acc_ramp, bts);
I think it really makes sense to move _init inside gsm_bts_alloc_register now, 
as it's only used at this time.


Line 3278:      acc_ramp_enable(&bts->acc_ramp);
You want to dothe same in here (abort + set_system_infos) as you do in 
no_acc_ramping below. Imagine for instance if a vty cfg file contains 
"access-control-class-ramping" twice, or if someone enters 
"access-control-class-ramping" in the vty while a ramping is still in progress.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia25bff85d9e5c277da76bffa11d31972e9fdc323
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