Change in ...libosmocore[master]: tdef: Introduce min_val and max_val fields

[neels]

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 <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <axilirator at gmail.com>
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-CC: laforge <laforge at gnumonks.org>
Gerrit-Comment-Date: Tue, 17 Sep 2019 23:33:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: fixeria <axilirator at gmail.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190917/d1a33eab/attachment.htm>