Attention is currently required from: csaba.sipos.

Patch set 3:Code-Review -1

View Change

3 comments:

• File src/osmo-bsc/bts_nokia_site.c:

  • Patch Set #3, 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.

  • Patch Set #3, 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.

  • Patch Set #3, 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.

To view, visit change 39416. To unsubscribe, or for help writing mail filters, visit settings.