On Tue, Mar 08, 2011 at 08:50:44AM -0800, Jason Gerecke wrote:
> Do additional checking to ensure the user provides valid values
> for boolean parameters. Also, provide a helpful error if the user
> goes astray.
>
> Signed-off-by: Jason Gerecke <killert...@gmail.com>
> ---
>
> Changes to previous version:
> - convert_value_from_user now returns conversion success and requires a
> return pointer
> - atoi() swapped to strtol() for better error checking
> - error message more generic
> - extra crazy to safely handle memory de/allocation
>
> tools/xsetwacom.c | 51 ++++++++++++++++++++++++++++++++++++++++-----------
> 1 files changed, 40 insertions(+), 11 deletions(-)
>
> diff --git a/tools/xsetwacom.c b/tools/xsetwacom.c
> index fa98e3e..04feed5 100644
> --- a/tools/xsetwacom.c
> +++ b/tools/xsetwacom.c
> @@ -24,6 +24,8 @@
> #include <wacom-properties.h>
> #include "Xwacom.h"
>
> +#include <errno.h>
> +#include <limits.h>
> #include <stdio.h>
> #include <stdarg.h>
> #include <ctype.h>
> @@ -1420,18 +1422,30 @@ error:
> return;
> }
>
> -static int convert_value_from_user(param_t *param, char *value)
> +static Bool convert_value_from_user(param_t *param, char *value, int
> *return_value)
> {
> - int val;
> + if (!return_value)
> + return False;
>
> - if ((param->prop_flags & PROP_FLAG_BOOLEAN) && strcmp(value, "off") ==
> 0)
> - val = 0;
> - else if ((param->prop_flags & PROP_FLAG_BOOLEAN) && strcmp(value, "on")
> == 0)
> - val = 1;
> - else
> - val = atoi(value);
> + if (param->prop_flags & PROP_FLAG_BOOLEAN)
> + {
> + if (strcmp(value, "off") == 0)
> + *return_value = 0;
> + else if (strcmp(value, "on") == 0)
> + *return_value = 1;
> + else
> + return False;
> + } else {
> + char *end;
> + long conversion = strtol(value, &end, 10);
> + if (end == value || *end != '\0' || errno == ERANGE ||
> + conversion < INT_MIN || conversion > INT_MAX)
> + return False;
> +
> + *return_value = (int)conversion;
> + }
there are two different patches in this hunk alone. please don't merge
logical changes into one patch, it makes it hard to revert, review and
figure out the reasoning. I realise this patch is simple enough but better
be strict on it.
There are at least two patches in here:
- boolean value handling fixes (which requires new syntax and the new code
below
- the atoi → strtol change
Now, good form would be to change the function signature first (leaving the
buggy boolean code). then the boolean change, then the strtol change.
and because of the recent doxygen requirement, if you touch a function
signature, you now also have to document it. bad luck for you there ;)
I'd appreciated it if you could add tests as well to make sure that the
input/output values work as expected now. see the bottom of xsetwacom.c, all
the test_foo(void) functions.
the changes itself looks fine though, thanks.
>
> - return val;
> + return True;
> }
>
> static void set(Display *dpy, int argc, char **argv)
> @@ -1507,7 +1521,22 @@ static void set(Display *dpy, int argc, char **argv)
>
> for (i = 0; i < nvals; i++)
> {
> - val = convert_value_from_user(param, values[i]);
> + int *retval = calloc(1,sizeof(int));
> + if (!retval)
> + {
> + fprintf(stderr, "calloc failled\n");
typo.
> + exit(EXIT_FAILURE);
> + }
> + Bool success = convert_value_from_user(param, values[i],
> retval);
> + int val = *retval;
old C style please, no declarations after code.
Cheers,
Peter
> + free(retval);
> +
> + if (!success)
> + {
> + fprintf(stderr, "'%s' is not a valid value for the '%s'
> property.\n",
> + values[i], param->name);
> + goto out;
> + }
>
> switch(param->prop_format)
> {
> --
> 1.7.1
>
>
>
------------------------------------------------------------------------------
Colocation vs. Managed Hosting
A question and answer guide to determining the best fit
for your organization - today and in the future.
http://p.sf.net/sfu/internap-sfd2d
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
