Attention is currently required from: jolly.

View Change

15 comments:

• File src/libmsc/vgcs_fsm.c:

  • Patch Set #10, Line 123:

    tatic __attribute__((constructor)) void vgcs_bcc_fsm_init(void)
    {
    	OSMO_ASSERT(osmo_fsm_register(&vgcs_bcc_fsm) == 0);
    }
    
    static __attribute__((constructor)) void vgcs_gcc_fsm_init(void)
    {
    	OSMO_ASSERT(osmo_fsm_register(&vgcs_gcc_fsm) == 0);
    }
    
    static __attribute__((constructor)) void vgcs_bss_fsm_init(void)
    {
    	OSMO_ASSERT(osmo_fsm_register(&vgcs_bss_fsm) == 0);
    }
    
    static __attribute__((constructor)) void vgcs_cell_fsm_init(void)
    {
    	OSMO_ASSERT(osmo_fsm_register(&vgcs_cell_fsm) == 0);
    }
    
    static __attribute__((constructor)) void vgcs_mgw_ep_fsm_init(void)
    {
    	OSMO_ASSERT(osmo_fsm_register(&vgcs_mgw_ep_fsm) == 0);
    }
    

    is there a reason to have separate constructor symbols for this? Why not have one consturctor function which has N OSMO_ASSERT statements?

  • Patch Set #10, Line 152: sprintf(string, "%08u", callref);

    yes, 8 decimal digits plus zero termination is shorter than 16 characters, but I'd say as a matter of good practice we should still use snprintf. This makes it easier for somebody else to to audit the code, where every sprintf requires auditing if it's really ok, with snprintf you can just ignore it from manual audit/inspection.

  • Patch Set #10, Line 222: if (with_prio)

    also here I would first build the 32bit value as a stack variable of uint32_t type and use msgb_put_u32() to avoid having to type-cast around and using the third byte of an uint32....

  • Patch Set #10, Line 232: int _ie_invalid(void)

    are those (_add_cause_ie, _add_callref_ie, _msg_too_short, _ie_invalid) really intended to be exported (non-static) functions? IF yes, then their name should be expanded to reflect they relate to ASCI as the name is very geneirc. If not, then they should be static.

  • Patch Set #10, Line 249: e = msgb_put(msg, 1)

    you can first buld the uint8_t as a local variable and then use msgb_put_u8(msg, value), avoiding a pointer to an array and indexing only the 0th element in it.

  • Patch Set #10, Line 265: #if

    not sure why this is commented out? because there are no users yet and it's a static function? IF there is a good reason to do it like this, then add a comment as explanation, please.

  • Patch Set #10, Line 314: payload_len

    as you're decrmenting this variable, I thing I'd call it remaining_len or remaining_payload_len or something like that. payload_len sounds like the total length of the payloda field

  • Patch Set #10, Line 323: [0] + 1;

    I don't have an immediate better solution, but I really think ti is very hard to read/follow and particularly had to say if it is correct, contains some overflows, ....

    You're basically incrementing the ie pointer as you go, but also deducting the payload_len pointer, both by the same amount, causing duplicate sub-expressions like "ie[0] + 1" which all must be consistent.

  • Patch Set #10, Line 337: ie[0] +

    here I think you're trusting ie[0] blindly, without having done much checking on it? probably osmo_mobile_identity_decode() imlpicitly trusts it as it is used as input variable.

  • Patch Set #10, Line 346: ie += 4;

    in each of those blocks we have a magic value (length) that is used three times and which must be consistent. 4 here in this example. I think there must be a way without all those copies of the same value. Like some kind of helper functions, or in the worst case macros?

  • Patch Set #10, Line 391: ie[0] = (da << 3)

    again I'd put the value together in a uint8_t local variable and then msgb_put_u8() it aftrewards. This way we're not de-refernecing the zero'th element in an aray of 1, which makes it harder to read (and one might assume the array might have more elements, ...

  • Patch Set #10, Line 416: if (ie[3] & 0x10) {

    same comments as other decoders above.

  • Patch Set #10, Line 604: osmo_fsm_inst_free(mgw->fi);

    we typically use the goto err_* label construct here to not have to repeat all the various free functions in every case.

  • Patch Set #10, Line 694: return -EINVAL;

    here we return -EINVAL without talloc_free etc?

  • Patch Set #10, Line 1279: int gsm44068_rcv_bcc_gcc(struct msc_a *msc_a, struct gsm_trans *trans, struct msgb *msg)

    this function is quite long. Even the variable list alone fills half a page on soem screen heights. That's usually a sign that it should be pslit up into sub-functions.

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