Attention is currently required from: csaba.sipos, fixeria, laforge.

View Change

3 comments:

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

  • Patch Set #11, 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]));

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

  • Patch Set #11, 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 change 39416. To unsubscribe, or for help writing mail filters, visit settings.