[RFC weston] Add strtoint() helper

[Peter Hutterer]

On Thu, Jul 14, 2016 at 01:03:34PM -0700, Bryce Harrington wrote:
> Adds a safe strtol helper function, modeled loosely after Wayland
> scanner's strtouint.  This encapsulates the various quirks of strtol
> behavior, and streamlines the interface to just handling base-10 numbers
> with a simple true/false error indicator.
> 
> Signed-off-by: Bryce Harrington <bryce at osg.samsung.com>
> ---
> 
> Hi,
> 
> With much of the strtol uses cleaned up, I figure the time's ripe to
> introduce a helper routine for factoring the logic out.
> 
> For brevity I've omitted tests and the actual substitution of this
> routine into the codebase - those should be straightforward once the
> function implementation is deemed acceptable and I plan to add them.
> 
> The implementation style is derived from Wayland scanner's strtouint.
> Placement of the code as an inline function in its own shared header
> file is following the precident set with shared/zalloc.h.  Both of these
> are just arbitrary choices though.
> 
> The most notable difference from strtouint() is that the value is
> returned by reference rather than as a return value.  This approach
> was suggested originally by Peter Hutterer, and is felt to provide
> a more reliably robust error handling mechanism.
> 
> So, some questions I'm requesting comment on:
> 
> 1. Is 'strtoint' an ok name for the function?  What would be better?

systemd uses a safe_ prefix for those to signal that it doesn't have
sideeffects. and it makes it easy to grep for all instances of those.
it also calls it safe_atoi, safe_atoui, etc. but that's mostly personal
preference.
 
> 2. I plan to also add similar helpers for uint and hexadecimal (color)
>    parsing.  Should a more general filename be chosen than strtoint.h?

string-helpers.h

> 3. Do we care at all about detailed error codes (ERANGE or EINVAL)?
>    This implementation assumes 'no', which is based on the lack of use
>    of detailed errno checks in the current codebase.

EINVAL must never happen in our implementation since we control the base.
ERANGE should return false since it's a parsing error.
the possible EINVAL in the man page should return false since it's a parsing
error. so - the answer is "no" :)

> 4. Should the return data type be int or int32_t?  I've seen it both
>    ways through the code.  I picked int here just since that's what
>    scanner.c uses.   

int32_t. in 99.9% it doesn't matter and where it does it's clear what we do.
 
> 5. Is it appropriate for this function to be inline in a header like
>    done with zalloc, or should the implementation be in a .c file
>    like done with most of the other helper routines?

leave it inline. The only drawback here afaict is bigger code size but it's
not like we're calling this from a million places. and the overhead of
not inlining this is likely worse.

>  shared/strtoint.h | 72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
>  create mode 100644 shared/strtoint.h
> 
> diff --git a/shared/strtoint.h b/shared/strtoint.h
> new file mode 100644
> index 0000000..c4d2928
> --- /dev/null
> +++ b/shared/strtoint.h
> @@ -0,0 +1,72 @@
> +/*
> + * Copyright © 2015 Samsung Electronics Co., Ltd

I think your calendar needs new batteries :)

> + *
> + * Permission is hereby granted, free of charge, to any person obtaining
> + * a copy of this software and associated documentation files (the
> + * "Software"), to deal in the Software without restriction, including
> + * without limitation the rights to use, copy, modify, merge, publish,
> + * distribute, sublicense, and/or sell copies of the Software, and to
> + * permit persons to whom the Software is furnished to do so, subject to
> + * the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> + * next paragraph) shall be included in all copies or substantial
> + * portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT.  IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#ifndef WESTON_STRTOINT_H
> +#define WESTON_STRTOINT_H
> +
> +#ifdef  __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <errno.h>
> +
> +/* Convert string to integer
> + *
> + * Parses a base-10 number from the given string.  Checks that the
> + * string is not blank, contains only numerical characters, and is
> + * within the range of -INT_MAX to INT_MAX.  If the validation is
> + * successful the result is stored in *value.
> + *
> + * Upon error, this routine does not modify or set errno, and leaves
> + * *value unchanged.

tbh, I think this is unnecessary. anyone who relies on errno to remain set
across multiple function calls is optimistic at best.

> + *
> + * \returns true if number parsed successfully, false on error

I think it's just \return (no s), isn't it?

> + */
> +static inline bool
> +strtoint(const char *str, int *value)
> +{
> +	int ret;
> +	char *end;
> +	int prev_errno = errno;
> +
> +	errno = 0;
> +	ret = strtol(str, &end, 10);
> +
> +	if (errno != 0 || end == str || *end != '\0') {

if you don't care about the specific value of errno anyway, there's no need
to check it here since we can rely on ret only. That makes the code simpler,
you can just set errno = prev_errno immediately after the strtol call.

also, there's an argument for an assert(str != NULL) but I'm not going to
fight too hard for that.

Cheers,
   Peter

> +		errno = prev_errno;
> +		return false;
> +	}
> +
> +	*value = ret;
> +	errno = prev_errno;
> +
> +	return true;
> +}
> +
> +#ifdef  __cplusplus
> +}
> +#endif
> +
> +#endif /* WESTON_STRTOINT_H */
> -- 
> 1.9.1