[PATCH] Fix string to integer conversion

[Ralph Böhme]

Hi Jeremy,

> Am 23.02.2019 um 19:23 schrieb Jeremy Allison <jra at samba.org>:
> 
> On Sat, Feb 23, 2019 at 09:18:31AM +0100, Ralph Böhme wrote:
>>> then you don't need to look inside, the
>>> code is obvious.
>> 
>> I'd still have to look to check why the initialisation is needed.
> 
> Well this kind of pattern (no-initialization
> at declaration, followed by test after function
> call) used to confuse Coverity into issuing false
> positives, so I still think in this case initialization
> at declaration is the right thing to do.

I guess fixing the API is right thing to do here. :)

> 
>>> So I'm going to stick with my request, to make
>>> future reviewers lives easier !
>>> 
>>> You could help by reviewing the patch :-) :-).
>> 
>> now you got my attention. :) I'd say the problem is, that the API of the new
>> functions is still not ideal. I'd prefer
>> 
>> bool tstrto[u]l[l](const char *nptr, char **endptr, int base, [unsigned][long]long int *result)
>> 
>> Then the caller just does the usual
>> 
>> ok = tstrtol(...);
>> if (!ok) {
>>       ....
>> }
>> 
>> The function would return false for values that are out of range and
>> probably also for the empty string and if there were no digits, I guess that
>> is what all callers care about.
> 
> Oh god, what color is my bike shed today :-).

The *right* one :)))

> OK, that was my initial take on the API too, but
> I was trying to use this as a teaching moment
> and Swen was doing the work.
> 
> That makes it a different interface to the
> existing POSIX strtoul()/strtoull() calls,

yes please. :)

> which I think Swen was trying to preserve
> as a coding pattern.

Why?

> I'm at Vault next week, so if you want it
> changed I'll have to get to it later.

Well, I guess the current implementation of strtoul[l]_err() is not even correct.

The patchset introduces an additional error check in the callers in some places where only the return error is checked, causing an error log message or even a failure in the caller. The problem is, according to POSIX/SUS, strtoul and friends are not *required* to set errno to EINVAL "in case no conversion was performed". The implementation of strtoul[l]_err() doesn't set it, it just returns the errno from strtoul.

If strtoul[l]_err() returns an error, it must be reliable. While this could be addressed by returning EINVAL for the "no conversion was performed" case, it still leaves us with a strange API.

Thoughts?
-slow

-- 
Ralph Boehme, Samba Team                https://samba.org/
Samba Developer, SerNet GmbH   https://sernet.de/en/samba/
GPG-Fingerprint   FAE2C6088A24252051C559E4AA1E9B7126399E46