Attention is currently required from: csaba.sipos.

fixeria 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 3: Code-Review-1

(3 comments)

File src/osmo-bsc/bts_nokia_site.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/39416/comment/35fc4681_b4938778?usp=... : PS3, Line 1823: if (find_element Could you please explain the idea of using `if` statement without the actual body? I may be missing something, but it looks like the logic would be the same if you remove `if` here and below.

https://gerrit.osmocom.org/c/osmo-bsc/+/39416/comment/bea33940_aa6aeecd?usp=... : PS3, Line 1825: get_object_identity_string(object_id_state[4]), object_id_state[5], get_object_state_string(object_id_state[10])); Please align this line to the opening brace `(` and make sure to not exceed the line length of 120 characters.

https://gerrit.osmocom.org/c/osmo-bsc/+/39416/comment/b5dffafa_a827d9c8?usp=... : PS3, Line 1853: if (find_element Again, what's the idea here? If the element must be present according to the protocol specification, then I would expect a logic like this:

``` if (!find_element(noh->data, len_data, NOKIA_EI_OBJ_ID, object_identity, sizeof(object_identity)) { LOG_BTS(bts, DNM, LOGL_NOTICE, "Missing NOKIA_EI_OBJ_ID\n"); return -EINVAL; } ```

If it's optional, then you should not be accessing `object_identity` and `object_state` unconditionally below, because they may contain garbage.