Attention is currently required from: csaba.sipos, fixeria, laforge. pespin has posted comments on this change by csaba.sipos. ( https://gerrit.osmocom.org/c/osmo-bsc/+/39416?usp=email )
Change subject: nokia_site: Add object_identity, object_state and object_identity_state attributes ...................................................................... Patch Set 11: (3 comments) File src/osmo-bsc/bts_nokia_site.c: https://gerrit.osmocom.org/c/osmo-bsc/+/39416/comment/f7145f41_536483a2?usp=email : PS11, Line 1828: get_object_identity_string(object_id_state[4]), object_id_state[5], get_object_state_string(object_id_state[10])); Please, as mentioned, keep lines belows 120chars, otherwise it's a mess. This is far more readable: LOG_BTS(bts, DNM, LOGL_NOTICE, "State changed: %s=%d, %s\n", get_object_identity_string(object_id_state[4]), object_id_state[5], get_object_state_string(object_id_state[10])); https://gerrit.osmocom.org/c/osmo-bsc/+/39416/comment/8695f6f3_0ed66c31?usp=email : PS11, Line 1859: find_element(noh->data, len_data, NOKIA_EI_OBJ_STATE, &object_state, sizeof(object_state)) Again, all the indentation choices you are selecting here make it impossible to follow. btw, did you think about adding a macro or a static inline function helper to make all this much more readable? Something like: #define FIND_ELEM(data, data_len, ei, var) (find_element(data, data_len, ei, &var, sizeof(var)) == sizeof(var)) if (!FIND_ELEM(noh->data, len_data, NOKIA_EI_OBJ_ID, object_identity) || !FIND_ELEM(noh->data, len_data, NOKIA_EI_OBJ_STATE, object_state) { if (!FIND_ELEM(noh->data, len_data, NOKIA_EI_OBJ_ID_STATE, object_id_state) { LOG_BTS(bts, DNM, LOGL_NOTICE, "Missing NOKIA_EI_OBJ_ID & NOKIA_EI_OBJ_STATE or NOKIA_EI_OBJ_ID_STATE\n"); return -EINVAL; } object_identity[1] = object_id_state[4]; object_identity[2] = object_id_state[5]; object_state = object_id_state[10]; } See how with a bit more thinking all this becomes much more readable imho. You could even probably have all that in a separate function find_identity() and find_state(), which would check for both IEs internally. https://gerrit.osmocom.org/c/osmo-bsc/+/39416/comment/e44f4877_37141021?usp=email : PS11, Line 1863: object_identity[1] = object_id_state[4]; so basically object_identity[0] is left uninitialized in this code path? same for object_identity[3]. Are those nose used? Then let's better have uint8_t identity and a uint8_t severity instead of half initializing some array. -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/39416?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: Id9f819b0649ba3c247db72d7d738e49c72388dc3 Gerrit-Change-Number: 39416 Gerrit-PatchSet: 11 Gerrit-Owner: csaba.sipos <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <[email protected]> Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-Attention: csaba.sipos <[email protected]> Gerrit-Attention: laforge <[email protected]> Gerrit-Attention: fixeria <[email protected]> Gerrit-Comment-Date: Wed, 05 Feb 2025 20:06:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No
