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=... : 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=... : 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=... : 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.