From owner-freebsd-hackers@FreeBSD.ORG Sun Jan 2 10:38:40 2011 Return-Path: Delivered-To: hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 53E74106566C for ; Sun, 2 Jan 2011 10:38:40 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.zoral.com.ua (mx0.zoral.com.ua [91.193.166.200]) by mx1.freebsd.org (Postfix) with ESMTP id DE4DE8FC0C for ; Sun, 2 Jan 2011 10:38:39 +0000 (UTC) Received: from deviant.kiev.zoral.com.ua (root@deviant.kiev.zoral.com.ua [10.1.1.148]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id p02AIjuv009104 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 2 Jan 2011 12:18:45 +0200 (EET) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.4/8.14.4) with ESMTP id p02AIj3L093251; Sun, 2 Jan 2011 12:18:45 +0200 (EET) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.4/8.14.4/Submit) id p02AIjbE093250; Sun, 2 Jan 2011 12:18:45 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Sun, 2 Jan 2011 12:18:45 +0200 From: Kostik Belousov To: Eitan Adler Message-ID: <20110102101845.GC90883@deviant.kiev.zoral.com.ua> References: <6BEFA669-2E1F-44E6-897A-0A51DA939A74@gmail.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="62BerGhzGk0sq9XA" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: clamav-milter 0.95.2 at skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-3.4 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_00, DNS_FROM_OPENWHOIS autolearn=no version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua Cc: hackers@freebsd.org Subject: Re: [patch] have rtprio check that arguments are numeric; change atoi to strtol X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 02 Jan 2011 10:38:40 -0000 --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--