Re: patch 36: application interface

Dear Andreas, I have finally found some time to review the MNCC patches in detail. There were quite a bit of details that I wanted to change, mostly about the code and data structure. The logic and the idea of the interface is still the same. You can have a look at my code in the 'mncc-harald' branch of the new git tree. It's probably easiest if you look at the new mncc.h file. Instead of a flat list of fields, I decided for a bit more structure. This results in a situation where most of the ancode_foo() and decode_foo() functions now only take a pointer to a sub-structure, rather than a long list of arguments. I think this is much better to understand - and also reduces the number of lines that are either long or have to be broken in many more lines, resulting in less code visibility on one screen page. Furthermore, I've introduced some additional functions like mncc_set_cause() for constantly repeated sequences of code. Changing mncc.h obviously breaks interoperability with your lcr, and I'm honestly sorry about that. I'm not planning any intentional data structure changes any time soon. I've tested the standalone bsc_hack of the mncc-harald branch for quite some time on a nanobts1800, it works fine. I have some more thoughts on improving MNCC, but those changes would only affect the implementation, not the interface. I don't like the many explicit lists of "if (TLVP_PRESENT(&tp, ...)) decode_something" sequences. If we had something like a table of ======== struct parser_def { u_int8_t tlv_tag; u_int32_t field_flag; unsigned int member_offset; (int) (decode_function)(void *, u_int8_t *lv); }; struct parser_def parser_defs[] = { { GSM48_IE_SS_VERS, MNCC_F_SSVERSION, offset_of(ssversion_info, struct gsm_mncc), &decode_ssversion }, ... }; ======== then we could do something like have a for-loop and then only one statement like this: ======== struct parser_def *pd; if (TLVP_PRESENT(&tp, pd->tlv_tag)) { connect.fields |= pd->field_tag; pd->decode_function(&connect + pd->member_offset, TLVP_VAL(&tp, pd->tlv_tag)-1); } ======== It would replace a lot of explicit open-coded code (38 incarnations!) with a more data-driven model. Regards, -- - Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)