Jes Sorensen <jes.soren...@redhat.com> writes:
> On 10/12/10 17:52, Markus Armbruster wrote:
[...]
>>> + c = *endptr++;
>>> + if (isspace(c) || c == '\0') {
>>> + c = 0;
>>> + } else if (!isspace(*endptr) && *endptr != 0) {
>>> + goto fail;
>>> + }
>>
>> I'm not happy with this check.
>>
>> If the caller needs a complete string consumed, then this check is
>> insufficient, because it doesn't catch trailing garbage as long as it
>> starts with whitespace. The caller still needs to check !*endptr.
>>
>> If the caller needs to continue parsing after the value, and expects
>> anything but whitespace there, it has to copy the value first. Only
>> easy if the value is followed by some delimiter that can't occur in the
>> value. Example: parse a size value from something of them form
>> name=value,name=value... Need to copy up to the next comma or end of
>> string.
>>
>> The check complicates the second case without really helping the first
>> case.
>>
>> Nevertheless, it's good enough for the uses in this patch series, so I'm
>> not insisting on getting this changed now.
>
> I hadn't thought of case #2, but I think that is pretty easy to handle
> by accepting ',' as a separator as well.
Then callers that don't want ',' have to check the suffix.
And when a caller comes around that wants '"', it has to copy the value
string, or hack strtosz() and figure out which of the existing callers
now have to check the suffix.
strtosz() shouldn't second guess what characters can legitimately follow
(the follow set in parsing parlance). That's only known up the call
chain.
I still think there are two clean interfaces:
* Consume the longest prefix that makes sense, then stop. Return where
we stopped. Leave parsing / rejecting the suffix to the caller.
That's how strtol() & friends work.
* Consume a complete string, fail if there's trailing garbage. This is
easier to use when you want to parse complete strings. It's awkward
when you don't.
> It's worth keeping in kind that
> the old code didn't do anything with trailing garbage either, it was
> silently ignored.
>
> For case #1 then I think it's ok to just accept trailing garbage, the
> old code would simply use strtoull and leave it at that.
As long as your patch doesn't make things worse, it can be committed.
When I come across another use of strtosz() where it doesn't work
nicely, I can fix it up.
[...]
