Hi Max,
On Mon, Apr 25, 2016 at 12:22:17PM +0200, msuraev@sysmocom.de wrote:
- Explicitly use gsm_chan_t enum when checking for full rate channels.
- Consistently use enums instead of (u)int, char etc.
Is this supposed to be a pure cleanup patch (not changing behavior) or a bug fix (changing behavior)? This is currently not clear to me
I'm not particularly happy that in many cases when there is a bug to fix, you first seem to restructure/cleanup the code and then address the bug. This adds the risk of introducing behavioral changes in the first step (particularly with lack of good test coverage), and it also means we're focussing less on the relevant topic.
Note: because actual enum representation is up to compiler this might change the size of gsm_mncc struct so the MNCC protocol version is bumped.
I think this kind of change just creates more effort for everyone and I don't really see much benefit of it. You would have to merge related changes not only to the NITB but to all its users (lcr, mncc_python, osmo-sip-connector, ...) at the same time. Have you changed all the respective users and re-tested them? Do you think it is a good use of our time to do so for a merely stylistic improvement?
In general I think it is a bad idea to have enum's in structures that describe an external interface / protocol betweeo two programs, exactly for the reason you stated. Alignment differences can be easily avoided with __attribute__ ((packed)), but enums have the inherent problem of '-fshort-enums', which might be default on some platform. and then you add a new enum value, and your structure size changes again.
Regards, Harald