[patch] have rtprio check that arguments are numeric; change atoi to strtol

[Kostik Belousov]

On Sun, Jan 02, 2011 at 02:41:10AM -0500, Eitan Adler wrote:
> > Just set the second argument to strtol to something non-NULL and then check
> > the value returned; that will help provide the error handling with
> > simplicity that you desire :).
> 
> How about this version? It also corrects a copy/paste error I have above
> 
> Index: rtprio.c
> ===================================================================
> --- rtprio.c    (revision 216679)
> +++ rtprio.c    (working copy)
> @@ -56,6 +56,7 @@
>        char   *p;
>        int     proc = 0;
>        struct rtprio rtp;
While there, you may change the type of proc to pid_t.
Also, move the initialization of proc out of local declaration section,
according to style(9).

> +       char *invalidChar;
We do not use camelCase, according to style(9).

> 
>        /* find basename */
>        if ((p = rindex(argv[0], '/')) == NULL)
> @@ -70,8 +71,9 @@
> 
>        switch (argc) {
>        case 2:
> -               proc = abs(atoi(argv[1]));      /* Should check if numeric
> -                                                * arg! */
> +               proc = abs((int)strtol(argv[1], &invalidChar, 10));
Why is the cast needed there ?
Also, I think that doing
	proc = strtol();
	if (*invalid_char ...)
		...;
	proc = abs(proc);

> +               if (*invalidChar != '\0')
> +                       errx(1,"Process should be a number");
Probably, "Pid" or "Process id" instead of "Process".

>                /* FALLTHROUGH */
>        case 1:
>                if (rtprio(RTP_LOOKUP, proc, &rtp) != 0)
> @@ -104,16 +106,20 @@
>                                        break;
>                                }
>                        } else {
> -                               rtp.prio = atoi(argv[1]);
> +                               rtp.prio = (int)strtol(argv[1],
> &invalidChar, 10);
> +                               if (*invalidChar != '\0')
> +                                       errx(1,"Priority should be a
> number", invalidChar);
I suspect that the line overflows 80 characters limit. Consider wrapping
it or unindenting.

>                        }
>                } else {
>                        usage();
>                        break;
>                }
> 
> -               if (argv[2][0] == '-')
> -                       proc = -atoi(argv[2]);
> -
> +               if (argv[2][0] == '-') {
> +                       proc = -(int)strtol(argv[2], &invalidChar, 10);
The easier solution would be
	proc = strtol(argv[2] + 1, &invalid_char, 10);
?
> +                       if (*invalidChar != '\0')
> +                               errx(1,"Process should be a number");
Again, Pid.
> +               }
>                if (rtprio(RTP_SET, proc, &rtp) != 0)
>                        err(1, "%s", argv[0]);
> 
The syntax of the prio commands is weird, there is an obvious corner
(or wrongly handled) case, where the command name starts with a digit.
Command name starting with dash is even harder.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-hackers/attachments/20110102/650ee420/attachment.pgp