Re: lib/49429: Import strtonum(3)

[christos%zoulas.com@localhost (Christos Zoulas)]

The following reply was made to PR lib/49429; it has been noted by GNATS.

From: christos%zoulas.com@localhost (Christos Zoulas)
To: "Kamil Rytarowski" <n54%gmx.com@localhost>, gnats-bugs%NetBSD.org@localhost
Cc: 
Subject: Re: lib/49429: Import strtonum(3)
Date: Fri, 26 Dec 2014 20:57:33 -0500

 On Dec 27,  2:26am, n54%gmx.com@localhost ("Kamil Rytarowski") wrote:
 -- Subject: Re: lib/49429: Import strtonum(3)
 
 | Hello!
 | 
 | Thank you very much!
 | 
 | Your solution looks reasonable, but I have few notes:
 | 1) the function for (lo > hi) behaves in unpredicted way, I
 | suggest set errno immediatetely to EINVAL and return 0 (special
 | case)
 
 I can see putting a _DIAGASSERT() for it, or returning EINVAL.
 Still it is not unpredictable; it will always return the same result
 for the same value according to the prescribed logic.
 
 | 2) assume correct reults so for check of invalid results use __prefict_false()
 | 
 | if (__predict_false(lo > hi)) {
 | errno = EINVAL;  
 | return 0;
 | }
 
 This API is usually not performance critical, but sure.
 
 | 3) the example strtoi("foo", NULL, 0, 10, 20, NULL); makes NULL
 | pointer dereference
 
 Where?
 
 | 4) do we really want ENOSUP this way?
 
 We'd like to differentiate between a result failed by strtoimax()
 and one that we failed because of our additional checks.
 
 | 5) if base is not supported then we can unexpectedly abuse errno from EINVAL
 | and change return value from 0 to lo/hi
 
 We wanted base to be supported. Why castrate strtoimax() needlessly?
 Anyway even strtonum() does not differentiate; it is all "invalid"
 for it.
 
 | I'm upset for __OPENBSD_SOURCE, please don't do it!
 
 Well, you are going to make a compile-time error a link-time error
 if you introduce -lobcompat. Anyway we would like to discourage
 using functions that we don't like their API so... If you are
 compiling our own code, you are not supposed to use strtonum. If
 you want 3rd party code to use our own strtonum() you need to state
 so.
 
 | What do you think of libobcompat (OpenBSD compat layer) in
 | src/external (and in some appropriate place in pkgsrc), on top of
 | libnbcompat, with inline OpenBSD-specific functions. These functions
 | will be promised that they are NOT supported by us and they forbidden
 | in our own code. The compat layer will be used only to ease having
 | external code in the base (OpenSSH, tmux...).
 
 Functions we like we should add to the base ABI. The ones we do not
 like, we should decide what to do rather than cargo-cult them over to
 a library just for the sake of compatibility. We definitely do not want
 to encourage people adding API's we do not like in the base source.
 
 | One of the functions, next to strtonum(3) will be explicit_bzero(3).
 
 Case in point. We already have explicit_memset(3) which is superior to
 explicit_bzero(3) because:
 
 	- it does not build on obsolete APIs (b{copy,zero,cmp} are
 	  not obsolete -- politely marked LEGACY in ToG)
 	- it has more functionality, allowing to set a value instead
 	  of just zero.
 
 | In all other places, where the code is not external we will switch to
 | strtoi(3)/strtou(3).
 
 Right, and for external code, we can decide; if autoconf is supported
 and a replacement is provided we choose. If not, we define
 _OPENBSD_SOURCE.
 
 This is not about NIH; we'll gladly import API's that we consider
 robust, well designed, and orthogonal with existing ones.
 
 christos