Re: [patch] have rtprio check that arguments are numeric; change atoi to strtol
On Sun, 2 Jan 2011 18:46:47 -0500, Eitan Adler wrote:
> What about this patch? I incorporated your feedback so I am not going
> to reply inline.
Since the pattern of converting strings to int-derivative values appears
multiple times, I'd probably prefer something like a new function that
does the parsing *and* error-checking, to avoid duplication of errno
checks, invalidchar checks, and so on, e.g. something like this near the
top of rtprio.c:
#include
#include
#include
/*
* Parse an integer from 'string' into 'value'. Return the first
* invalid character in 'endp', if endp is non-NULL. The return value
* of parseint is 0 on success, -1 for any parse error.
*/
int
parseint(const char *string, const char **endp, int base, int *value)
{
long x;
errno = 0;
x = strtol(string, endp, base);
if (errno != 0 || endp == str || (endp != NULL &&
endp != str && *endp != '\0' && (isdigit(*endp) == 0 ||
isspace(*endp) != 0)))
return -1;
if (x >= INT_MAX) {
errno = ERANGE;
return -1;
}
return (int)x;
}
Then you can replace all the atoi() calls with:
int x;
if (parseint(argv[1], NULL, 0, &x) != 0)
errx(1, "your message");
which is a relatively cleaner way of handling strtol() parsing errors,
without the associated clutter of checking properly all possible edge
cases at every call-point.
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [patch] have rtprio check that arguments are numeric; change atoi to strtol
On Tue, 04 Jan 2011 11:36:38 +0100, Giorgos Keramidas
wrote:
> On Sun, 2 Jan 2011 18:46:47 -0500, Eitan Adler wrote:
>> What about this patch? I incorporated your feedback so I am not going
>> to reply inline.
>
> Since the pattern of converting strings to int-derivative values appears
> multiple times, I'd probably prefer something like a new function that
> does the parsing *and* error-checking, to avoid duplication of errno
> checks, invalidchar checks, and so on, e.g. something like this near the
> top of rtprio.c:
>
> #include
> #include
> #include
>
> /*
> * Parse an integer from 'string' into 'value'. Return the first
> * invalid character in 'endp', if endp is non-NULL. The return value
> * of parseint is 0 on success, -1 for any parse error.
> */
>
> int
> parseint(const char *string, const char **endp, int base, int *value)
> {
> long x;
>
> errno = 0;
> x = strtol(string, endp, base);
> if (errno != 0 || endp == str || (endp != NULL &&
> endp != str && *endp != '\0' && (isdigit(*endp) == 0 ||
> isspace(*endp) != 0)))
> return -1;
> if (x >= INT_MAX) {
> errno = ERANGE;
> return -1;
> }
> return (int)x;
> }
instead of `return (int)x' the last bits should be slightly different,
of course:
if (value != NULL)
*value = (int)x;
return 0;
}
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [patch] have rtprio check that arguments are numeric; change atoi to strtol
On Tue, Jan 04, 2011 at 11:40:45AM +0100, Giorgos Keramidas wrote:
> On Tue, 04 Jan 2011 11:36:38 +0100, Giorgos Keramidas
> wrote:
> > On Sun, 2 Jan 2011 18:46:47 -0500, Eitan Adler wrote:
> >> What about this patch? I incorporated your feedback so I am not going
> >> to reply inline.
> >
> > Since the pattern of converting strings to int-derivative values appears
> > multiple times, I'd probably prefer something like a new function that
> > does the parsing *and* error-checking, to avoid duplication of errno
> > checks, invalidchar checks, and so on, e.g. something like this near the
> > top of rtprio.c:
> >
> > #include
> > #include
> > #include
> >
> > /*
> > * Parse an integer from 'string' into 'value'. Return the first
> > * invalid character in 'endp', if endp is non-NULL. The return
> > value
> > * of parseint is 0 on success, -1 for any parse error.
> > */
> >
> > int
> > parseint(const char *string, const char **endp, int base, int
> > *value)
> > {
> > long x;
> >
> > errno = 0;
> > x = strtol(string, endp, base);
> > if (errno != 0 || endp == str || (endp != NULL &&
> > endp != str && *endp != '\0' && (isdigit(*endp) == 0 ||
> > isspace(*endp) != 0)))
> > return -1;
> > if (x >= INT_MAX) {
> > errno = ERANGE;
> > return -1;
> > }
> > return (int)x;
> > }
>
> instead of `return (int)x' the last bits should be slightly different,
> of course:
>
> if (value != NULL)
> *value = (int)x;
> return 0;
> }
Well, I think it shall be simplified. What about this ?
diff --git a/usr.sbin/rtprio/rtprio.c b/usr.sbin/rtprio/rtprio.c
index 245f714..fc212b5 100644
--- a/usr.sbin/rtprio/rtprio.c
+++ b/usr.sbin/rtprio/rtprio.c
@@ -37,31 +37,31 @@ __FBSDID("$FreeBSD$");
#include
#include
-#include
#include
#include
+#include
#include
#include
#include
#include
-static void usage();
+static int parseint(const char *, const char *);
+static void usage(void);
int
-main(argc, argv)
- int argc;
- char **argv;
+main(int argc, char *argv[])
{
- char *p;
- int proc = 0;
struct rtprio rtp;
+ char *p;
+ pid_t proc;
/* find basename */
if ((p = rindex(argv[0], '/')) == NULL)
p = argv[0];
else
++p;
+ proc = 0;
if (!strcmp(p, "rtprio"))
rtp.type = RTP_PRIO_REALTIME;
@@ -70,12 +70,12 @@ main(argc, argv)
switch (argc) {
case 2:
- proc = abs(atoi(argv[1])); /* Should check if numeric
-* arg! */
+ proc = parseint(argv[1], "pid");
+ proc = abs(proc);
/* FALLTHROUGH */
case 1:
if (rtprio(RTP_LOOKUP, proc, &rtp) != 0)
- err(1, "%s", argv[0]);
+ err(1, "RTP_LOOKUP");
printf("%s: ", p);
switch (rtp.type) {
case RTP_PRIO_REALTIME:
@@ -103,19 +103,17 @@ main(argc, argv)
usage();
break;
}
- } else {
- rtp.prio = atoi(argv[1]);
- }
+ } else
+ rtp.prio = parseint(argv[1], "priority");
} else {
usage();
break;
}
if (argv[2][0] == '-')
- proc = -atoi(argv[2]);
-
+ proc = parseint(argv[2] + 1, "pid");
if (rtprio(RTP_SET, proc, &rtp) != 0)
- err(1, "%s", argv[0]);
+ err(1, "RTP_SET");
if (proc == 0) {
execvp(argv[2], &argv[2]);
@@ -123,12 +121,28 @@ main(argc, argv)
}
exit(0);
}
- exit (1);
+ exit(1);
+}
+
+static int
+parseint(const char *str, const char *errname)
+{
+ char *endp;
+ long res;
+
+ errno = 0;
+ res = strtol(str, &endp, 10);
+ if (errno != 0 || endp == str || *endp != '\0')
+ err(1, "%s shall be a number", errname);
+ if (res >= INT_MAX)
+ err(1, "Integer overflow parsing %s", errname);
+ return (res);
}
static void
-usage()
+usage(void)
{
+
(void) fprintf(stderr, "%s\n%s\n%s\n%s\n%s\n%s\n",
"usage: [id|rt]prio",
" [id|rt]prio [-]pid",
pgpRlkNXRGHNm.pgp
Description: PGP sign
Re: [patch] have rtprio check that arguments are numeric; change atoi to strtol
On Tue, 4 Jan 2011 13:25:02 +0200, Kostik Belousov wrote:
>On Tue, Jan 04, 2011 at 11:40:45AM +0100, Giorgos Keramidas wrote:
>>On Tue, 04 Jan 2011 11:36:38 +0100, Giorgos Keramidas
>>wrote:
>>> Since the pattern of converting strings to int-derivative values appears
>>> multiple times, I'd probably prefer something like a new function that
>>> does the parsing *and* error-checking, to avoid duplication of errno
>>> checks, invalidchar checks, and so on, e.g. something like this near the
>>> top of rtprio.c:
>>>
>>> /*
>>> * Parse an integer from 'string' into 'value'. Return the first
>>> * invalid character in 'endp', if endp is non-NULL. The return
>>> value
>>> * of parseint is 0 on success, -1 for any parse error.
>>> */
>>>
>>> int
>>> parseint(const char *string, const char **endp, int base, int
>>> *value)
>>> ...
> }
>
> Well, I think it shall be simplified. What about this ?
This looks much better than plain strtol() calls. Thanks :)
> diff --git a/usr.sbin/rtprio/rtprio.c b/usr.sbin/rtprio/rtprio.c
> index 245f714..fc212b5 100644
> --- a/usr.sbin/rtprio/rtprio.c
> +++ b/usr.sbin/rtprio/rtprio.c
> @@ -37,31 +37,31 @@ __FBSDID("$FreeBSD$");
>
> #include
> #include
> -#include
>
> #include
> #include
> +#include
> #include
> #include
> #include
> #include
>
> -static void usage();
> +static int parseint(const char *, const char *);
> +static void usage(void);
>
> int
> -main(argc, argv)
> - int argc;
> - char **argv;
> +main(int argc, char *argv[])
> {
> - char *p;
> - int proc = 0;
> struct rtprio rtp;
> + char *p;
> + pid_t proc;
>
> /* find basename */
> if ((p = rindex(argv[0], '/')) == NULL)
> p = argv[0];
> else
> ++p;
> + proc = 0;
>
> if (!strcmp(p, "rtprio"))
> rtp.type = RTP_PRIO_REALTIME;
> @@ -70,12 +70,12 @@ main(argc, argv)
>
> switch (argc) {
> case 2:
> - proc = abs(atoi(argv[1])); /* Should check if numeric
> - * arg! */
> + proc = parseint(argv[1], "pid");
> + proc = abs(proc);
> /* FALLTHROUGH */
> case 1:
> if (rtprio(RTP_LOOKUP, proc, &rtp) != 0)
> - err(1, "%s", argv[0]);
> + err(1, "RTP_LOOKUP");
> printf("%s: ", p);
> switch (rtp.type) {
> case RTP_PRIO_REALTIME:
> @@ -103,19 +103,17 @@ main(argc, argv)
> usage();
> break;
> }
> - } else {
> - rtp.prio = atoi(argv[1]);
> - }
> + } else
> + rtp.prio = parseint(argv[1], "priority");
> } else {
> usage();
> break;
> }
>
> if (argv[2][0] == '-')
> - proc = -atoi(argv[2]);
> -
> + proc = parseint(argv[2] + 1, "pid");
> if (rtprio(RTP_SET, proc, &rtp) != 0)
> - err(1, "%s", argv[0]);
> + err(1, "RTP_SET");
>
> if (proc == 0) {
> execvp(argv[2], &argv[2]);
> @@ -123,12 +121,28 @@ main(argc, argv)
> }
> exit(0);
> }
> - exit (1);
> + exit(1);
> +}
> +
> +static int
> +parseint(const char *str, const char *errname)
> +{
> + char *endp;
> + long res;
> +
> + errno = 0;
> + res = strtol(str, &endp, 10);
> + if (errno != 0 || endp == str || *endp != '\0')
> + err(1, "%s shall be a number", errname);
> + if (res >= INT_MAX)
> + err(1, "Integer overflow parsing %s", errname);
> + return (res);
> }
>
> static void
> -usage()
> +usage(void)
> {
> +
> (void) fprintf(stderr, "%s\n%s\n%s\n%s\n%s\n%s\n",
> "usage: [id|rt]prio",
> " [id|rt]prio [-]pid",
pgp01wK7MOYI9.pgp
Description: PGP signature
Re: [patch] have rtprio check that arguments are numeric; change atoi to strtol
On Tuesday, January 04, 2011 6:25:02 am Kostik Belousov wrote:
> On Tue, Jan 04, 2011 at 11:40:45AM +0100, Giorgos Keramidas wrote:
> @@ -123,12 +121,28 @@ main(argc, argv)
> }
> exit(0);
> }
> - exit (1);
> + exit(1);
> +}
> +
> +static int
> +parseint(const char *str, const char *errname)
> +{
> + char *endp;
> + long res;
> +
> + errno = 0;
> + res = strtol(str, &endp, 10);
> + if (errno != 0 || endp == str || *endp != '\0')
> + err(1, "%s shall be a number", errname);
Small nit, maybe use 'must' instead of 'shall'.
--
John Baldwin
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [patch] have rtprio check that arguments are numeric; change atoi to strtol
On Tue Jan 4 11, John Baldwin wrote:
> On Tuesday, January 04, 2011 6:25:02 am Kostik Belousov wrote:
> > On Tue, Jan 04, 2011 at 11:40:45AM +0100, Giorgos Keramidas wrote:
> > @@ -123,12 +121,28 @@ main(argc, argv)
> > }
> > exit(0);
> > }
> > - exit (1);
> > + exit(1);
> > +}
> > +
> > +static int
> > +parseint(const char *str, const char *errname)
> > +{
> > + char *endp;
> > + long res;
> > +
> > + errno = 0;
> > + res = strtol(str, &endp, 10);
> > + if (errno != 0 || endp == str || *endp != '\0')
> > + err(1, "%s shall be a number", errname);
>
> Small nit, maybe use 'must' instead of 'shall'.
it seems at some point there has been a massive usage of the term 'shall' in
manual pages, which people tried to get rid of. hence the
'usr/share/examples/mdoc/deshallify.sh' script.
maybe this should be noted in style(9)?
cheers.
alex
>
> --
> John Baldwin
--
a13x
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [patch] have rtprio check that arguments are numeric; change atoi to strtol
On Jan 4, 2011, at 5:49 AM, Alexander Best wrote:
> On Tue Jan 4 11, John Baldwin wrote:
>> On Tuesday, January 04, 2011 6:25:02 am Kostik Belousov wrote:
>>> On Tue, Jan 04, 2011 at 11:40:45AM +0100, Giorgos Keramidas wrote:
>>> @@ -123,12 +121,28 @@ main(argc, argv)
>>> }
>>> exit(0);
>>> }
>>> - exit (1);
>>> + exit(1);
>>> +}
>>> +
>>> +static int
>>> +parseint(const char *str, const char *errname)
>>> +{
>>> + char *endp;
>>> + long res;
>>> +
>>> + errno = 0;
>>> + res = strtol(str, &endp, 10);
>>> + if (errno != 0 || endp == str || *endp != '\0')
>>> + err(1, "%s shall be a number", errname);
>>
>> Small nit, maybe use 'must' instead of 'shall'.
>
> it seems at some point there has been a massive usage of the term 'shall' in
> manual pages, which people tried to get rid of. hence the
> 'usr/share/examples/mdoc/deshallify.sh' script.
I know shall is used widely by opengroup when describing definitions and
interfaces in the POSIX standards, but the connotation in English is very
squishy, so I agree with John that must would be better.
BTW, only if errno was non-zero would using err(3) be logical. Otherwise it
will just produce noise :).
> maybe this should be noted in style(9)?
Thanks,
-Garrett___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [patch] have rtprio check that arguments are numeric; change atoi to strtol
On Tue, 4 Jan 2011 08:58:48 -0800, Garrett Cooper wrote: + errno = 0; + res = strtol(str, &endp, 10); + if (errno != 0 || endp == str || *endp != '\0') + err(1, "%s shall be a number", errname); >>> >>> Small nit, maybe use 'must' instead of 'shall'. >> >> it seems at some point there has been a massive usage of the term >> 'shall' in manual pages, which people tried to get rid of. hence the >> 'usr/share/examples/mdoc/deshallify.sh' script. > > I know shall is used widely by opengroup when describing definitions > and interfaces in the POSIX standards, but the connotation in English > is very squishy, so I agree with John that must would be better. > > BTW, only if errno was non-zero would using err(3) be > logical. Otherwise it will just produce noise :). That's a good point. I think we should change err() to errx() there. ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
Re: [patch] have rtprio check that arguments are numeric; change atoi to strtol
On Jan 4, 2011, at 10:12 AM, Giorgos Keramidas wrote: > On Tue, 4 Jan 2011 08:58:48 -0800, Garrett Cooper wrote: > + errno = 0; > + res = strtol(str, &endp, 10); > + if (errno != 0 || endp == str || *endp != '\0') > + err(1, "%s shall be a number", errname); Small nit, maybe use 'must' instead of 'shall'. >>> >>> it seems at some point there has been a massive usage of the term >>> 'shall' in manual pages, which people tried to get rid of. hence the >>> 'usr/share/examples/mdoc/deshallify.sh' script. >> >> I know shall is used widely by opengroup when describing definitions >> and interfaces in the POSIX standards, but the connotation in English >> is very squishy, so I agree with John that must would be better. >> >> BTW, only if errno was non-zero would using err(3) be >> logical. Otherwise it will just produce noise :). > > That's a good point. I think we should change err() to errx() there. kib was quick and already did it in r216967. Cheers! -Garrett___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to "freebsd-hackers-unsubscr...@freebsd.org"
