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

[Bruce Evans]

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

From: Bruce Evans <brde at optusnet.com.au>
To: Efstratios Karatzas <gpf.kira at gmail.com>
Cc: bug-followup at FreeBSD.org, 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 19:07:35 +1100 (EST)

 On Wed, 20 Jan 2010, Efstratios Karatzas wrote:
 
 > 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, ...
 > correctly and the error checking is kinda ugly.
 > 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.  I would use INT_MAX, etc.
 
 % --- vmstat.c	2010-01-20 17:12:09.000000000 +0200
 % +++ vmstat.orig.c	2010-01-20 16:58:56.000000000 +0200
 
 The patch is reversed.  This can be confusing.
 
 % @@ -196,9 +195,7 @@
 %  			aflag++;
 %  			break;
 %  		case 'c':
 % -			reps = (int)strtonum(optarg, 0, 10000, &errstr);
 
 I don't like casting the return value, just to silence broken compilers/
 CFLAGS.  strtonum() 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).
 
 % -			if (errstr != NULL)
 % -				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.
 
 % @@ -226,9 +223,10 @@
 %  			break;
 %  		case 'n':
 %  			nflag = 1;
 % -			maxshowdevs = (int)strtonum(optarg, 0, 1000, &errstr);
 % -			if (errstr != NULL)
 % -				errx(1, "-n %s: %s", optarg, errstr); 
 % +			maxshowdevs = atoi(optarg);
 % +			if (maxshowdevs < 0)
 % +				errx(1, "number of devices %d is < 0",
 % +				     maxshowdevs);
 
 Your error messages are a bit too cryptic.  The old one here is better.
 
 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.  Only
 strtonum() is well placed to do this, but it only prints a generic message.
 
 %  			break;
 %  		case 'p':
 %  			if (devstat_buildmatch(optarg, &matches, &num_matches) != 0)
 % @@ -302,14 +298,9 @@
 %  #define	BACKWARD_COMPATIBILITY
 %  #ifdef	BACKWARD_COMPATIBILITY
 %  	if (*argv) {
 % -		interval = (unsigned int)strtonum(*argv, 0, 3600, &errstr);
 % -		if (errstr != NULL)
 % -			errx(1, "wait time %s: %s", *argv, errstr);
 % -		if (*++argv) {
 % -			reps = (int)strtonum(*argv, 0, 10000, &errstr);
 % -			if (errstr != NULL)
 % -				errx(1, "iterations %s: %s", *argv, errstr);
 % -		}
 % +		interval = atoi(*argv);
 % +		if (*++argv)
 % +			reps = atoi(*argv);
 %  	}
 %  #endif
 %
 
 strtonum() is still painful to use after adding error handling, sigh.
 
 The above messages are less cryptic than the correspnding ones for -w
 and -n.  I prefer them for -w and -n too.
 
 Concerning strtonum()'s bad API, I prefer something like:
 
 uintmax_t strtoix(const char restrict *nptr, intmax_t min, uintmax_t max,
                    const char restrict *errfmt, ...);
 
 so that the above can become:
 
  		interval = strtoix(*argv, 0, 3600, "wait time %s: %m", *argv);
  		if (*++argv)
  			reps = strtoix(*argv, 0, 10000, "iterations %s: %m",
  			    *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, ...).  A 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/ERANGE
 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().
 
 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.
 
 Bruce