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

Efstratios Karatzas gpf.kira at
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>
To: bug-followup at
Cc: Bruce Evans <brde at>
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

 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> 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=
 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=
 > 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=
 > strtonum() is well placed to do this, but it only prints a generic messag=
 > 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.
 "vmstat: -n -1: less than 0" (0 being the min value we want in this scenari=
 would be a good error message.
 The same goes for the error message for a longer than our max supplied valu=
 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=
 > 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=
 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=
 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 =
 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=
 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,
 Efstratios "GPF" Karatzas
 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

More information about the freebsd-bugs mailing list