Attention is currently required from: laforge, dexter.
19 comments:
File include/osmocom/msc/Makefile.am:
Patch Set #10, Line 28: vgcs_fsm.h \
maybe rename this to "msc_vgcs" (not critical, but i see all the msc_ prefixes and msc_ho. […]
Done
File include/osmocom/msc/vgcs_fsm.h:
Patch Set #10, Line 210: void _gsm44068_bcc_gcc_trans_free(struct gsm_trans *trans);
why does this function begin with an underscore? To me this looks like a function that is not really […]
This is the handler that is called if a GCC/BCC transaction is freed. I used the same name style as for CC/SMS/SS transaction types. You are right, it is confusing. I removed that dash.
File src/libmsc/vgcs_fsm.c:
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 functi […]
Done
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 o […]
Done
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_ […]
Done
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 expor […]
Done
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 […]
Done
not sure why this is commented out? because there are no users yet and it's a static function? IF th […]
Done
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 s […]
Done
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 par […]
Done
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_m […]
I forgot ti check the return code of tlv_parse. Also I added check for each variable length IE fitting in the message.
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 co […]
I replaced this "4" by the size of the IE payload. A variable "ie_len" holds the length and is then used instead of repeating the integer.
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. […]
Done
Patch Set #10, Line 416: if (ie[3] & 0x10) {
same comments as other decoders above.
If you mean to check that the IE fits into the msg, its done.
Patch Set #10, Line 559: { VGCS_GCC_EV_TIMEOUT, "Timeout due to inactiviy" },
It probably makes more sense to stringify the constant names using OSMO_VALUE_STRING(). […]
Done
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 func […]
Done
Patch Set #10, Line 694: return -EINVAL;
here we return -EINVAL without talloc_free etc?
This will be done within the handling of the CLEAR event. I just added a comment that states that.
Patch Set #10, Line 951: #define connect_option false
maybe use capital letters, so that it is immediately clear, that this is a define constant?
Done
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. […]
Done
To view, visit change 33511. To unsubscribe, or for help writing mail filters, visit settings.