Bug #4152
closedosmo_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
100%
Description
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 https://gerrit.osmocom.org/c/osmo-sgsn/+/15214
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?
Updated by neels over 4 years ago
I just noticed this patch merged recently:
https://git.osmocom.org/libosmocore/commit/?id=cfd6ac646224c59fc3f46c9e4ef9f7fecb6c1407
commit cfd6ac646224c59fc3f46c9e4ef9f7fecb6c1407
Author: Harald Welte <laforge@gnumonks.org>
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 https://gerrit.osmocom.org/c/libosmocore/+/12717/1 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.
Updated by neels over 4 years ago
- Status changed from New to In Progress
- % Done changed from 0 to 90
Updated by neels over 4 years ago
- Status changed from In Progress to Resolved
- % Done changed from 90 to 100
