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

Thu Jan 21 18:50:03 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: Thu, 21 Jan 2010 20:41:13 +0200

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

 On Thu, Jan 21, 2010 at 10:07 AM, Bruce Evans <brde at optusnet.com.au> wrote:
 > On Wed, 20 Jan 2010, Efstratios Karatzas wrote:
 >
 >> 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
 >
 > I don't like arbitrary limits. =C2=A0I would use INT_MAX, etc.

 Although huge values don't make any sense, we should not put values in code
 like that. I should have used a #define MAX_TIME 3600 or something like tha=
 t.
 Now it's INT_MAX and UINT_MAX respectively.

 >
 > % --- vmstat.c =C2=A02010-01-20 17:12:09.000000000 +0200
 > % +++ vmstat.orig.c =C2=A0 =C2=A0 2010-01-20 16:58:56.000000000 +0200
 >
 > The patch is reversed. =C2=A0This can be confusing.

 Sorry about that!

 >
 > % @@ -196,9 +195,7 @@
 > % =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
 =C2=A0 aflag++;
 > % =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
 =C2=A0 break;
 > % =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 case 'c':
 > % - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
  reps =3D (int)strtonum(optarg, 0, 10000, &errstr);
 >
 > I don't like casting the return value, just to silence broken compilers/
 > CFLAGS. =C2=A0strtonum() is supposed to be easy to use, but it is not so
 > easy to use if you have to put unnecessary type info in or near it.
 > If I wanted to do that then I would want an interface with the type
 > in the name, like the one that this is replacing (atoi() for ints, and
 > strtol() for longs, but unfortunately no strtoi() for ints).

 For the record, I don't think that casting here makes this code any less
 neat or readable, although now that you point it out it does provide no
 extra helpful information to the person reading the code, maybe
 less is better. I did remove them in this version. Out of curiosity, why
 is there no strtoi()? A lot of code still uses atoi() and this way we would
 lose the long to int conversion problem that you mentioned in 64bit archs.

 >
 > % - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
  if (errstr !=3D NULL)
 > % - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
  =C2=A0 =C2=A0 =C2=A0 =C2=A0 errx(1, "-c %s: %s", optarg, errstr);
 >
 > The previous line has lots of trailing white space which looks like an
 > extra blank line after line wrap.

 No more white space trails this time.

 >
 > % @@ -226,9 +223,10 @@
 > % =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
 =C2=A0 break;
 > % =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 case 'n':
 > % =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
 =C2=A0 nflag =3D 1;
 > % - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
  maxshowdevs =3D (int)strtonum(optarg, 0, 1000,
 > &errstr);
 > % - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
  if (errstr !=3D NULL)
 > % - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
  =C2=A0 =C2=A0 =C2=A0 =C2=A0 errx(1, "-n %s: %s", optarg, errstr); % +
 > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 maxshowdev=
 s =3D atoi(optarg);
 > % + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
  if (maxshowdevs < 0)
 > % + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
  =C2=A0 =C2=A0 =C2=A0 =C2=A0 errx(1, "number of devices %d is < 0",
 > % + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
  =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0maxshowdevs);
 >
 > Your error messages are a bit too cryptic. =C2=A0The old one here is bett=
 er.
 >
 > Old: "vmstat: number of devices -1 is < 0"
 > New: "vmstat: -n -1: too small"
 >
 > Not too bad, but it is better to print the limit that was not met. =C2=A0=
 Only
 > strtonum() is well placed to do this, but it only prints a generic messag=
 e.
 >
 > The above messages are less cryptic than the correspnding ones for -w
 > and -n. =C2=A0I prefer them for -w and -n too.

 I do think that the original messages for -w and -c are cryptic. But after
 going through a lot of *BSD code, I thought that for some strange reason
 this was the norm (meaning cryptic error messages and code comments).
 I guess it's just because a lot of code was written a long time ago.
 Anyhoo, the current error messages should be "ok" but keep reading,
 there's more on that.

 > % =C2=A0#define =C2=A0 =C2=A0 =C2=A0BACKWARD_COMPATIBILITY
 > % =C2=A0#ifdef =C2=A0 =C2=A0 =C2=A0 BACKWARD_COMPATIBILITY
 > % =C2=A0 =C2=A0 =C2=A0 if (*argv) {
 > % - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 interval =3D (unsigned int)=
 strtonum(*argv, 0, 3600, &errstr);
 > % - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (errstr !=3D NULL)
 > % - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
  errx(1, "wait time %s: %s", *argv, errstr);
 > % - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (*++argv) {
 > % - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
  reps =3D (int)strtonum(*argv, 0, 10000, &errstr);
 > % - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
  if (errstr !=3D NULL)
 > % - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
  =C2=A0 =C2=A0 =C2=A0 =C2=A0 errx(1, "iterations %s: %s", *argv, errstr);
 > % - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 }
 > % + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 interval =3D atoi(*argv);
 > % + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (*++argv)
 > % + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
  reps =3D atoi(*argv);
 > % =C2=A0 =C2=A0 =C2=A0 }
 > % =C2=A0#endif
 > %
 >
 > strtonum() is still painful to use after adding error handling, sigh.
 >

 Well not *that* painful for simple line argument parsing, i believe
 it will do for this case and I certainly prefer it to the strtol() solution=
 .
 What I don't like are the generic error messages of strtonum(). For example=
 :

 > Old: "vmstat: number of devices -1 is < 0"
 > New: "vmstat: -n -1: too small"

 Instead of printing the limits that are acceptable here through errx(),
 we could try and change the error message that strtonum() returns and
 solve this ugliness at its root. That would be the right thing to do imho,
 but I believe I need a commiter that agrees with me before I go around
 posting PRs just to change error strings in openbsd code because *I*
 don't like them.

 eg
 "vmstat: -n -1: less than 0" (0 being the min value we want in this scenari=
 o)
 would be a good error message.

 The same goes for the error message for a longer than our max supplied valu=
 e.

 What do you think?

 >
 > Concerning strtonum()'s bad API, I prefer something like:
 >
 > uintmax_t strtoix(const char restrict *nptr, intmax_t min, uintmax_t max,
 > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0const char =
 restrict *errfmt, ...);
 >
 > so that the above can become:
 >
 > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0interval =3D strto=
 ix(*argv, 0, 3600, "wait time %s: %m",
 > *argv);
 > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (*++argv)
 > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
 =A0 =C2=A0reps =3D strtoix(*argv, 0, 10000, "iterations %s: %m",
 > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
 =A0 =C2=A0 =C2=A0 =C2=A0*argv);
 >
 > , plus many variants to regain control over standard features of the
 > strto*() family: (char **endptr, int base, long double fmax, int
 > typectl, long double fmin, long double *fresultptr, int errctl) and
 > add extensions (int multiplier_ctl, int unit_ctl, ...). =C2=A0A version
 > that can handle either integers or floating point in input, and output
 > both (e.g., decimal integer <1 followed by 100 zeros> -> UINTMAX_MAX/ERAN=
 GE
 > together with 1e100/no_fp_err) necessarily takes a lot of parameters,
 > but this is still easier to use than building a general number parser
 > out of strtouimax() and strtold().
 >

 A few days ago I was reading netbsd's mailing list and the topic at the tim=
 e
 was if they were going to port the strtonum() or not. A popular opinion was
 that we don't need yet another function to parse numbers, so I don't know
 how other fbsd devs feel about it. But all of our existing options have the=
 ir
 shortcomings and no single function family is panacea, that is true.

 > Concerning implementation errors in strtonum(): it does undocumented
 > clobbering of errno: it sets errno to 0 even when there is no error.
 > This particular clobbering is explicitly forbidden for any Standard C
 > Library function.
 >

 I wasn't aware of this, I have yet to read to source code of strtonum() to =
 be
 honest. I was going to post a pr so that we could add the extra "caveats"
 section of the openbsd man page for atoi(3) to the freebsd man page and als=
 o
 include strtonum() in the "see also" section of atoi.
 I could file another pr so that the freebsd man page for strtonum() will
 contain this information about errno though.

 Thank you for your time,

 --=20

 Efstratios "GPF" Karatzas

 --00032555563632c592047db10ced
 Content-Type: application/octet-stream; name="patch-3.diff"
 Content-Disposition: attachment; filename="patch-3.diff"
 Content-Transfer-Encoding: base64
 X-Attachment-Id: f_g4pv4q0k0

 LS0tIHZtc3RhdC5vcmlnLmMJMjAxMC0wMS0yMSAyMDoxMzowOS4wMDAwMDAwMDAgKzAyMDAKKysr
 IHZtc3RhdC5jCTIwMTAtMDEtMjEgMjA6MTM6MDMuMDAwMDAwMDAwICswMjAwCkBAIC0xODIsNiAr
 MTgyLDcgQEAKIAlpbnQgYywgdG9kbzsKIAl1bnNpZ25lZCBpbnQgaW50ZXJ2YWw7CiAJaW50IHJl
 cHM7CisJY29uc3QgY2hhciAqZXJyc3RyOwogCWNoYXIgKm1lbWYsICpubGlzdGY7CiAJY2hhciBl
 cnJidWZbX1BPU0lYMl9MSU5FX01BWF07CiAKQEAgLTE5NSw3ICsxOTYsOSBAQAogCQkJYWZsYWcr
 KzsKIAkJCWJyZWFrOwogCQljYXNlICdjJzoKLQkJCXJlcHMgPSBhdG9pKG9wdGFyZyk7CisJCQly
 ZXBzID0gc3RydG9udW0ob3B0YXJnLCAwLCBJTlRfTUFYLCAmZXJyc3RyKTsKKwkJCWlmIChlcnJz
 dHIgIT0gTlVMTCkKKwkJCQllcnJ4KDEsICJpdGVyYXRpb25zICVzOiAlcyIsIG9wdGFyZywgZXJy
 c3RyKTsKIAkJCWJyZWFrOwogCQljYXNlICdQJzoKIAkJCVBmbGFnKys7CkBAIC0yMjMsMTAgKzIy
 Niw5IEBACiAJCQlicmVhazsKIAkJY2FzZSAnbic6CiAJCQluZmxhZyA9IDE7Ci0JCQltYXhzaG93
 ZGV2cyA9IGF0b2kob3B0YXJnKTsKLQkJCWlmIChtYXhzaG93ZGV2cyA8IDApCi0JCQkJZXJyeCgx
 LCAibnVtYmVyIG9mIGRldmljZXMgJWQgaXMgPCAwIiwKLQkJCQkgICAgIG1heHNob3dkZXZzKTsK
 KwkJCW1heHNob3dkZXZzID0gc3RydG9udW0ob3B0YXJnLCAwLCBJTlRfTUFYLCAmZXJyc3RyKTsK
 KwkJCWlmIChlcnJzdHIgIT0gTlVMTCkKKwkJCQllcnJ4KDEsICJudW1iZXIgb2YgZGV2aWNlcyAl
 czogJXMiLCBvcHRhcmcsIGVycnN0cik7CiAJCQlicmVhazsKIAkJY2FzZSAncCc6CiAJCQlpZiAo
 ZGV2c3RhdF9idWlsZG1hdGNoKG9wdGFyZywgJm1hdGNoZXMsICZudW1fbWF0Y2hlcykgIT0gMCkK
 QEAgLTI0Myw3ICsyNDUsOSBAQAogI2VuZGlmCiAJCQlicmVhazsKIAkJY2FzZSAndyc6Ci0JCQlp
 bnRlcnZhbCA9IGF0b2kob3B0YXJnKTsKKwkJCWludGVydmFsID0gc3RydG9udW0ob3B0YXJnLCAw
 LCBVSU5UX01BWCwgJmVycnN0cik7CisJCQlpZiAoZXJyc3RyICE9IE5VTEwpCisJCQkJZXJyeCgx
 LCAid2FpdCB0aW1lICVzOiAlcyIsIG9wdGFyZywgZXJyc3RyKTsKIAkJCWJyZWFrOwogCQljYXNl
 ICd6JzoKIAkJCXRvZG8gfD0gWk1FTVNUQVQ7CkBAIC0yOTgsOSArMzAyLDE0IEBACiAjZGVmaW5l
 CUJBQ0tXQVJEX0NPTVBBVElCSUxJVFkKICNpZmRlZglCQUNLV0FSRF9DT01QQVRJQklMSVRZCiAJ
 aWYgKCphcmd2KSB7Ci0JCWludGVydmFsID0gYXRvaSgqYXJndik7Ci0JCWlmICgqKythcmd2KQot
 CQkJcmVwcyA9IGF0b2koKmFyZ3YpOworCQlpbnRlcnZhbCA9IHN0cnRvbnVtKCphcmd2LCAwLCBV
 SU5UX01BWCwgJmVycnN0cik7CisJCWlmIChlcnJzdHIgIT0gTlVMTCkKKwkJCWVycngoMSwgIndh
 aXQgdGltZSAlczogJXMiLCAqYXJndiwgZXJyc3RyKTsKKwkJaWYgKCorK2FyZ3YpIHsKKwkJCXJl
 cHMgPSBzdHJ0b251bSgqYXJndiwgMCwgSU5UX01BWCwgJmVycnN0cik7CisJCQlpZiAoZXJyc3Ry
 ICE9IE5VTEwpCisJCQkJZXJyeCgxLCAiaXRlcmF0aW9ucyAlczogJXMiLCAqYXJndiwgZXJyc3Ry
 KTsKKwkJfQogCX0KICNlbmRpZgogCg==
 --00032555563632c592047db10ced--