[PATCH] Refactor MNCC code

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/OpenBSC@lists.osmocom.org/.

Harald Welte laforge at gnumonks.org
Mon Apr 25 13:03:15 UTC 2016


Hi Max,

On Mon, Apr 25, 2016 at 12:22:17PM +0200, msuraev at 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
-- 
- Harald Welte <laforge at gnumonks.org>           http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
                                                  (ETSI EN 300 175-7 Ch. A6)



More information about the OpenBSC mailing list