Change in ...libosmocore[master]: osmo_tdef_get(): allow passing -1 as default timeout
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15218 ) Change subject: osmo_tdef_get(): allow passing -1 as default timeout .. Patch Set 2: (1 comment) https://gerrit.osmocom.org/#/c/15218/2/src/tdef.c File src/tdef.c: https://gerrit.osmocom.org/#/c/15218/2/src/tdef.c@186 PS2, Line 186: osmo_tdef_get > Alternatively, you could add another function without that optional > parameter, so it would simply cr […] quite possible, but osmo-bsc already uses the API as it was intended, so I'd rather just fix it here... -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15218 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ie61c3c85069916336e6dbd91a2c16f7634816417 Gerrit-Change-Number: 15218 Gerrit-PatchSet: 2 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-Comment-Date: Mon, 19 Aug 2019 20:09:18 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria Gerrit-MessageType: comment
Change in ...libosmocore[master]: osmo_tdef_get(): allow passing -1 as default timeout
neels has submitted this change and it was merged. (
https://gerrit.osmocom.org/c/libosmocore/+/15218 )
Change subject: osmo_tdef_get(): allow passing -1 as default timeout
..
osmo_tdef_get(): allow passing -1 as default timeout
The intention of osmo_tdef_get()'s val_if_not_present argument was to return a
default timeout, or to optionally abort the program for missing timer
definitions if the default timeout is < 0. This was the case in the original
implementation of this API in osmo-bsc, but in the migration to libosmocore,
the argument was by accident changed to an unsigned type. In consequence, the
assertion in the implementation that was intended to abort the program seemed
bogus to coverity, and was fixed by removal in
I7a544d2d43b83135def296674f777e48fe5fd80a -- the wrong direction, as is obvious
from the API doc for osmo_tdef_get().
Note that osmo-bsc master passes -1 in various places and expects the
program-abort behavior that was missing from the libosmocore implementation.
Change the val_if_not_present argument to a signed type, and revert removal of
the assertion, so that passing -1 has the effect described in the API doc:
program abort on missing timer definition.
This bug was not detected because it is hard to write tests that expect a
program abort to happen, hence no tests for this API feature exist.
Related: OS#4152
Change-Id: Ie61c3c85069916336e6dbd91a2c16f7634816417
---
M TODO-RELEASE
M include/osmocom/core/tdef.h
M src/tdef.c
3 files changed, 4 insertions(+), 2 deletions(-)
Approvals:
fixeria: Looks good to me, but someone else must approve
pespin: Looks good to me, approved
Jenkins Builder: Verified
diff --git a/TODO-RELEASE b/TODO-RELEASE
index 8ccfa49..665ecf7 100644
--- a/TODO-RELEASE
+++ b/TODO-RELEASE
@@ -7,3 +7,4 @@
# If any interfaces have been added since the last public release: c:r:a + 1.
# If any interfaces have been removed or changed since the last public
release: c:r:0.
#library whatdescription / commit summary line
+core osmo_tdef_get() change val_if_not_present arg from
unsigned long to long to allow passing -1
diff --git a/include/osmocom/core/tdef.h b/include/osmocom/core/tdef.h
index c8d9053..566f5dd 100644
--- a/include/osmocom/core/tdef.h
+++ b/include/osmocom/core/tdef.h
@@ -95,7 +95,7 @@
void osmo_tdefs_reset(struct osmo_tdef *tdefs);
unsigned long osmo_tdef_get(const struct osmo_tdef *tdefs, int T, enum
osmo_tdef_unit as_unit,
- unsigned long val_if_not_present);
+ long val_if_not_present);
struct osmo_tdef *osmo_tdef_get_entry(struct osmo_tdef *tdefs, int T);
/*! Using osmo_tdef for osmo_fsm_inst: array entry for a mapping of state
numbers to timeout definitions.
diff --git a/src/tdef.c b/src/tdef.c
index 3cfb17c..40a9900 100644
--- a/src/tdef.c
+++ b/src/tdef.c
@@ -183,10 +183,11 @@
* \param[in] val_if_not_present Fallback value to return if no timeout is
defined.
* \return Timeout value in the unit given by as_unit, rounded up if
necessary, or val_if_not_present.
*/
-unsigned long osmo_tdef_get(const struct osmo_tdef *tdefs, int T, enum
osmo_tdef_unit as_unit, unsigned long val_if_not_present)
+unsigned long osmo_tdef_get(const struct osmo_tdef *tdefs, int T, enum
osmo_tdef_unit as_unit, long val_if_not_present)
{
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);
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15218
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Ie61c3c85069916336e6dbd91a2c16f7634816417
Gerrit-Change-Number: 15218
Gerrit-PatchSet: 2
Gerrit-Owner: neels
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria
Gerrit-Reviewer: neels
Gerrit-Reviewer: pespin
Gerrit-MessageType: merged
Change in ...libosmocore[master]: osmo_tdef_get(): allow passing -1 as default timeout
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15218 ) Change subject: osmo_tdef_get(): allow passing -1 as default timeout .. Patch Set 2: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15218 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ie61c3c85069916336e6dbd91a2c16f7634816417 Gerrit-Change-Number: 15218 Gerrit-PatchSet: 2 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: pespin Gerrit-Comment-Date: Thu, 15 Aug 2019 09:33:41 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...libosmocore[master]: osmo_tdef_get(): allow passing -1 as default timeout
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15218 ) Change subject: osmo_tdef_get(): allow passing -1 as default timeout .. Patch Set 2: Code-Review+1 (1 comment) https://gerrit.osmocom.org/#/c/15218/2/src/tdef.c File src/tdef.c: https://gerrit.osmocom.org/#/c/15218/2/src/tdef.c@186 PS2, Line 186: osmo_tdef_get Alternatively, you could add another function without that optional parameter, so it would simply crash the process if (t == NULL), while this function would always return the val_if_not_present. Just an idea. The current approach also looks good. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15218 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ie61c3c85069916336e6dbd91a2c16f7634816417 Gerrit-Change-Number: 15218 Gerrit-PatchSet: 2 Gerrit-Owner: neels Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Comment-Date: Thu, 15 Aug 2019 07:00:08 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...libosmocore[master]: osmo_tdef_get(): allow passing -1 as default timeout
neels has uploaded a new patch set (#2). ( https://gerrit.osmocom.org/c/libosmocore/+/15218 ) Change subject: osmo_tdef_get(): allow passing -1 as default timeout .. osmo_tdef_get(): allow passing -1 as default timeout The intention of osmo_tdef_get()'s val_if_not_present argument was to return a default timeout, or to optionally abort the program for missing timer definitions if the default timeout is < 0. This was the case in the original implementation of this API in osmo-bsc, but in the migration to libosmocore, the argument was by accident changed to an unsigned type. In consequence, the assertion in the implementation that was intended to abort the program seemed bogus to coverity, and was fixed by removal in I7a544d2d43b83135def296674f777e48fe5fd80a -- the wrong direction, as is obvious from the API doc for osmo_tdef_get(). Note that osmo-bsc master passes -1 in various places and expects the program-abort behavior that was missing from the libosmocore implementation. Change the val_if_not_present argument to a signed type, and revert removal of the assertion, so that passing -1 has the effect described in the API doc: program abort on missing timer definition. This bug was not detected because it is hard to write tests that expect a program abort to happen, hence no tests for this API feature exist. Related: OS#4152 Change-Id: Ie61c3c85069916336e6dbd91a2c16f7634816417 --- M TODO-RELEASE M include/osmocom/core/tdef.h M src/tdef.c 3 files changed, 4 insertions(+), 2 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/18/15218/2 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15218 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Ie61c3c85069916336e6dbd91a2c16f7634816417 Gerrit-Change-Number: 15218 Gerrit-PatchSet: 2 Gerrit-Owner: neels Gerrit-MessageType: newpatchset
Change in ...libosmocore[master]: osmo_tdef_get(): allow passing -1 as default timeout
neels has uploaded this change for review. (
https://gerrit.osmocom.org/c/libosmocore/+/15218
Change subject: osmo_tdef_get(): allow passing -1 as default timeout
..
osmo_tdef_get(): allow passing -1 as default timeout
The intention of osmo_tdef_get()'s val_if_not_present argument was to return a
default timeout, or to optionally abort the program for missing timer
definitions if the default timeout is < 0. This was the case in the original
implementation of this API in osmo-bsc, but in the migration to libosmocore,
the argument was by accident changed to an unsigned type. In consequence, the
assertion in the implementation that was intended to abort the program seemed
bogus to coverity, and was fixed by removal in
I7a544d2d43b83135def296674f777e48fe5fd80a -- the wrong direction, as is obvious
from the API doc for osmo_tdef_get().
Note that osmo-bsc master passes -1 in various places and expects the
program-abort behavior that was missing from the libosmocore implementation.
Change the val_if_not_present argument to a signed type, and revert removal of
the assertion, so that passing -1 has the effect described in the API doc:
program abort on missing timer definition.
This bug was not detected because it is hard to write tests that expect a
program abort to happen, hence no tests for this API feature exist.
Change-Id: Ie61c3c85069916336e6dbd91a2c16f7634816417
---
M TODO-RELEASE
M include/osmocom/core/tdef.h
M src/tdef.c
3 files changed, 4 insertions(+), 2 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/18/15218/1
diff --git a/TODO-RELEASE b/TODO-RELEASE
index 8ccfa49..665ecf7 100644
--- a/TODO-RELEASE
+++ b/TODO-RELEASE
@@ -7,3 +7,4 @@
# If any interfaces have been added since the last public release: c:r:a + 1.
# If any interfaces have been removed or changed since the last public
release: c:r:0.
#library whatdescription / commit summary line
+core osmo_tdef_get() change val_if_not_present arg from
unsigned long to long to allow passing -1
diff --git a/include/osmocom/core/tdef.h b/include/osmocom/core/tdef.h
index c8d9053..566f5dd 100644
--- a/include/osmocom/core/tdef.h
+++ b/include/osmocom/core/tdef.h
@@ -95,7 +95,7 @@
void osmo_tdefs_reset(struct osmo_tdef *tdefs);
unsigned long osmo_tdef_get(const struct osmo_tdef *tdefs, int T, enum
osmo_tdef_unit as_unit,
- unsigned long val_if_not_present);
+ long val_if_not_present);
struct osmo_tdef *osmo_tdef_get_entry(struct osmo_tdef *tdefs, int T);
/*! Using osmo_tdef for osmo_fsm_inst: array entry for a mapping of state
numbers to timeout definitions.
diff --git a/src/tdef.c b/src/tdef.c
index 3cfb17c..40a9900 100644
--- a/src/tdef.c
+++ b/src/tdef.c
@@ -183,10 +183,11 @@
* \param[in] val_if_not_present Fallback value to return if no timeout is
defined.
* \return Timeout value in the unit given by as_unit, rounded up if
necessary, or val_if_not_present.
*/
-unsigned long osmo_tdef_get(const struct osmo_tdef *tdefs, int T, enum
osmo_tdef_unit as_unit, unsigned long val_if_not_present)
+unsigned long osmo_tdef_get(const struct osmo_tdef *tdefs, int T, enum
osmo_tdef_unit as_unit, long val_if_not_present)
{
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);
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15218
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Ie61c3c85069916336e6dbd91a2c16f7634816417
Gerrit-Change-Number: 15218
Gerrit-PatchSet: 1
Gerrit-Owner: neels
Gerrit-MessageType: newchange
