--3V7upXqbjpZ4EhLz
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Jun-ichiro itojun Hagino wrote:
=20
> 	more error check to strtoul().
=20
> itojun
=20
=20
> RCS file: /cvsroot/src/sbin/disklabel/disklabel.c,v
> retrieving revision 1.126
> diff -u -r1.126 disklabel.c
> --- disklabel.c	18 Jan 2004 22:34:22 -0000	1.126
> +++ disklabel.c	28 Feb 2004 21:44:12 -0000
> @@ -1458,13 +1458,15 @@
>  			continue;
>  		}
>  		if (!strcmp(cp, "total sectors")) {
> -			v =3D atoi(tp);
> -			if (v <=3D 0) {
> +			char *ep;
> +			errno =3D 0;
> +			lp->d_secperunit =3D strtoul(tp, &ep, 10);

I suggest a "if (isdigit((unsigned char)*tp))" first.

> +			if (*tp =3D=3D '\0' || !ep || *ep !=3D '\0' ||

> +			    errno =3D=3D ERANGE) {
>  				warnx("line %d: bad %s: %s", lineno, cp, tp);

Double-quotes around the last %s would make it much easier to notice
trailing whitespaces.

>  				errors++;
>  			} else
> -				lp->d_secperunit =3D v;
> -			continue;
> +				continue;
>  		}
>  		if (!strcmp(cp, "rpm")) {
>  			v =3D atoi(tp);

Eh, is invalid input unimportant here? Sorry, I really don't see why
anybody would use atoi(), atol() etc. unless you already know that
the given parameter is a valid value within the correct range. These
functions are *crap*, period.

> @@ -1538,19 +1540,22 @@
>  		break;						\
>  	}
> =20
> -#define NXTNUM(n) do { \
> +/* cannot use do-while due to the use of "break" in _CHECKLINE */
> +#define NXTNUM(n) { \
>  	_CHECKLINE						\
>  	cp =3D tp, tp =3D word(cp), (n) =3D (cp !=3D NULL ? atoi(cp) : 0);	\
> -} while (/* CONSTCOND */ 0)
> +}
> =20
> -#define NXTXNUM(n) do { \
> +/* cannot use do-while due to the use of "break" in _CHECKLINE */
> +#define NXTXNUM(n) { \
>  	char	*ptr;							\
> -	int	 m;							\
> +	u_int32_t m;							\

Wouldn't it be better to use uintmax_t here?

>  									\
>  	_CHECKLINE							\
>  	cp =3D tp, tp =3D word(cp);						\
> -	m =3D (int)strtol(cp, &ptr, 10);					\
> -	if (*ptr =3D=3D '\0')						\
> +	errno =3D 0;							\
> +	m =3D strtoul(cp, &ptr, 10);					\

unsigned long isn't necessarily an alias for u_int32_t and strtoul() happily
accepts negative values.

> +	if (*cp && ptr && *ptr =3D=3D '\0' && errno =3D=3D 0)			\

Hmm, if it's save to assume (errno =3D=3D 0) in case of a successful strtou=
l()
you might want to shorten the other checks to "errno" instead of
"errno =3D=3D ERANGE". Otherwise, I'd change this to "errno !=3D ERANGE".

The same applies to the rest of the patch resp. disklabel.c.

--=20
Christian
=20
As you can see, this a signature. It's not related to the contents of the
mail in any way. But you probably won't listen to me anyway, will you?

--3V7upXqbjpZ4EhLz
Content-Type: application/pgp-signature
Content-Disposition: inline

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (NetBSD)

iD8DBQFAQR1K0KQix3oyIMcRAiI6AKDBBzwVZ72oehBh5Vjhp5zD+wnYdACgxhPh
qWy3V3sw4j8Q/E84aLAiygc=
=Ygu/
-----END PGP SIGNATURE-----

--3V7upXqbjpZ4EhLz--