From owner-freebsd-bugs@FreeBSD.ORG Thu Jan 21 18:50:03 2010 Return-Path: Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 48B73106566B for ; Thu, 21 Jan 2010 18:50:03 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id 28F748FC13 for ; Thu, 21 Jan 2010 18:50:03 +0000 (UTC) Received: from freefall.freebsd.org (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.3/8.14.3) with ESMTP id o0LIo254091841 for ; Thu, 21 Jan 2010 18:50:02 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.14.3/8.14.3/Submit) id o0LIo2vG091840; Thu, 21 Jan 2010 18:50:02 GMT (envelope-from gnats) Date: Thu, 21 Jan 2010 18:50:02 GMT Message-Id: <201001211850.o0LIo2vG091840@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: Efstratios Karatzas Cc: Subject: Re: bin/142911: [patch] vmstat(8) -w should produce error message if fed a negative value X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Efstratios Karatzas List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Jan 2010 18:50:03 -0000 The following reply was made to PR bin/142911; it has been noted by GNATS. From: Efstratios Karatzas To: bug-followup@freebsd.org Cc: Bruce Evans 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 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--