[procps] Re: [PATCH 1/2] top: restore terminal state on exit
- From: Sami Kerola <kerolasa@xxxxxx>
- To: procps@xxxxxxxxxxxxx
- Date: Tue, 1 May 2012 12:22:17 +0200
On Tue, May 1, 2012 at 10:01, Jim Warner <james.warner@xxxxxxxxxxx> 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.
>
> Mike,
>
> Here are the reasons I objected to your patches...
>
> 1) While probably not a factor with /proc, I prefer to not use a function
> like your access() whose own documentation suggests "use of this system
> call should be avoided".
I thought using access() is fine.
http://pubs.opengroup.org/onlinepubs/9699919799/functions/access.html
BTW what documentation discourages using access()?
> 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.
Hold on if you 'tolerated' the patch because it was needed and correct, not
because of it was wrote by me. IMHO that should be only criteria of (not)
accepting a patch.
By correct I mean; it does what it is supposed to, understandable, clean way
without being in conflict with rest of the project.
> 3) You violated the alphabetical order of functions within sections when
> you added restore_terminal(). And while that doesn't impact the executable
> in the least, I consider the current discipline a useful programmer
> challenge.
> commit 270e8e7eeb66b47569940f537cdad4ab46cd36be
>
> 4) You failed to update the pseudo prototypes that have been carefully
> maintained in top.h. I don't know if anyone else has ever used that
> documentation for a quick program overview or source code navigation, but
> even as the program author I have.
> commit efb9fcb81604941a28532f9df841eb93bde7f653
> commit 270e8e7eeb66b47569940f537cdad4ab46cd36be
>
> 5) You completely ignored nls support with your new message. The existing
> library message emitted under my approach also bypasses nls but, unlike
> your message, there is precious little to translate with the library text.
> commit e64b6f70d7c99db01d7270c27f09de5b02cacebb
> commit db88a8d2420664ab108d6aa5400f2cea6ac660d5
I am afraid you failed to encourage people to contribute this project. If
there are rules of alphabetical order, pseudo prototypes, nls support and so
on
1. Document the rules.
2. If new committer does not follow the rules say 'thank you', modify the
proposed patch to be perfect and resubmit with a your 'Signed-off-by:' line
added to it.
Ok, back to actual patch. If I do not read wrong Mike is proposing adding
robustness. To me that sounds good idea, even if the main() would gain yet
another code line. There does seem to be few thing which need to be correct
in patch, but they do make the proposal incorrect. Right?
--
Sami Kerola
http://www.iki.fi/kerolasa/
Other related posts:
