bin/142911: [patch] vmstat(8) -w should produce error message if fed a negative value

Wed Jan 20 15:50:09 UTC 2010

The following reply was made to PR bin/142911; it has been noted by GNATS.

From: Efstratios Karatzas <gpf.kira at gmail.com>
To: bug-followup at freebsd.org
Cc: Bruce Evans <brde at optusnet.com.au>
Subject: Re: bin/142911: [patch] vmstat(8) -w should produce error message if 
	fed a negative value
Date: Wed, 20 Jan 2010 17:42:28 +0200

 --0016e6d78426099cbe047d9a6fc2
 Content-Type: text/plain; charset=UTF-8
 Content-Transfer-Encoding: quoted-printable

 On Tue, Jan 19, 2010 at 4:04 PM, Bruce Evans <brde at optusnet.com.au> wrote:

 > This behaviour should never be depended on. =C2=A0Just use strtol() with
 > normal error checking (check that the end pointer has advanced and
 > doesn't point to garbage). =C2=A0errno doesn't need to be checked for thi=
 s.
 > However, errno needs to be set to !=3D ERANGE before the call and checked
 > =3D=3D ERANGE after the call to check for range errors. =C2=A0Then the re=
 sult
 > must be checked to fit in the int or unsigned variable.
 >
 > FreeBSD has 2 nonstandard badly designed interfaces that are intended
 > to be easier to use than strto*() and atoi(): strtonum() and
 > expand_number(). =C2=A0Their bad design starts with then not supporting
 > intmax_t, uintmax_t or any floating point type. =C2=A0strtonum() doesn't
 > even support unsigned longs on 64-bit arches, and it uses the long
 > long abomination. =C2=A0But it is an adequate replacement for strtol(),
 > and thus OK for replacing atoi() in most command option parsing.
 >
 > Another issue is whether numeric command options should be limited to
 > decimal values representable as a long or even as an int. =C2=A0I think t=
 hey
 > shouldn't have any arbitrary limits, but POSIX may over-specify them as
 > being decimal and/or of limited size. =C2=A0Another design bug in strtonu=
 m()
 > is that it only supports decimal numbers. =C2=A0This makes it more compat=
 ible
 > with atoi() but less useful than it should be.

 Thanks for the input Bruce.

 I studied the manpages of the aforementioned alternatives to atoi and strto=
 *.
 I also examined code from OpenBSD and NetBSD to see how they deal with
 this kind of problem. My conclusion is that strtonum() is the best choise
 when dealing  with simple argument parsing, strtol() is just difficult  to =
 use
 correctly and the error checking is kinda ugly.

 So I applied a "strtonum fix" to all of the atoi()s in vmstat.c to
 save some time.
 Checked the program with various input and it seems ok.
 I also took style(9) into account.

 The only thing I don't like about this fix is the self-imposed limit on the
 values we accept.
 min limit for everything is 0
 -w max is an hour
 -c max is 10 000
 -n max is 1000

 Although the openbsd devs don't seem to mind it, for example the max limit
 for vmstat -w of openbsd is 16,6 mins. If you think that's not acceptable,
 I'm willing to write a strtol fix.

 But it definitely looks better now,

 Cheers

 --=20

 Efstratios "GPF" Karatzas

 --0016e6d78426099cbe047d9a6fc2
 Content-Type: application/octet-stream; name="patch-2.diff"
 Content-Disposition: attachment; filename="patch-2.diff"
 Content-Transfer-Encoding: base64
 X-Attachment-Id: f_g4o9u6zz0

 LS0tIHZtc3RhdC5jCTIwMTAtMDEtMjAgMTc6MTI6MDkuMDAwMDAwMDAwICswMjAwCisrKyB2bXN0
 YXQub3JpZy5jCTIwMTAtMDEtMjAgMTY6NTg6NTYuMDAwMDAwMDAwICswMjAwCkBAIC0xODIsNyAr
 MTgyLDYgQEAKIAlpbnQgYywgdG9kbzsKIAl1bnNpZ25lZCBpbnQgaW50ZXJ2YWw7CiAJaW50IHJl
 cHM7Ci0JY29uc3QgY2hhciAqZXJyc3RyOwogCWNoYXIgKm1lbWYsICpubGlzdGY7CiAJY2hhciBl
 cnJidWZbX1BPU0lYMl9MSU5FX01BWF07CiAKQEAgLTE5Niw5ICsxOTUsNyBAQAogCQkJYWZsYWcr
 KzsKIAkJCWJyZWFrOwogCQljYXNlICdjJzoKLQkJCXJlcHMgPSAoaW50KXN0cnRvbnVtKG9wdGFy
 ZywgMCwgMTAwMDAsICZlcnJzdHIpOwotCQkJaWYgKGVycnN0ciAhPSBOVUxMKQotCQkJCWVycngo
 MSwgIi1jICVzOiAlcyIsIG9wdGFyZywgZXJyc3RyKTsJCQkKKwkJCXJlcHMgPSBhdG9pKG9wdGFy
 Zyk7CiAJCQlicmVhazsKIAkJY2FzZSAnUCc6CiAJCQlQZmxhZysrOwpAQCAtMjI2LDkgKzIyMywx
 MCBAQAogCQkJYnJlYWs7CiAJCWNhc2UgJ24nOgogCQkJbmZsYWcgPSAxOwotCQkJbWF4c2hvd2Rl
 dnMgPSAoaW50KXN0cnRvbnVtKG9wdGFyZywgMCwgMTAwMCwgJmVycnN0cik7Ci0JCQlpZiAoZXJy
 c3RyICE9IE5VTEwpCi0JCQkJZXJyeCgxLCAiLW4gJXM6ICVzIiwgb3B0YXJnLCBlcnJzdHIpOwkK
 KwkJCW1heHNob3dkZXZzID0gYXRvaShvcHRhcmcpOworCQkJaWYgKG1heHNob3dkZXZzIDwgMCkK
 KwkJCQllcnJ4KDEsICJudW1iZXIgb2YgZGV2aWNlcyAlZCBpcyA8IDAiLAorCQkJCSAgICAgbWF4
 c2hvd2RldnMpOwogCQkJYnJlYWs7CiAJCWNhc2UgJ3AnOgogCQkJaWYgKGRldnN0YXRfYnVpbGRt
 YXRjaChvcHRhcmcsICZtYXRjaGVzLCAmbnVtX21hdGNoZXMpICE9IDApCkBAIC0yNDUsOSArMjQz
 LDcgQEAKICNlbmRpZgogCQkJYnJlYWs7CiAJCWNhc2UgJ3cnOgotCQkJaW50ZXJ2YWwgPSAodW5z
 aWduZWQgaW50KXN0cnRvbnVtKG9wdGFyZywgMCwgMzYwMCwgJmVycnN0cik7Ci0JCQlpZiAoZXJy
 c3RyICE9IE5VTEwpCi0JCQkJZXJyeCgxLCAiLXcgJXM6ICVzIiwgb3B0YXJnLCBlcnJzdHIpOwkJ
 CQorCQkJaW50ZXJ2YWwgPSBhdG9pKG9wdGFyZyk7CiAJCQlicmVhazsKIAkJY2FzZSAneic6CiAJ
 CQl0b2RvIHw9IFpNRU1TVEFUOwpAQCAtMzAyLDE0ICsyOTgsOSBAQAogI2RlZmluZQlCQUNLV0FS
 RF9DT01QQVRJQklMSVRZCiAjaWZkZWYJQkFDS1dBUkRfQ09NUEFUSUJJTElUWQogCWlmICgqYXJn
 dikgewotCQlpbnRlcnZhbCA9ICh1bnNpZ25lZCBpbnQpc3RydG9udW0oKmFyZ3YsIDAsIDM2MDAs
 ICZlcnJzdHIpOwotCQlpZiAoZXJyc3RyICE9IE5VTEwpCi0JCQllcnJ4KDEsICJ3YWl0IHRpbWUg
 JXM6ICVzIiwgKmFyZ3YsIGVycnN0cik7Ci0JCWlmICgqKythcmd2KSB7Ci0JCQlyZXBzID0gKGlu
 dClzdHJ0b251bSgqYXJndiwgMCwgMTAwMDAsICZlcnJzdHIpOwotCQkJaWYgKGVycnN0ciAhPSBO
 VUxMKQotCQkJCWVycngoMSwgIml0ZXJhdGlvbnMgJXM6ICVzIiwgKmFyZ3YsIGVycnN0cik7Ci0J
 CX0KKwkJaW50ZXJ2YWwgPSBhdG9pKCphcmd2KTsKKwkJaWYgKCorK2FyZ3YpCisJCQlyZXBzID0g
 YXRvaSgqYXJndik7CiAJfQogI2VuZGlmCiAK
 --0016e6d78426099cbe047d9a6fc2--