[PATCH] Fix string to integer conversion

[Ralph Böhme]

On Tue, Feb 26, 2019 at 11:30:26AM +0100, swen wrote:
>On Tue, 2019-02-26 at 11:15 +0100, Ralph Böhme wrote:
>> Then I guess we also need
>>
>> +       if (nptr[0] == '\0') {
>> +               /* we got an empty string */
>> +               *err = EINVAL;
>> +               errno = save_errno;
>> +               return val;
>> +       }
>> +
>>
>> before the call to strtoul().
>
>No why ?
>strtoul(l) can deal with leading blanks and even the "new" check
>for fully wrong strings is still working.

  if (nptr[0] == '\0')

is not checking for leading blanks, it's checking for the empty string, but I 
just checked that strtoul() really returns nptr == tmp_endptr for that case, I 
wasn't sure and the standard leaves this a bit vague.

>..something like " foo" would be detected as invalid while
>something like " 1234" would be valid.
>Isn't that what we want ?
>
>Not sure if you know, but there are places in the code
>(I remember seeing at least one) where strings like "1234 foo" should
>be considered valid, with a conversion of 1234 only of course.
>
>>
>> Then lets remove the out-parameter uglyness and we're going
>> somewhere:
>>
>> bool tstrtoull(const char *nptr, char **endptr, int base, unsigned
>> long long int *val);
>>
>> :)
>>
>> Let's see what Jeremy has to say on it, I'll give him a chance to
>> convince me
>> that strtoull_err() in the current form is really a good idea.
>>
>well, I prefer the old-fashioned idea as well, not just Jeremy :-)
>Instead, could you come up with an explanation why your approach would
>be so beneficial ?

there's an existing pattern that functions return an error code as return 
argument, we have a few functions that follow it. :)

>> I imagine one previous argument has been, that a strict
>> implementation of a new
>> conversion function, be it strtoull_err() or tstrtoull(), may not be
>> suitable to
>> replace the existing call to strtoull() in all places due to the
>> behaviour
>> change?
>
>To be honest, I prefer to not open Pandora's box here and go
>with what I'd call a fix instead of a design change.

Yeah, I might be willing to cross that bridge. Let me go through the 
changes in the caller. Stay tuned...

>> Btw, there'a bug in strtoull_err(), fixup attached.
>
>I know, I fixed it already (running at gitlab currently) but was
>waiting for more "concerns" to be fixed with a new version.

ok. It just cost me half an hour to find it.

-slow

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