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

[Efstratios Karatzas]

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: Mon, 18 Jan 2010 15:33:48 +0200

 On Mon, Jan 18, 2010 at 5:55 AM, Bruce Evans <brde at optusnet.com.au> wrote:
 > On Sun, 17 Jan 2010, Efstratios Karatzas <gpf.kira at gmail.com> wrote:
 >
 >>> Description:
 >>
 >> vmstat(8) -w should produce an error message and exit when fed a negativ=
 e
 >> numerical value or a non numerical value at all, in which case atoi simp=
 ly
 >> returns 0. This is the way iostat(8) handles this situation.
 >>
 >> If we do not check for a negative value, then the negative value we are
 >> fed becomes an extremely large unsigned int and the thread will sleep(3)=
  for
 >> a long time indeed.
 >
 > Please use line lengths of considerably less than 168 characters. =C2=A0I=
 'm
 > editing this with a slightly wrong $TERMCAP and the line wrap from the
 > long lines is even more horrible than usual.
 
 Sorry about that, won't happen again.
 
 >
 > There is another bad atoi() for the wait interval, in the
 > BACKWARD_COMPATIBILITY ifdef, which is a non-optional option.
 >
 > There are other bad atoi()s in the file. =C2=A0One has a bounds check and=
  neither
 > has a type error to break negative values.
 >
 
 One pr, one bug fix. Let's see if we can get anywhere with this first.
 
 >>> Fix:
 >>
 >> apply my patch, all we need is a simple check if the value is less than =
 1.
 >> This way an error message also occurs if we could not parse a number, si=
 nce
 >> the return value in that case is 0.
 >
 > -w 0 used to sort of work -- it was equivalent to not specifying an
 > interval. =C2=A0Maybe some scripts or fingers depend on this. =C2=A0Proba=
 bly lots
 > still depends on the undocumented BACKWARD_COMPATIBILITY behaviour
 > (vmstat 1 =3D=3D vmstat -w 1), so this non-optional option can never be
 > removed.
 >
 > The other bad atoi()s don't use this trick, so they give a garbage result
 > for parse errors.
 >
 > Bruce
 >
 
 I still can't understand why would anyone use "vmstat 0" or "vmstat -w 0".
 It's the same output if you just use "vmstat".
 I am aware of the BACKWARD_COMPATIBILITY behavior but negative values
 don't work with it, for example "vmstat -3" produces an illegal option erro=
 r.
 Although "vmstat abc" will work just fine but it is not much of a problem.
 
 We could perform a more "sophisticated" error check.
 Elaboration:
 
 A call to atoi(3) is actually a call to strtol(3).
 strtol(3) will return 0 and set errno to EINVAL if no conversion could be
 performed.
 eg "vmstat -w abc"
 So we could check the value of errno when atoi() returns 0.
 But the man page states that this feature is not portable across all platfo=
 rms!
 
 We could also just accept the "vmstat -w string" behavior as well as the
 "vmstat -w 0" and try to fix only the negative values which are clearly bug=
 s.
 
 To sum up, I'm willing to rewrite my patch again and again and supply
 more patches for other bugs as long as they don't remain forever in the
 PR database uncommented and *if* they are ok, un-commited. I read the
 quarterly status report yesterday and the relative wiki pages and I 'm
 starting to get a feeling of what is going on with the process of bugbustin=
 g and
 I also feel I must say thank you to everyone who's contributing to this
 part of the project. Rock on!
 
 So, any more input anyone?
 
 Thank you for your time,
 
 --=20
 
 Efstratios "GPF" Karatzas