[procps] Re: [PATCH 1/2] top: restore terminal state on exit

  • From: Mike Frysinger <vapier@xxxxxxxxxx>
  • To: procps@xxxxxxxxxxxxx
  • Date: Tue, 1 May 2012 11:57:57 -0400

On Tuesday 01 May 2012 04:01:49 Jim Warner wrote:
> On Apr 30, 2012, at 12:33 PM, Mike Frysinger wrote:
> > as i said in a private e-mail:
> > i still think the atexit hook should be added regardless of any other
> > /proc testing top acquires.  assuming the code will always handle all
> > edge cases all the time and will never unexpectedly exit is naïve.
> > 
> > also, i'd point out that "amount of code threw at the problem" is a bit
> > melodramatic.  i added 1 new function call, and split out code that
> > already existed in the bye_bye() function into a dedicated
> > restore_terminal() function.
> 
> Here are the reasons I objected to your patches...

you seem to be confusing things

> 1) While probably not a factor with /proc, I prefer to not use a function
> like your access()

i didn't say add that patch.  i said add these two patches neither of which 
use access().

> whose own documentation suggests "use of this system
> call should be avoided".

i think you misread the documentation or don't understand the context.  that 
description is specifically for TOCTTOU vulnerabilities which has no bearing at 
all in this situation.
        http://en.wikipedia.org/wiki/Time_of_check_to_time_of_use

> 2) You disrespected my main function with unnecessay bloat.  I've tried to
> keep main() as short as possible, concentrating on the run-time loop.  Low
> level start up details were encapsulated in other functions.  And while I
> tolerated Sami's recent atexit() addition, the code you added more
> properly belongs in the before() routine.

this is laughable.  the overhead of the single new atexit call is meaningless 
here.  it tweaks an array of pointers which is significantly faster than any of 
the startup code in top itself.

> 3) You violated the alphabetical order of functions within sections when
> you added restore_terminal().
> 4) You failed to update the pseudo prototypes that have been carefully
> maintained in top.h.

this is mechanical drudgery that you failed to highlight.  it's trivial for 
you to respond and say "please keep the list sorted" and "please keep the 
comments in top.h up-to-date".  although the latter looks almost wholly 
pointless and could easily be accomplished with a script.

> 5) You completely ignored nls support with your new message.

again, you're looking at the wrong patch
-mike

Other related posts: