osmo-bsc[master]: ensure that acc_ramp_init() is only called once

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/.

Pau Espin Pedrol gerrit-no-reply at lists.osmocom.org
Wed Apr 11 08:57:26 UTC 2018


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 <ssperling at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-HasComments: Yes



More information about the gerrit-log mailing list