Change in ...libosmocore[master]: tdef: Introduce min_val and max_val fields
pespin has submitted this change and it was merged. (
https://gerrit.osmocom.org/c/libosmocore/+/15546 )
Change subject: tdef: Introduce min_val and max_val fields
..
tdef: Introduce min_val and max_val fields
This is useful for timers expected to have a range of valid or expected
values.
Validation is done at runtime when timer values are set by the app or by
the user through the VTY.
Related: OS#4190
Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2
---
M TODO-RELEASE
M include/osmocom/core/tdef.h
M src/tdef.c
M src/vty/tdef_vty.c
M tests/tdef/tdef_test.c
M tests/tdef/tdef_test.ok
M tests/tdef/tdef_vty_test_config_root.c
M tests/tdef/tdef_vty_test_config_root.vty
8 files changed, 175 insertions(+), 13 deletions(-)
Approvals:
laforge: Looks good to me, but someone else must approve
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 665ecf7..547b5a9 100644
--- a/TODO-RELEASE
+++ b/TODO-RELEASE
@@ -8,3 +8,4 @@
# 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
+core struct osmo_tdeffields min_val,max_val added, ABI break
(arrays of structs used in programs)
diff --git a/include/osmocom/core/tdef.h b/include/osmocom/core/tdef.h
index 8155688..54819d9 100644
--- a/include/osmocom/core/tdef.h
+++ b/include/osmocom/core/tdef.h
@@ -77,6 +77,10 @@
/*! Currently active timeout value, e.g. set by user config. This is
the only mutable member: a user may
* configure the timeout value, but neither unit nor any other field. */
unsigned long val;
+ /*! Minimum timer value (in this tdef unit), checked if set (not zero).
*/
+ unsigned long min_val;
+ /*! Maximum timer value (in this tdef unit), checked if set (not zero).
*/
+ unsigned long max_val;
};
/*! Iterate an array of struct osmo_tdef, the last item should be fully zero,
i.e. "{}".
@@ -98,6 +102,8 @@
long val_if_not_present);
struct osmo_tdef *osmo_tdef_get_entry(struct osmo_tdef *tdefs, int T);
int osmo_tdef_set(struct osmo_tdef *tdefs, int T, unsigned long val, enum
osmo_tdef_unit val_unit);
+bool osmo_tdef_val_in_range(struct osmo_tdef *tdef, unsigned long new_val);
+int osmo_tdef_range_str_buf(char *buf, size_t buf_len, struct osmo_tdef *t);
/*! Using osmo_tdef for osmo_fsm_inst: array entry for a mapping of state
numbers to timeout definitions.
* For a usage example, see osmo_tdef_get_state_timeout() and
test_tdef_state_timeout() in tdef_test.c. */
diff --git a/src/tdef.c b/src/tdef.c
index ab6a51b..94d987f 100644
--- a/src/tdef.c
+++ b/src/tdef.c
@@ -136,14 +136,22 @@
/*! Set all osmo_tdef values to the default_val.
* It is convenient to define a tdefs array by setting only the default_val,
and calling osmo_tdefs_reset() once for
- * program startup. (See also osmo_tdef_vty_init())
+ * program startup. (See also osmo_tdef_vty_init()).
+ * During call to this function, default values are verified to be inside
valid range; process is aborted otherwise.
* \param[in] tdefs Array of timer definitions, last entry being fully zero.
*/
void osmo_tdefs_reset(struct osmo_tdef *tdefs)
{
struct osmo_tdef *t;
- osmo_tdef_for_each(t, tdefs)
+ osmo_tdef_for_each(t, tdefs) {
+ if (!osmo_tdef_val_in_range(t, t->default_val)) {
+ char range_str[64];
+ osmo_tdef_range_str_buf(range_str, sizeof(range_str),
t);
+ osmo_panic("%s:%d Timer " OSMO_T_FMT " contains default
value %lu not in range %s\n",
+ __FILE__, __LINE__, OSMO_T_FMT_ARGS(t->T),
t->default_val, range_str);
+ }
t->val = t->default_val;
+ }
}
/*! Return the value of a T timer from a list of osmo_tdef, in the given unit.
@@ -221,13 +229,55 @@
*/
int osmo_tdef_set(struct osmo_tdef *tdefs, int T, unsigned long val, enum
osmo_tdef_unit val_unit)
{
+ unsigned long new_val;
struct osmo_tdef *t = osmo_tdef_get_entry(tdefs, T);
if (!t)
return -EEXIST;
- t->val = osmo_tdef_round(val, val_unit, t->unit);
+
+ new_val = osmo_tdef_round(val, val_unit, t->unit);
+ if (!osmo_tdef_val_in_range(t, new_val))
+ return -ERANGE;
+
+ t->val = new_val;
return 0;
}
+/*! Check if value new_val is in range of valid possible values for timer
entry tdef.
+ * \param[in] tdef Timer entry from a timer definition table.
+ * \param[in] new_val The value whose validity to check, in units as per this
timer entry.
+ * \return true if inside
Change in ...libosmocore[master]: tdef: Introduce min_val and max_val fields
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15546 ) Change subject: tdef: Introduce min_val and max_val fields .. Patch Set 7: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15546 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2 Gerrit-Change-Number: 15546 Gerrit-PatchSet: 7 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Mon, 07 Oct 2019 13:14:10 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...libosmocore[master]: tdef: Introduce min_val and max_val fields
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15546 ) Change subject: tdef: Introduce min_val and max_val fields .. Patch Set 7: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15546 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2 Gerrit-Change-Number: 15546 Gerrit-PatchSet: 7 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Mon, 07 Oct 2019 13:07:58 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...libosmocore[master]: tdef: Introduce min_val and max_val fields
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15546 ) Change subject: tdef: Introduce min_val and max_val fields .. Patch Set 7: ping? last patch version was submitted around 10 days ago. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15546 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2 Gerrit-Change-Number: 15546 Gerrit-PatchSet: 7 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Mon, 07 Oct 2019 08:18:43 + Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in ...libosmocore[master]: tdef: Introduce min_val and max_val fields
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15546 ) Change subject: tdef: Introduce min_val and max_val fields .. Patch Set 7: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15546 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2 Gerrit-Change-Number: 15546 Gerrit-PatchSet: 7 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: laforge Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Sat, 28 Sep 2019 10:41:19 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...libosmocore[master]: tdef: Introduce min_val and max_val fields
Hello fixeria, neels, osmith, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmocore/+/15546 to look at the new patch set (#7). Change subject: tdef: Introduce min_val and max_val fields .. tdef: Introduce min_val and max_val fields This is useful for timers expected to have a range of valid or expected values. Validation is done at runtime when timer values are set by the app or by the user through the VTY. Related: OS#4190 Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2 --- M TODO-RELEASE M include/osmocom/core/tdef.h M src/tdef.c M src/vty/tdef_vty.c M tests/tdef/tdef_test.c M tests/tdef/tdef_test.ok M tests/tdef/tdef_vty_test_config_root.c M tests/tdef/tdef_vty_test_config_root.vty 8 files changed, 175 insertions(+), 13 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/46/15546/7 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15546 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2 Gerrit-Change-Number: 15546 Gerrit-PatchSet: 7 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-CC: laforge Gerrit-MessageType: newpatchset
Change in ...libosmocore[master]: tdef: Introduce min_val and max_val fields
Hello fixeria, neels, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmocore/+/15546 to look at the new patch set (#6). Change subject: tdef: Introduce min_val and max_val fields .. tdef: Introduce min_val and max_val fields This is useful for timers expected to have a range of valid or expected values. Validation is done at runtime when timer values are set by the app or by the user through the VTY. Related: OS#4190 Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2 --- M TODO-RELEASE M include/osmocom/core/tdef.h M src/tdef.c M src/vty/tdef_vty.c M tests/tdef/tdef_test.c M tests/tdef/tdef_test.ok M tests/tdef/tdef_vty_test_config_root.c M tests/tdef/tdef_vty_test_config_root.vty 8 files changed, 175 insertions(+), 13 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/46/15546/6 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15546 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2 Gerrit-Change-Number: 15546 Gerrit-PatchSet: 6 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-CC: laforge Gerrit-MessageType: newpatchset
Change in ...libosmocore[master]: tdef: Introduce min_val and max_val fields
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15546 ) Change subject: tdef: Introduce min_val and max_val fields .. Patch Set 5: This change is ready for review. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15546 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2 Gerrit-Change-Number: 15546 Gerrit-PatchSet: 5 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-CC: laforge Gerrit-Comment-Date: Thu, 19 Sep 2019 17:55:35 + Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in ...libosmocore[master]: tdef: Introduce min_val and max_val fields
Hello fixeria, neels, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmocore/+/15546 to look at the new patch set (#4). Change subject: tdef: Introduce min_val and max_val fields .. tdef: Introduce min_val and max_val fields This is useful for timers expected to have a range of valid or expected values. Validation is done at runtime when timer values are set by the app or by the user through the VTY. Related: OS#4190 Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2 --- M TODO-RELEASE M include/osmocom/core/tdef.h M src/tdef.c M src/vty/tdef_vty.c M tests/tdef/tdef_test.c M tests/tdef/tdef_test.ok M tests/tdef/tdef_vty_test_config_root.c M tests/tdef/tdef_vty_test_config_root.vty 8 files changed, 172 insertions(+), 13 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/46/15546/4 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15546 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2 Gerrit-Change-Number: 15546 Gerrit-PatchSet: 4 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-CC: laforge Gerrit-MessageType: newpatchset
Change in ...libosmocore[master]: tdef: Introduce min_val and max_val fields
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15546 ) Change subject: tdef: Introduce min_val and max_val fields .. Patch Set 3: (2 comments) https://gerrit.osmocom.org/#/c/15546/3/src/vty/tdef_vty.c File src/vty/tdef_vty.c: https://gerrit.osmocom.org/#/c/15546/3/src/vty/tdef_vty.c@188 PS3, Line 188: ULONG_MAX Are you sure it's safe to print this macro? Maybe rather print "INF" as string? Or print min/max values separately? vty_out(vty, ", min %lu", ...); if (t->max_val) vty_out(vty, ", max %lu", ...); https://gerrit.osmocom.org/#/c/15546/3/tests/tdef/tdef_vty_test_config_root.vty File tests/tdef/tdef_vty_test_config_root.vty: https://gerrit.osmocom.org/#/c/15546/3/tests/tdef/tdef_vty_test_config_root.vty@25 PS3, Line 25: 18446744073709551615 This value may be different on some architectures, so this test will fail. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15546 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2 Gerrit-Change-Number: 15546 Gerrit-PatchSet: 3 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-CC: laforge Gerrit-Comment-Date: Wed, 18 Sep 2019 17:29:39 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in ...libosmocore[master]: tdef: Introduce min_val and max_val fields
Hello fixeria, neels, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmocore/+/15546 to look at the new patch set (#3). Change subject: tdef: Introduce min_val and max_val fields .. tdef: Introduce min_val and max_val fields This is useful for timers expected to have a range of valid or expected values. Validation is done at runtime when timer values are set by the app or by the user through the VTY. Related: OS#4190 Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2 --- M TODO-RELEASE M include/osmocom/core/tdef.h M src/tdef.c M src/vty/tdef_vty.c M tests/tdef/tdef_test.c M tests/tdef/tdef_test.ok M tests/tdef/tdef_vty_test_config_root.c M tests/tdef/tdef_vty_test_config_root.vty 8 files changed, 139 insertions(+), 13 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/46/15546/3 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15546 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2 Gerrit-Change-Number: 15546 Gerrit-PatchSet: 3 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-CC: laforge Gerrit-MessageType: newpatchset
Change in ...libosmocore[master]: tdef: Introduce min_val and max_val fields
Hello fixeria, neels, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmocore/+/15546 to look at the new patch set (#2). Change subject: tdef: Introduce min_val and max_val fields .. tdef: Introduce min_val and max_val fields This is useful for timers expected to have a range of valid or expected values. Validation is done at runtime when timer values are set by the app or by the user through the VTY. Related: OS#4190 Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2 --- M include/osmocom/core/tdef.h M src/tdef.c M src/vty/tdef_vty.c M tests/tdef/tdef_test.c M tests/tdef/tdef_test.ok M tests/tdef/tdef_vty_test_config_root.c M tests/tdef/tdef_vty_test_config_root.vty 7 files changed, 138 insertions(+), 13 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/46/15546/2 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15546 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2 Gerrit-Change-Number: 15546 Gerrit-PatchSet: 2 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-CC: laforge Gerrit-MessageType: newpatchset
Change in ...libosmocore[master]: tdef: Introduce min_val and max_val fields
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15546 ) Change subject: tdef: Introduce min_val and max_val fields .. Patch Set 1: (1 comment) https://gerrit.osmocom.org/#/c/15546/1/src/tdef.c File src/tdef.c: https://gerrit.osmocom.org/#/c/15546/1/src/tdef.c@235 PS1, Line 235: EINVAL > also prefer -ERANGE. […] Yes it should, see tdef_vty.c -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15546 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2 Gerrit-Change-Number: 15546 Gerrit-PatchSet: 1 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-CC: laforge Gerrit-Comment-Date: Wed, 18 Sep 2019 11:18:41 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: fixeria Comment-In-Reply-To: neels Gerrit-MessageType: comment
Change in ...libosmocore[master]: tdef: Introduce min_val and max_val fields
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15546 ) Change subject: tdef: Introduce min_val and max_val fields .. Patch Set 1: > Patch Set 1: Code-Review-1 > > (7 comments) > > All in all very nice. > > Please also add range tests to all of the *.vty transcript tests, see > tests/tdef/tdef_vty*, so that we can also verify the VTY behavior. > > BTW, in order to benefit from the range checks, all tdef users need to be > changed to use osmo_tdef_set() instead of osmo_tdef_get_entry()? Do you have > related patches? ... just realizing that this only needs to be done as soon > as an application actually sets min and max values, so that should be fine. > But this should be mentioned in the API doc of the new min and max members. Most tdef users actually set the values through VTY, and that one is already checking for ranges (done in this patch). I recently introduced osmo_tdef_set myself in osmo-pcu for timers whose values are coming from osmo-bts, so those are working already. I still need to check if there are other users os osmo_tdef setting tdef->val manually. But in any case it doesn't hurt because they don't have min_val or max_val set, so range check is not applied there. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15546 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2 Gerrit-Change-Number: 15546 Gerrit-PatchSet: 1 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Reviewer: neels Gerrit-Reviewer: pespin Gerrit-CC: laforge Gerrit-Comment-Date: Wed, 18 Sep 2019 10:49:29 + Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in ...libosmocore[master]: tdef: Introduce min_val and max_val fields
neels has posted comments on this change. (
https://gerrit.osmocom.org/c/libosmocore/+/15546 )
Change subject: tdef: Introduce min_val and max_val fields
..
Patch Set 1: Code-Review-1
(7 comments)
All in all very nice.
Please also add range tests to all of the *.vty transcript tests, see
tests/tdef/tdef_vty*, so that we can also verify the VTY behavior.
BTW, in order to benefit from the range checks, all tdef users need to be
changed to use osmo_tdef_set() instead of osmo_tdef_get_entry()? Do you have
related patches? ... just realizing that this only needs to be done as soon as
an application actually sets min and max values, so that should be fine. But
this should be mentioned in the API doc of the new min and max members.
https://gerrit.osmocom.org/#/c/15546/1/src/tdef.c
File src/tdef.c:
https://gerrit.osmocom.org/#/c/15546/1/src/tdef.c@137
PS1, Line 137: Set all osmo_tdef values to the default_val.
> Please update documentation too as you're introducing additional
> functionality.
IMHO this doesn't need a doc update...
https://gerrit.osmocom.org/#/c/15546/1/src/tdef.c@235
PS1, Line 235: EINVAL
> How about ERANGE?
also prefer -ERANGE.
Does a cfg file with out-of-range value cause a proper errmsg and stop the
program from starting?
https://gerrit.osmocom.org/#/c/15546/1/src/vty/tdef_vty.c
File src/vty/tdef_vty.c:
https://gerrit.osmocom.org/#/c/15546/1/src/vty/tdef_vty.c@127
PS1, Line 127: Invalid T timer value %lu (should be %lu <= val <= %lu)
> This message could be more concrete: […]
I also prefer fixeria's errmsg,
and please also output the T number (using OSMO_T_FMT and OSMO_T_FMT_ARGS like
in line 90).
The reason is that otherwise it is really really hard to figure out which .cfg
line was responsible for the error.
https://gerrit.osmocom.org/#/c/15546/1/src/vty/tdef_vty.c@128
PS1, Line 128: new_val, t->min_val, t->max_val ? : (unsigned
long) -1 , VTY_NEWLINE);
rather ULONG_MAX from limits.h
https://gerrit.osmocom.org/#/c/15546/1/src/vty/tdef_vty.c@188
PS1, Line 188: vty_out(vty, ", range: [%lu,%lu]", t->min_val,
t->max_val ? : (unsigned long) -1);
ULONG_MAX
https://gerrit.osmocom.org/#/c/15546/1/tests/tdef/tdef_test.c
File tests/tdef/tdef_test.c:
https://gerrit.osmocom.org/#/c/15546/1/tests/tdef/tdef_test.c@164
PS1, Line 164: printf("setting 7 = 10 (EINVAL)\n");
(also -ERANGE)
https://gerrit.osmocom.org/#/c/15546/1/tests/tdef/tdef_test.c@183
PS1, Line 183: OSMO_ASSERT(osmo_tdef_set(tdefs, 23, 50, OSMO_TDEF_S) ==
-EEXIST);
this is a nice test to add, but seems unrelated to this patch?
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15546
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2
Gerrit-Change-Number: 15546
Gerrit-PatchSet: 1
Gerrit-Owner: pespin
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria
Gerrit-Reviewer: neels
Gerrit-CC: laforge
Gerrit-Comment-Date: Tue, 17 Sep 2019 23:33:29 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: fixeria
Gerrit-MessageType: comment
Change in ...libosmocore[master]: tdef: Introduce min_val and max_val fields
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15546 ) Change subject: tdef: Introduce min_val and max_val fields .. Patch Set 1: needs entry in TODO-RELEASE as it breaks ABI -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15546 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2 Gerrit-Change-Number: 15546 Gerrit-PatchSet: 1 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-CC: laforge Gerrit-Comment-Date: Tue, 17 Sep 2019 17:49:10 + Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment
Change in ...libosmocore[master]: tdef: Introduce min_val and max_val fields
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15546 ) Change subject: tdef: Introduce min_val and max_val fields .. Patch Set 1: Code-Review+1 (3 comments) https://gerrit.osmocom.org/#/c/15546/1/src/tdef.c File src/tdef.c: https://gerrit.osmocom.org/#/c/15546/1/src/tdef.c@137 PS1, Line 137: Set all osmo_tdef values to the default_val. Please update documentation too as you're introducing additional functionality. https://gerrit.osmocom.org/#/c/15546/1/src/tdef.c@235 PS1, Line 235: EINVAL How about ERANGE? https://gerrit.osmocom.org/#/c/15546/1/src/vty/tdef_vty.c File src/vty/tdef_vty.c: https://gerrit.osmocom.org/#/c/15546/1/src/vty/tdef_vty.c@127 PS1, Line 127: Invalid T timer value %lu (should be %lu <= val <= %lu) This message could be more concrete: "Timer value %lu is out of range (%lu .. %lu)" Not a merge blocker, just a recommendation. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15546 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2 Gerrit-Change-Number: 15546 Gerrit-PatchSet: 1 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria Gerrit-Comment-Date: Tue, 17 Sep 2019 17:33:54 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...libosmocore[master]: tdef: Introduce min_val and max_val fields
pespin has uploaded this change for review. (
https://gerrit.osmocom.org/c/libosmocore/+/15546
Change subject: tdef: Introduce min_val and max_val fields
..
tdef: Introduce min_val and max_val fields
This is useful for timers expected to have a range of valid or expected
values.
Validation is done at runtime when timer values are set by the app or by
the user through the VTY.
Related: OS#4190
Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2
---
M include/osmocom/core/tdef.h
M src/tdef.c
M src/vty/tdef_vty.c
M tests/tdef/tdef_test.c
M tests/tdef/tdef_test.ok
5 files changed, 87 insertions(+), 12 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/46/15546/1
diff --git a/include/osmocom/core/tdef.h b/include/osmocom/core/tdef.h
index 8155688..ac4b640 100644
--- a/include/osmocom/core/tdef.h
+++ b/include/osmocom/core/tdef.h
@@ -77,6 +77,10 @@
/*! Currently active timeout value, e.g. set by user config. This is
the only mutable member: a user may
* configure the timeout value, but neither unit nor any other field. */
unsigned long val;
+ /*! Minimum timer value (in this tdef unit), checked if set (not zero).
*/
+ unsigned long min_val;
+ /*! Maximum timer value (in this tdef unit), checked if set (not zero).
*/
+ unsigned long max_val;
};
/*! Iterate an array of struct osmo_tdef, the last item should be fully zero,
i.e. "{}".
@@ -98,6 +102,7 @@
long val_if_not_present);
struct osmo_tdef *osmo_tdef_get_entry(struct osmo_tdef *tdefs, int T);
int osmo_tdef_set(struct osmo_tdef *tdefs, int T, unsigned long val, enum
osmo_tdef_unit val_unit);
+bool osmo_tdef_val_in_range(struct osmo_tdef *tdef, unsigned long new_val);
/*! Using osmo_tdef for osmo_fsm_inst: array entry for a mapping of state
numbers to timeout definitions.
* For a usage example, see osmo_tdef_get_state_timeout() and
test_tdef_state_timeout() in tdef_test.c. */
diff --git a/src/tdef.c b/src/tdef.c
index ab6a51b..bb73f76 100644
--- a/src/tdef.c
+++ b/src/tdef.c
@@ -142,8 +142,12 @@
void osmo_tdefs_reset(struct osmo_tdef *tdefs)
{
struct osmo_tdef *t;
- osmo_tdef_for_each(t, tdefs)
+ osmo_tdef_for_each(t, tdefs) {
+ if (!osmo_tdef_val_in_range(t, t->default_val))
+ osmo_panic("%s:%d Invalid default value %lu <= %lu <=
%lu set in struct osmo_tdef (%s)\n",
+ __FILE__, __LINE__, t->min_val,
t->default_val, t->max_val ? : (unsigned long) -1, t->desc);
t->val = t->default_val;
+ }
}
/*! Return the value of a T timer from a list of osmo_tdef, in the given unit.
@@ -221,13 +225,29 @@
*/
int osmo_tdef_set(struct osmo_tdef *tdefs, int T, unsigned long val, enum
osmo_tdef_unit val_unit)
{
+ unsigned long new_val;
struct osmo_tdef *t = osmo_tdef_get_entry(tdefs, T);
if (!t)
return -EEXIST;
- t->val = osmo_tdef_round(val, val_unit, t->unit);
+
+ new_val = osmo_tdef_round(val, val_unit, t->unit);
+ if (!osmo_tdef_val_in_range(t, new_val))
+ return -EINVAL;
+
+ t->val = new_val;
return 0;
}
+/*! Check if value new_val is in range of valid possible values for timer
entry tdef.
+ * \param[in] tdef Timer entry from a timer definition table.
+ * \param[in] new_val The value whose validity to check, in units as per this
timer entry.
+ * \return true if inside range, false otherwise.
+ */
+bool osmo_tdef_val_in_range(struct osmo_tdef *tdef, unsigned long new_val)
+{
+ return new_val >= tdef->min_val && (!tdef->max_val || new_val <=
tdef->max_val);
+}
+
/*! Using osmo_tdef for osmo_fsm_inst: find a given state's
osmo_tdef_state_timeout entry.
*
* The timeouts_array shall contain exactly 32 elements, regardless whether
only some of them are actually populated
diff --git a/src/vty/tdef_vty.c b/src/vty/tdef_vty.c
index eb05c3c..a8e48d3 100644
--- a/src/vty/tdef_vty.c
+++ b/src/vty/tdef_vty.c
@@ -115,12 +115,20 @@
*/
int osmo_tdef_vty_set_cmd(struct vty *vty, struct osmo_tdef *tdefs, const char
**args)
{
+ unsigned long new_val;
const char *T_arg = args[0];
const char *val_arg = args[1];
struct osmo_tdef *t = osmo_tdef_vty_parse_T_arg(vty, tdefs, T_arg);
if (!t)
return CMD_WARNING;
- t->val = osmo_tdef_vty_parse_val_arg(val_arg, t->default_val);
+ new_val = osmo_tdef_vty_parse_val_arg(val_arg, t->default_val);
+
+ if (!osmo_tdef_val_in_range(t, new_val)) {
+ vty_out(vty, "%% Invalid T timer value %lu (should be %lu <=
val <= %lu)%s",
+ new_val, t->min_val, t->max_val ? : (unsigned long) -1
, VTY_NEWLINE);
+ return CMD_WARNING;
+ }
+ t->val = new_val;
return CMD_SUCCESS;
}
@@ -167,12 +175,19 @@
}
