<p style="white-space: pre-wrap; word-wrap: break-word;">All in all very nice.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/15546">View Change</a></p><p>7 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15546/1/src/tdef.c">File src/tdef.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15546/1/src/tdef.c@137">Patch Set #1, Line 137:</a> <code style="font-family:monospace,monospace">Set all osmo_tdef values to the default_val.</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Please update documentation too as you're introducing additional functionality.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">IMHO this doesn't need a doc update...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15546/1/src/tdef.c@235">Patch Set #1, Line 235:</a> <code style="font-family:monospace,monospace">EINVAL</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">How about ERANGE?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">also prefer -ERANGE.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Does a cfg file with out-of-range value cause a proper errmsg and stop the program from starting?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15546/1/src/vty/tdef_vty.c">File src/vty/tdef_vty.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15546/1/src/vty/tdef_vty.c@127">Patch Set #1, Line 127:</a> <code style="font-family:monospace,monospace">Invalid T timer value %lu (should be %lu <= val <= %lu)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">This message could be more concrete: […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I also prefer fixeria's errmsg,<br>and please also output the T number (using OSMO_T_FMT and OSMO_T_FMT_ARGS like in line 90).<br>The reason is that otherwise it is really really hard to figure out which .cfg line was responsible for the error.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15546/1/src/vty/tdef_vty.c@128">Patch Set #1, Line 128:</a> <code style="font-family:monospace,monospace"> new_val, t->min_val, t->max_val ? : (unsigned long) -1 , VTY_NEWLINE);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">rather ULONG_MAX from limits.h</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15546/1/src/vty/tdef_vty.c@188">Patch Set #1, Line 188:</a> <code style="font-family:monospace,monospace"> vty_out(vty, ", range: [%lu,%lu]", t->min_val, t->max_val ? : (unsigned long) -1);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ULONG_MAX</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15546/1/tests/tdef/tdef_test.c">File tests/tdef/tdef_test.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15546/1/tests/tdef/tdef_test.c@164">Patch Set #1, Line 164:</a> <code style="font-family:monospace,monospace"> printf("setting 7 = 10 (EINVAL)\n");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">(also -ERANGE)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15546/1/tests/tdef/tdef_test.c@183">Patch Set #1, Line 183:</a> <code style="font-family:monospace,monospace"> OSMO_ASSERT(osmo_tdef_set(tdefs, 23, 50, OSMO_TDEF_S) == -EEXIST);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">this is a nice test to add, but seems unrelated to this patch?</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmocore/+/15546">change 15546</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/libosmocore/+/15546"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I4661ac41c29a009a1d5fc57d87aaee6041c7d1b2 </div>
<div style="display:none"> Gerrit-Change-Number: 15546 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: laforge <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 17 Sep 2019 23:33:29 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Comment-In-Reply-To: fixeria <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>