Review Request: Fix hang in kcm_useraccount
Rolf Eike Beer
kde at opensource.sf-tec.de
Thu Aug 9 08:06:39 BST 2012
Michael Palimaka wrote:
> On 2012-08-09 02:59, Rolf Eike Beer wrote:
> > Now we have the following situation in case the program has exited (if not
> > the behavior is unchanged):
> > -we read in one line, if it is empty we break. What happens if the first
> > line of output is empty and correct output comes later?
> That's why I originally went for readAll.
You could have just said that instead of taking my suggestions ;)
> > -the line is not empty, we continue (new check)
> > -we check if the line is empty (old check)
> > So, what now?
> > I would suggest the following:
> > -readAll()
> > -if empty, break
> > -find a linebreak, if we have one: unreadLine for everything beyond it,
> > cut
> > the input line we have
> > -move the readLine() and the isEmpty() of the old code together in the
> > else
> > (i.e. pidExited == NotExited).
> Isn't that what effectively what the original revision does (dropping
> the unread since readLine after readAll is safe)?
> If the program has exited:
> - Read all output
> - If no output is found, break
> - If output is found, continue along the original code
> The purpose of this change is only to break if the program has exited
> without output.
You have a nice way trying to say I'm a moron. Thank you. ;)
Your patch isn't technically ideal as it does the same operation on data
multiple times. But seriously: we have wasted way more time and computing
power by doing this mail thread then the most ideal code solution will ever be
able to return.
Just one thing: you do unreadLine(line), which adds "true" as second argument,
so a needless newline is appended that wasn't there before (and which causes a
deep copy of all that stuff for nothing). So unreadLine(line, false) should it
be. And I will not speak up again on this unless explicitely requested to ;)
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 198 bytes
Desc: This is a digitally signed message part.
More information about the kde-core-devel