Review Request: "konsole --nofork" crashes when started not from terminal

[Thomas Lübking]

> On Dec. 24, 2011, 8:28 a.m., David Faure wrote:
> > This seems like a dangerous change to me.
> > 
> > With it, you could run multiple instance of "kmail --nofork", and they would step on each other's toes. The whole point of kuniqueapplication (in normal apps, not in konsole), is to prevent multiple instances of the application from running.
> > A developer using --nofork in valgrind or gdb shouldn't run the risk that an app that wasn't mean to run twice, ends up running twice.
> > 
> > konsole is doing rather unusual stuff with kuniqueapplication so I can't comment on what would be the proper fix for konsole, but I'm pretty sure it should only affect konsole, not other kuniqueapplications.
> 
> Thomas Lübking wrote:
>     Seconded - "Do NOT ship it!"
>     
>     $ <random kuniqueapplication> --help-kde
>     ...
>       --nofork                  Do not run in the background.
>     
>     
>     There's no indication this means to spawn a new process and it's (despite the ambigious term) obviously not meant like this (but meaning "do not fork to background")
>     
>     -> Solution:
>     Konsole should add an extra parameter or map it's --nofork to NonUniqueInstance in the ::start() call.
>     
>     Turning "--nofork" to "--new-process" for general kuniqueapplication is rather a no-go, because applications like eg. k3b or amarok could possibly really rely on it (ioctl/database messup)
>     
>     I do see your point in either having an extra konsole process as well as in redirecting IO, but please fix it in konsole *only*
> 
> David Faure wrote:
>     Don't get confused by the variable name. AFAICS "forceNewProcess" only means that it will register to DBus as kfoo-PID instead of kfoo.
>     Nofork doesn't fork, the patch doesn't change that, AFAICS.
>
> 
> Thomas Lübking wrote:
>     Looking at the patch, setting that flag would get him into the block he desires so whatever the effect of the patch is, setting the flag should gain the same, yesno?!
> 
> Askar Safin wrote:
>     Yes, you are right. So, please add to konsole/src/main.cpp to function forceNewProcess() check, wherever or not there is option "--nofork".
> 
> Askar Safin wrote:
>     Thomas, if you apply the patch, konsole will start in new process if there is --nofork option independently of terminal status. But, as I said, my patch is bad idea, edit konsole/src/main.cpp

I didn't mean to question that result, just that that's not the purpose of --nofork in general. Konsole somehow abuses it ;-)

PS: i'm not the maintainer of konsole, so i cannot just add that check there across whoever /is/ the maintainer, sorry.


- Thomas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103333/#review9226
-----------------------------------------------------------


On Dec. 23, 2011, 11:58 a.m., Askar Safin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103333/
> -----------------------------------------------------------
> 
> (Updated Dec. 23, 2011, 11:58 a.m.)
> 
> 
> Review request for kdelibs.
> 
> 
> Description
> -------
> 
> See https://bugs.kde.org/show_bug.cgi?id=288200
> 
> 
> This addresses bug 288200.
>     http://bugs.kde.org/show_bug.cgi?id=288200
> 
> 
> Diffs
> -----
> 
>   kdeui/kernel/kuniqueapplication.cpp 777fc35 
> 
> Diff: http://git.reviewboard.kde.org/r/103333/diff/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Askar Safin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20111224/23daa869/attachment.htm>