Patch Set 3: Code-Review-1 (5 comments)
sorry to again convolute the patch with criticism of prior API state ... but here goes. What do you think? https://gerrit.osmocom.org/#/c/7574/3/src/libbsc/bts_ipaccess_nanobts.c File src/libbsc/bts_ipaccess_nanobts.c: Line 53: static char *_get_rsl_status(const struct gsm_bts *bts, bool is_oml_status) Cosmetically, I'd prefer a _get_rsl_status() without 'is_oml_status'. Then get_oml_status() uses the clean _get_rsl_status() and itself decides whether to return "connected" or "degraded". Line 67: return all_trx_rsl_connected_unlocked(bts) ? "connected" : "degraded"; The proper way to do this would be to define an enum of possible replies for get_oml_status() in gsm_data.h, accompanied by a value_string[] to convert to string. In case you decide to stay with strings, then instead of repeating the string, I'd prefer const definitions. That also guarantees that it's possible to do something like: /* static? */ const char *RSL_STR_UP = "connected"; ... if (_get_rsl_status(...) == RSL_STR_UP) return OML_STR_UP; return OML_STR_DEGRADED; (i.e. no need to strcmp() for string constants, comparing the addresses is enough. Would also work now, but only if you don't have typos anywhere.) And in case you decide to stay with strings, should return 'const char*' everywhere, not 'char*'. (and fix the API definition of oml_get_status() too; returning char* was a mistake from the start). Line 70: static char *get_rsl_status(const struct gsm_bts *bts) return const char * (or enum value) Line 75: static char *get_oml_status(const struct gsm_bts *bts) so we need to change this to const char*, too. Line 90: .rsl_status = &get_rsl_status, and change the gsm_bts_model API. I'd do that, because it lives in osmo-bsc only anyway, and passing const char* around as char* is mad. -- To view, visit https://gerrit.osmocom.org/7574 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4bd0821503fc4407dbee8cb489675c19384de5cb Gerrit-PatchSet: 3 Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Owner: Stefan Sperling <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Stefan Sperling <[email protected]> Gerrit-HasComments: Yes
