Re: [PATCH weston v3 4/5] Add safe_strtoint() helper

[Eric Engestrom]

On Mon, Aug 01, 2016 at 12:48:21PM +1000, Peter Hutterer wrote:
> On Fri, Jul 29, 2016 at 09:43:59AM -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 and a uint32_t return by
> > reference.
> > 
> > Signed-off-by: Bryce Harrington 
> > Reviewed-by: Thiago Macieira 
> > ---
> >  shared/string-helpers.h | 78 
> > +
> >  1 file changed, 78 insertions(+)
> >  create mode 100644 shared/string-helpers.h
> > 
> > diff --git a/shared/string-helpers.h b/shared/string-helpers.h
> > new file mode 100644
> > index 000..0617229
> > --- /dev/null
> > +++ b/shared/string-helpers.h
> > @@ -0,0 +1,78 @@
> > +/*
> > + * Copyright © 2016 Samsung Electronics Co., Ltd
> > + *
> > + * 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_STRING_HELPERS_H
> > +#define WESTON_STRING_HELPERS_H
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> 
> do we need this in an internal helper?
> 
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +/* 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

s/-INT_MAX/INT_MIN/, and if I'm being pedantic it should be
"INT32_MIN to INT32_MAX" since you're using int32_t, not int :)

> > + * successful the result is stored in *value; otherwise *value is
> > + * unchanged and errno is set appropriately.
> > + *
> > + * \return true if number parsed successfully, false on error
> 
> "if the number". IMO you should squash the safe_strtoint() tests into this
> patch and have a separate patch for switching the existing calls over. 5/5
> looks like a rebase gone wrong.
> 
> with that, Reviewed-by: Peter Hutterer 

Agreed, and you can also have my r-b (for the whole series):
Reviewed-by: Eric Engestrom 

> 
> Cheers,
>Peter
> 
> > + */
> > +static inline bool
> > +safe_strtoint(const char *str, int32_t *value)
> > +{
> > +   long ret;
> > +   char *end;
> > +
> > +   assert(str != NULL);
> > +
> > +   errno = 0;
> > +   ret = strtol(str, , 10);
> > +   if (errno != 0) {
> > +   return false;
> > +   } else if (end == str || *end != '\0') {
> > +   errno = EINVAL;
> > +   return false;
> > +   }
> > +
> > +   *value = (int32_t)ret;
> > +   if ((long)*value != ret) {
> > +   errno = ERANGE;
> > +   return false;
> > +   }
> > +
> > +   return true;
> > +}
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* WESTON_STRING_HELPERS_H */
> > -- 
> > 1.9.1
> > 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v3 4/5] Add safe_strtoint() helper

[Peter Hutterer]

On Fri, Jul 29, 2016 at 09:43:59AM -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 and a uint32_t return by
> reference.
> 
> Signed-off-by: Bryce Harrington 
> Reviewed-by: Thiago Macieira 
> ---
>  shared/string-helpers.h | 78 
> +
>  1 file changed, 78 insertions(+)
>  create mode 100644 shared/string-helpers.h
> 
> diff --git a/shared/string-helpers.h b/shared/string-helpers.h
> new file mode 100644
> index 000..0617229
> --- /dev/null
> +++ b/shared/string-helpers.h
> @@ -0,0 +1,78 @@
> +/*
> + * Copyright © 2016 Samsung Electronics Co., Ltd
> + *
> + * 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_STRING_HELPERS_H
> +#define WESTON_STRING_HELPERS_H
> +
> +#ifdef   __cplusplus
> +extern "C" {
> +#endif

do we need this in an internal helper?

> +
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/* 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; otherwise *value is
> + * unchanged and errno is set appropriately.
> + *
> + * \return true if number parsed successfully, false on error

"if the number". IMO you should squash the safe_strtoint() tests into this
patch and have a separate patch for switching the existing calls over. 5/5
looks like a rebase gone wrong.

with that, Reviewed-by: Peter Hutterer 

Cheers,
   Peter

> + */
> +static inline bool
> +safe_strtoint(const char *str, int32_t *value)
> +{
> + long ret;
> + char *end;
> +
> + assert(str != NULL);
> +
> + errno = 0;
> + ret = strtol(str, , 10);
> + if (errno != 0) {
> + return false;
> + } else if (end == str || *end != '\0') {
> + errno = EINVAL;
> + return false;
> + }
> +
> + *value = (int32_t)ret;
> + if ((long)*value != ret) {
> + errno = ERANGE;
> + return false;
> + }
> +
> + return true;
> +}
> +
> +#ifdef   __cplusplus
> +}
> +#endif
> +
> +#endif /* WESTON_STRING_HELPERS_H */
> -- 
> 1.9.1
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston v3 4/5] Add safe_strtoint() helper

[Bryce Harrington]

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 and a uint32_t return by
reference.

Signed-off-by: Bryce Harrington 
Reviewed-by: Thiago Macieira 
---
 shared/string-helpers.h | 78 +
 1 file changed, 78 insertions(+)
 create mode 100644 shared/string-helpers.h

diff --git a/shared/string-helpers.h b/shared/string-helpers.h
new file mode 100644
index 000..0617229
--- /dev/null
+++ b/shared/string-helpers.h
@@ -0,0 +1,78 @@
+/*
+ * Copyright © 2016 Samsung Electronics Co., Ltd
+ *
+ * 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_STRING_HELPERS_H
+#define WESTON_STRING_HELPERS_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include 
+#include 
+#include 
+#include 
+
+/* 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; otherwise *value is
+ * unchanged and errno is set appropriately.
+ *
+ * \return true if number parsed successfully, false on error
+ */
+static inline bool
+safe_strtoint(const char *str, int32_t *value)
+{
+   long ret;
+   char *end;
+
+   assert(str != NULL);
+
+   errno = 0;
+   ret = strtol(str, , 10);
+   if (errno != 0) {
+   return false;
+   } else if (end == str || *end != '\0') {
+   errno = EINVAL;
+   return false;
+   }
+
+   *value = (int32_t)ret;
+   if ((long)*value != ret) {
+   errno = ERANGE;
+   return false;
+   }
+
+   return true;
+}
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* WESTON_STRING_HELPERS_H */
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel