22 Jan
2019
22 Jan
'19
2 a.m.
I think we all agree that what happened with the msgb_wrap_with_TL() is a prime
example about how absolutely *not* to do things.
This makes me want to rewrite libosmocore history.
After all the naming / API duplication mistakes around msgb_wrap_with_TL(), the
final state I am trying to finish my day to is that the osmo-msc and openbsc
master builds remain broken, while patches to fix that are idling on gerrit
with a commit log summary that mentions neither that they fix the build nor
that they are related to msgb_wrap_with_TL().
All this happens with a new release tag on libosmocore, which was tagged to
include a commit that is supposed to fix things instead of break them...
Let's try to avoid this kind of series of events in the future.
* Separate function definitions must not have identical names.
* This applies both to the master HEADs as well as throughout entire API history.
* Especially functions moved to another source tree *must* change their name.
* Such API changes in libosmocore should be tested with "all" depending
programs.
* API fixes in libosmocore should ideally build with both past and future
versions of depending source trees, i.e. should not need a matching patch in
the other source trees.
* When a commit breaks the master build in depending programs,
* clearly mark that so in the commit log and/or gerrit comments, so it is not
merged on its own by accident.
* the author should not go away until either none or both of them are merged.
* Similarly obvious, avoid pouring water on burning oil.
Sure, we all make mistakes every now and then.
In this case we made a few more :P
I spent time now to merge fixes to the master builds instead of going to
sleep... (I hope I'm not adding mistakes to the minefield, though.)
Not sure what to do about osmocom-bb. It has lots of implicit function
definition warnings, only one of them is:
gsm480_ss.c:441:3: warning: implicit declaration of function ‘msgb_wrap_with_TL’
~N
22 Jan
22 Jan
2:04 a.m.
It looks like we need a 1.3.1 tag in osmo-msc,
osmo-msc 1.3.0 won't build with libosmocore 1.0.1 :/
~N
3:36 p.m.
Hi Neels,
On Tue, Jan 22, 2019 at 03:00:31AM +0100, Neels Hofmeyr wrote:
I think we all agree that what happened with the
msgb_wrap_with_TL() is a prime
example about how absolutely *not* to do things.
This makes me want to rewrite libosmocore history.
I had the same feeling yesterday. To clarify "rewrite libosmocore git history"
is
probably what you hinted here :)
Let's try to avoid this kind of series of events
in the future.
* Separate function definitions must not have identical names.
* This applies both to the master HEADs as well as throughout entire API history.
* Especially functions moved to another source tree *must* change their name.
This is generally what we do. And "generally", we prefix them with osmo_ in
the
libraries, and don't permit the osmo_ prefix for symbols in applications. However,
the msgb_ code pre-dates the osmo_ prefix - it even predates the name Osmocom as
the project name.
So we have some legacy prefixes that are "reserved" for use in libosmo*. Those
are
bitvec_
gsmtap_
log_
msgb_
rate_ctr_
abis_nm_
gprs_cipher_
gsm0341_
gsm0480_
gsm0502_
gsm0503_
gsm0808_
gsm29118_
gsm0858_
gsm340_
gsm411_
gsm414_
gsm48_
gsm610_
gsm620_
gsm690_
gsm_7bit_
lapd_
lapdm_
milenage_
rsl_
rxlev_
tlv_
ipa_ccm
bssgp_
gprs_ns_
gprs_nsvc_
btsctx_
osim_
vty_
vector_
telnet_
config_
cmd_
buffer_
Unfortunately a lot of them are rather generic, so it's hard to avoid, at least in
absence
of any automatic tests for it. It's a separate discussion whether we should e.g. stop
to
export los of libosmovty internals (buffer, vector, cmd) - and if we should prefix all
symbols in libosmo* with osmo_* providing the older names only as backwards compatibility
layer with possibly weak symbols to be able to migrate to a cleaner namespace at least at
some point in the future.
Furthermore, there is one factors that made this particular instance even more
problematic:
The respective functions were inline functions. Otherwise we could have simply turned
the library symbol into a weak symbol, making any application implementation supersede
the library one.
* Such API changes in libosmocore should be tested
with "all" depending programs.
ACK. So what's needed is some kind of build job that continuously builds old
applications
against modern libosmo*. This could be run at least once per day/night, so we get
notified
if we introduce any such breakage and can fix libosmo* shortly to keep the incompatiblity
something that appeared for a few days in the history, at maximum.
* API fixes in libosmocore should ideally build with
both past and future
versions of depending source trees, i.e. should not need a matching patch in
the other source trees.
That goes without saying. It just wasn't possible in this case.
Regards,
Harald
--
- 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)
23 Jan
23 Jan
1:25 a.m.
On Tue, Jan 22, 2019 at 04:36:19PM +0100, Harald Welte wrote:
So we have some legacy prefixes that are
"reserved" for use in libosmo*. Those are
bitvec_
gsmtap_
log_
msgb_
rate_ctr_
abis_nm_
gprs_cipher_
gsm0341_
gsm0480_
gsm0502_
gsm0503_
gsm0808_
gsm29118_
gsm0858_
gsm340_
gsm411_
gsm414_
gsm48_
gsm610_
gsm620_
gsm690_
gsm_7bit_
lapd_
lapdm_
milenage_
rsl_
rxlev_
tlv_
ipa_ccm
bssgp_
gprs_ns_
gprs_nsvc_
btsctx_
osim_
vty_
vector_
telnet_
config_
cmd_
buffer_
yikes, quite a few more than I would have thought!
BTW, in osmo-msc we still have a number of gsm48_ and other "reserved" prefixes
as well, for example gsm48_rx_mm_serv_req(). As I'm refactoring for the
inter-MSC split I should be able to get rid of them.
~N
1981
days inactive
1982
days old
3 comments
2 participants
participants (2)
-
Harald Welte -
Neels Hofmeyr