Bug #4152

osmo_tdef_get() has API doc that encourages to pass -1 to an unsigned long argument, and promises certain behavior in case of -1 being passed

Added by neels 2 months ago. Updated about 1 month ago.

Target version:
Start date:
Due date:
% Done:


Spec Reference:


unsigned long osmo_tdef_get(const struct osmo_tdef *tdefs, int T, enum osmo_tdef_unit as_unit, unsigned long val_if_not_present)

says in the API doc

 *      val = osmo_tdef_get(global_T_defs, 99, OSMO_TDEF_S, -1); // not defined, program aborts!

That is neither implemented nor possible with an unsigned arg.
How did that get past our review...

I messed up there, how should we fix it?

Given that osmo-bsc has code passing -1, maybe we should make the argument signed?
And then we should indeed add the advertised behavior, i.e. a program abort if no timer is defined?

I'm pretty sure that was also actually implemented in an earlier version, but must have gotten lost in the changes and migration to libosmocore... :/

Noticed this when reading
And noticed that osmo-bsc master passes -1 in seven distinct places.

Another way would be to fix the API doc and not abort the program on missing timer definitions,
but in fact I think that's a pretty nice API feature to have?


#1 Updated by neels 2 months ago

I just noticed this patch merged recently:

commit cfd6ac646224c59fc3f46c9e4ef9f7fecb6c1407
Author: Harald Welte <>
Date:   Sun Jul 21 09:22:19 2019 +0200

    tdef: remove bogus OSMO_ASSERT(unsigned long >= 0)

    Change-Id: I7a544d2d43b83135def296674f777e48fe5fd80a
    Closes: CID#190866

diff --git a/src/tdef.c b/src/tdef.c
index 9d5d7363..3cfb17c0 100644
--- a/src/tdef.c
+++ b/src/tdef.c
@@ -187,7 +187,6 @@ unsigned long osmo_tdef_get(const struct osmo_tdef *tdefs, int T, enum osmo_tdef
        const struct osmo_tdef *t = osmo_tdef_get_entry((struct osmo_tdef*)tdefs, T);
        if (!t) {
-               OSMO_ASSERT(val_if_not_present >= 0);
                return val_if_not_present;
        return osmo_tdef_round(t->val, t->unit, as_unit);

And looking at the libosmocore patch the 'unsigned long' arg had crept in already in the first patch set.
I believe the better way would be to re-add the assertion and change the argument to a long, to again match the API doc and what osmo-bsc expects to happen.


#2 Updated by neels 2 months ago

  • Status changed from New to In Progress
  • % Done changed from 0 to 90

#3 Updated by neels 2 months ago

  • Assignee set to neels

#4 Updated by pespin about 1 month ago

neels I think this ticket can be closed.

#5 Updated by neels about 1 month ago

  • Status changed from In Progress to Resolved
  • % Done changed from 90 to 100

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)