Date: Sun, 2 Jan 2011 12:18:45 +0200 From: Kostik Belousov <kostikbel@gmail.com> To: Eitan Adler <lists@eitanadler.com> Cc: hackers@freebsd.org Subject: Re: [patch] have rtprio check that arguments are numeric; change atoi to strtol Message-ID: <20110102101845.GC90883@deviant.kiev.zoral.com.ua> In-Reply-To: <AANLkTimdEpZ1DBoKU9g6w-j=F9KS0ONvo9racgOesdPf@mail.gmail.com> References: <AANLkTimiJPiHBSw5i5TVJYfh9uGOyrNJx%2BoUPeB%2Bt%2BY_@mail.gmail.com> <6BEFA669-2E1F-44E6-897A-0A51DA939A74@gmail.com> <AANLkTimmQHRoQOZBgfheds=_sv5SMzD9DrPfYk9em7j8@mail.gmail.com> <AANLkTimdEpZ1DBoKU9g6w-j=F9KS0ONvo9racgOesdPf@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--62BerGhzGk0sq9XA
Content-Type: text/plain; charset=koi8-r
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
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 c=
heck
> > the value returned; that will help provide the error handling with
> > simplicity that you desire :).
>=20
> How about this version? It also corrects a copy/paste error I have above
>=20
> Index: rtprio.c
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
> --- rtprio.c =9A =9A(revision 216679)
> +++ rtprio.c =9A =9A(working copy)
> @@ -56,6 +56,7 @@
> =9A =9A =9A =9Achar =9A *p;
> =9A =9A =9A =9Aint =9A =9A proc =3D 0;
> =9A =9A =9A =9Astruct 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).
> + =9A =9A =9A char *invalidChar;
We do not use camelCase, according to style(9).
>=20
> =9A =9A =9A =9A/* find basename */
> =9A =9A =9A =9Aif ((p =3D rindex(argv[0], '/')) =3D=3D NULL)
> @@ -70,8 +71,9 @@
>=20
> =9A =9A =9A =9Aswitch (argc) {
> =9A =9A =9A =9Acase 2:
> - =9A =9A =9A =9A =9A =9A =9A proc =3D abs(atoi(argv[1])); =9A =9A =9A/* =
Should check if numeric
> - =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A=
=9A =9A =9A =9A =9A =9A* arg! */
> + =9A =9A =9A =9A =9A =9A =9A proc =3D abs((int)strtol(argv[1], &invalidC=
har, 10));
Why is the cast needed there ?
Also, I think that doing
proc =3D strtol();
if (*invalid_char ...)
...;
proc =3D abs(proc);
> + =9A =9A =9A =9A =9A =9A =9A if (*invalidChar !=3D '\0')
> + =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A errx(1,"Process should be a=
number");
Probably, "Pid" or "Process id" instead of "Process".
> =9A =9A =9A =9A =9A =9A =9A =9A/* FALLTHROUGH */
> =9A =9A =9A =9Acase 1:
> =9A =9A =9A =9A =9A =9A =9A =9Aif (rtprio(RTP_LOOKUP, proc, &rtp) !=3D 0)
> @@ -104,16 +106,20 @@
> =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =
=9A =9Abreak;
> =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A}
> =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A} else {
> - =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A rtp.prio =
=3D atoi(argv[1]);
> + =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A rtp.prio =
=3D (int)strtol(argv[1],
> &invalidChar, 10);
> + =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A if (*invali=
dChar !=3D '\0')
> + =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A=
=9A errx(1,"Priority should be a
> number", invalidChar);
I suspect that the line overflows 80 characters limit. Consider wrapping
it or unindenting.
> =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A}
> =9A =9A =9A =9A =9A =9A =9A =9A} else {
> =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9Ausage();
> =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9Abreak;
> =9A =9A =9A =9A =9A =9A =9A =9A}
>=20
> - =9A =9A =9A =9A =9A =9A =9A if (argv[2][0] =3D=3D '-')
> - =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A proc =3D -atoi(argv[2]);
> -
> + =9A =9A =9A =9A =9A =9A =9A if (argv[2][0] =3D=3D '-') {
> + =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A proc =3D -(int)strtol(argv[=
2], &invalidChar, 10);
The easier solution would be
proc =3D strtol(argv[2] + 1, &invalid_char, 10);
?
> + =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A if (*invalidChar !=3D '\0')
> + =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A errx(1,"Pro=
cess should be a number");
Again, Pid.
> + =9A =9A =9A =9A =9A =9A =9A }
> =9A =9A =9A =9A =9A =9A =9A =9Aif (rtprio(RTP_SET, proc, &rtp) !=3D 0)
> =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9A =9Aerr(1, "%s", argv[0]);
>=20
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.
--62BerGhzGk0sq9XA
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (FreeBSD)
iEYEARECAAYFAk0gUQQACgkQC3+MBN1Mb4ipKwCgpBcLWn6311tDaL+sdRjE57l5
CyQAn1ZahHOjHjBJtikaViSvgNNd9Qv4
=WbWg
-----END PGP SIGNATURE-----
--62BerGhzGk0sq9XA--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110102101845.GC90883>