5 Feb
2025
5 Feb
'25
9:06 p.m.
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.
--
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 <metro4(a)freemail.hu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: csaba.sipos <metro4(a)freemail.hu>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 05 Feb 2025 20:06:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No