Attention is currently required from: jolly.
5 comments:
Patchset:
I had a quick look. I only have a few cosmetic hints yet.
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.h presumably has the FSM for handover)
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 public but needs to be called by a unittest, but I see you use this function in transaction.c
File src/libmsc/vgcs_fsm.c:
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(). The reason for this is that it is then easier to grep for an event name in the code when it appears in the log. Just look at the other FSMs (e.g. handover_fsm.c from osmo-bsc)
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?
To view, visit change 33511. To unsubscribe, or for help writing mail filters, visit settings.