Date: Thu, 21 Jan 2010 08:10:07 GMT From: Bruce Evans <brde@optusnet.com.au> To: freebsd-bugs@FreeBSD.org Subject: Re: bin/142911: [patch] vmstat(8) -w should produce error message if fed a negative value Message-ID: <201001210810.o0L8A7aY034862@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR bin/142911; it has been noted by GNATS. From: Bruce Evans <brde@optusnet.com.au> To: Efstratios Karatzas <gpf.kira@gmail.com> Cc: bug-followup@FreeBSD.org, Bruce Evans <brde@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201001210810.o0L8A7aY034862>