Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/11654 )
Change subject: paging: Make default T3113 timeout dynamic ...................................................................... Patch Set 1: Code-Review-1 (4 comments) https://gerrit.osmocom.org/#/c/11654/1/src/osmo-bsc/paging.c File src/osmo-bsc/paging.c: https://gerrit.osmocom.org/#/c/11654/1/src/osmo-bsc/paging.c@300 PS1, Line 300: return d->val; what if the user wants a fixed value == the default val? maybe a vty option like 'dynamic-T3113' / 'no dynamic-T3113' would be better https://gerrit.osmocom.org/#/c/11654/1/src/osmo-bsc/paging.c@312 PS1, Line 312: to = (to_us + 999999) / 1000000 + d->default_val; maybe using user supplied d->val would be better? https://gerrit.osmocom.org/#/c/11654/1/src/osmo-bsc/paging.c@313 PS1, Line 313: LOGP(DPAG, LOGL_DEBUG, "(bts=%d) Paging request calculated T3113 expires in %u seconds\n", sprinkle colons: "Paging request: calculated T3113: expires in %u seconds" (if you agree with above vty config option, maybe s/calculated/dynamic) Actually, I would rather log that below, where the timer actually gets fired. It's a common pattern of utility functions like this, maybe they get used for just quickly taking a look or somesuch, and then they log as if it was the real thing. https://gerrit.osmocom.org/#/c/11654/1/src/osmo-bsc/paging.c@348 PS1, Line 348: int t3113_timeout_s = calculate_timer_3113(bts); I know, but in our coding style we declare vars at top. I'd drop that rule, but here we are. (rather stick to unsigned int? Or instead int above? well whatever) -- To view, visit https://gerrit.osmocom.org/11654 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: I4fb2969b690151415038631fb6ad059aa6835c7f Gerrit-Change-Number: 11654 Gerrit-PatchSet: 1 Gerrit-Owner: Pau Espin Pedrol <[email protected]> Gerrit-Reviewer: Harald Welte <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Comment-Date: Thu, 08 Nov 2018 17:29:17 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
