Project

General

Profile

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 3 days ago. Updated 3 days ago.

Status:
In Progress
Priority:
High
Assignee:
Category:
-
Target version:
-
Start date:
08/15/2019
Due date:
% Done:

90%

Spec Reference:

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?

History

#1 Updated by neels 3 days 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.

See https://gerrit.osmocom.org/c/libosmocore/+/15218

#2 Updated by neels 3 days ago

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

#3 Updated by neels 3 days ago

  • Assignee set to neels

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 48.8 MB)