[PATCH v1] Added string conversion utility functions

[Pekka Paalanen]

On Tue, 2 Dec 2014 15:45:40 +0200
Imran Zaman <imran.zaman at gmail.com> wrote:

> Interesting read.. have some arguments about the raised points.. but
> lets not go in that direction..
> Can you guys suggest to do some changes or shall i drop the patch
> altogether? if you are happy with current implementation lets live
> with it.. I noticed bug when weston was not working properly due to
> improper error handling of strtol in one place, so I thought it would
> be good idea to fix it wherever it is used (and hence this patch) but
> seems that it is not always the case :-)
> 
> BR
> imran
> 
> p.s: To me it was really simple to understand that in MOST (=NOT ALL)
> of the cases str-to-number conversion was done using strtol/strtoul
> but without proper error handling. I moved it to single place to make
> the life easier IMHO...

Hi,

fixing bugs is always welcome and recommended.

If you see a place, where the error checking is wanted/attempted but
insufficient, it would be very nice to fix it.

However, writing a helper function whose benefit over a direct call to
strto*() is only the handling of errno, it might not be worth it in my
opinion. Such a case is probably simpler to just fix in place than make
it use a helper function. (This might change with libweston, though.)

If the helper can assume that the whole given string must be a valid
number, I think that would be much more worth it, because it would then
hide the end pointer and nul checks. Such a function would be simple to
use and make the code where it is used much more obvious.

If you want to add error checking to places where there is none to
begin with, I think that would be very good, but also should be a
separate patch.

I don't see any value in adding error checking (by calling a helper)
and then ignoring the check result though.

It depends on the whole, really, where we strike the balance between
generality and simplicity of the helper function API.

I think a good next proposal would be to keep your helper functions
mostly as is (they are quite thorough), and replace only the call sites
where there already is some error checking.

Another thing is the return value: your helper functions return success
only if the whole string is a valid number (ends in nul). It does not
discriminate between conversion errors and converting only a substring
successfully. That makes it hard to use for anything where you want to
parse only a substring and stop at the first non-digit character. The
caller will not know if the number overflowed or if it just found a
non-digit character. If a non-digit character is expected, the return
value success/fail is completely useless and there is no way to even
manually check errno to see if the conversion failed or not. I think
this is one thing that Bill tried to explain. This is why the end_ptr
argument is useless as it is, because on success it can only point to
the end of the string.

To make it useful, the success/fail return value needs to be a little
different.

We have two basic use cases:
a) convert a whole string (up to nul-terminator), and
b) convert possibly a substring (up to non-digit or nul) and tell where
the conversion stopped

If you want to make a helper function that is useful for both at the
same time, you need to return success when the conversion succeeds,
even if it does not end in nul. OTOH, if the caller wants to verify the
whole string up to nul, it is not interested in end_ptr, which means
that you could check the nul iff end_ptr is not wanted and save the
effort from the caller.

Then there is the question of negative numbers which you special-cased
in convert_strtoul(). I'd rather it followed the stroul() semantics to
avoid surprises.

When you do return failure like in weston_strtoi(), you should make
sure errno communicates the error like strtol() would. That's part of
consistency.

Finally, you should decide how the equivalent of an empty string parses:
is it a failure or does it produce zero? I think you need to look at
what majority of the potential callers would expect, and go with that.
For instance, in multi-resource.c create_device(), the string to be
parsed is "A:B". If substring A is actually empty, it leads to the
value zero. I don't know if that is an intentional shorthand, but I
think it works.

Writing tests for the helpers is excellent. The cherry on top would be
documentation.


Thanks,
pq

> On Mon, Dec 1, 2014 at 8:30 PM, Bill Spitzak <spitzak at gmail.com> wrote:
> > On 12/01/2014 04:10 AM, Pekka Paalanen wrote:
> >
> >>                 other = strtol(pid, &end, 0);
> >>                 if (end != pid + 10) {
> >>                         weston_log("can't parse lock file %s\n",
> >>                                 lockfile);
> >>                         close(fd);
> >>                         errno = EEXIST;
> >>                         return -1;
> >>                 }
> >
> >
> >> 'pid' is a fixed size string read in from the lock file, which is
> >> converted into a number of type pid_t. Because the number is assumed to
> >> be printed by "%10d\n", the file should have at least 11 bytes
> >> available, and we assume all the 10 characters form a valid number
> >> (with leading spaces). It's all just a way to avoid dealing with
> >> unexpected EOF when reading the file, and to avoid not knowing in
> >> advance how many characters long the number is.
> >>
> >> This code still wants to parse the whole string as a single number, but
> >> it also knows the number will end in a newline instead of nul. It
> >> wouldn't be difficult to replace that newline with nul before parsing,
> >> if you really want to convert this code to use a helper. But while
> >> doing so, you have to ask yourself: does this actually make the code
> >> any easier to understand or more correct?
> >
> >
> > I believe you are correct, and this is a good indication that blindly
> > inserting the replacement function is not a good idea.
> >
> > The original code failed if there was a digit at offset 11 (as well as other
> > reasons for failing). The proposed replacement failed if offset 11 was
> > anything other than NUL. This is different. When I said the endptr was not
> > needed, my proposal actually has the exact same mistake.
> >
> > This does bring up a question as to whether the helper function should eat
> > trailing whitespace.
> >
> > But also that you can't drop the wrapper in everywhere.