Attention is currently required from: neels, pespin, fixeria. arehbein has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/30602 )
Change subject: vty: Add check against sensible default value for Ny1 ...................................................................... Patch Set 19: (7 comments) File src/osmo-bsc/bts.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/4a80a88d_79abf804 PS9, Line 1729: struct gsm_lchan *gl = gsm_bts_get_cbch(bts); > According to the specs, T3124 is set depending on the channel type (I assume > 'lchan' stands for 'lin […] Done File src/osmo-bsc/bts.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/8aba729d_6ad38f09 PS18, Line 535: if (!gsm_bts_check_ny1(bts, LOGL_ERROR)) return -EINVAL; > put return always in a separate line. Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/9220b3e8_7eaa269f PS18, Line 1726: const struct gsm_lchan *gl = gsm_bts_get_cbch(bts); > I'm still not understanding why are you taking a CBCH channel here, seems > totally not related. […] I removed the check. https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/e0a4ab5e_c63b3fce PS18, Line 1729: if (gl) { > no need for {} here. Linter should already have warned about it. I used 'lint_diff.sh' from our Docker tests locally (I think that's where I got it from) and didn't get anything. Looking at it again, I guess I have to run it after creating the commit (seems like it runs on the latest commit, not on local changes, which I was assuming). Apart from that, does Jenkins fail if the linter warns? https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/294beef5_38231b59 PS18, Line 1730: T3124 = (gl->type == GSM_LCHAN_SDCCH) ? GSM_T3124_SDCCH : GSM_T3124_OTHER_CH; > this is always going to be false since gl is CBCH. Done https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/6bb07845_431fe884 PS18, Line 1732: /* Take the higher lower bound to be safe */ > you can then move the comment to the "else" line (1731) Done File src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c: https://gerrit.osmocom.org/c/osmo-bsc/+/30602/comment/069ed66a_041df112 PS18, Line 95: gsm_bts_check_ny1(bts, LOGL_NOTICE); > I really see no reason for having different log levels. This is the additional check during runtime (vs. startup), so should this function also exit with an error if the check isn't successful? My thinking was if we don't exit, it shouldn't count as an error message. -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/30602 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: If3f96a6bd4f9ae32b6421de43c1c5a5d64482089 Gerrit-Change-Number: 30602 Gerrit-PatchSet: 19 Gerrit-Owner: arehbein <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-CC: laforge <[email protected]> Gerrit-CC: neels <[email protected]> Gerrit-Attention: neels <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Attention: fixeria <[email protected]> Gerrit-Comment-Date: Wed, 04 Jan 2023 16:00:23 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: arehbein <[email protected]> Comment-In-Reply-To: pespin <[email protected]> Gerrit-MessageType: comment
