[prev in list] [next in list] [prev in thread] [next in thread]
List: konsole-devel
Subject: Re: [Konsole-devel] Review Request: "konsole --nofork" crashes when
From: "Askar Safin" <safinaskar () mail ! ru>
Date: 2011-12-25 11:38:16
Message-ID: 20111225113816.11287.98249 () vidsolbach ! de
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
> 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
> Thomas Lübking wrote:
> 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.
> Askar Safin wrote:
> What can I do to apply change to konsole?
> I cannot post it to reviewboard, because I cannot write patch (trivial solution 'return (isatty(1) && \
> !args->isSet("new-tab")) || args->isSet("no-fork")' suddenly doesn't work, because option "no-fork" is \
> in another container). I cannot post it to bugs.kde.org, because there bugs stay opened very long time.
> How to contact with konsole maintainers?
>
> David Faure wrote:
> Use
> !args->isSet("fork");
>
> The option is named fork and default to true, --no-fork is the way to set it to false.
>
> According to git log the last one from `konsole --author` who committed to the code was Kurt Hindenburg \
> <kurt.hindenburg@gmail.com>. Just a month ago, so he should still be around :-)
Thanks, David. I send e-mail to Kurt. !args->isSet("fork") doesn't work, too. I tried it. Option "--fork" \
or "--no-fork" is (as I said) in another container. Not in "KCmdLineArgs::parsedArgs()", but in \
"kuniqueapplication::parsedArgs()" or something like that. But if I write the following:
return (isatty(1) || !kuniqueapplication::parsedArgs()->isSet("fork")) && \
!KCmdLineArgs::parsedArgs()->isSet("new-tab");
program crashes with fatal error or segmentation fault
- Askar
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103333/#review9226
-----------------------------------------------------------
On Dec. 25, 2011, 9:01 a.m., Askar Safin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103333/
> -----------------------------------------------------------
>
> (Updated Dec. 25, 2011, 9:01 a.m.)
>
>
> Review request for kdelibs and Konsole.
>
>
> 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
>
>
[Attachment #5 (text/html)]
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/103333/">http://git.reviewboard.kde.org/r/103333/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 24th, 2011, 8:28 a.m., <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;">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.</pre> </blockquote>
<p>On December 24th, 2011, 2:45 p.m., <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;">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*</pre> </blockquote>
<p>On December 24th, 2011, 3:54 p.m., <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;">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. </pre>
</blockquote>
<p>On December 24th, 2011, 7:04 p.m., <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;">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?!</pre> \
</blockquote>
<p>On December 24th, 2011, 7:09 p.m., <b>Askar Safin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;">Yes, you are right. So, please add to konsole/src/main.cpp to \
function forceNewProcess() check, wherever or not there is option "--nofork".</pre> \
</blockquote>
<p>On December 24th, 2011, 7:13 p.m., <b>Askar Safin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;">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</pre> </blockquote>
<p>On December 24th, 2011, 7:46 p.m., <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;">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.</pre> </blockquote>
<p>On December 25th, 2011, 9:08 a.m., <b>Askar Safin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;">What can I do to apply change to konsole? I cannot post it to \
reviewboard, because I cannot write patch (trivial solution 'return (isatty(1) && \
!args->isSet("new-tab")) || args->isSet("no-fork")' suddenly doesn't \
work, because option "no-fork" is in another container). I cannot post it to bugs.kde.org, \
because there bugs stay opened very long time. How to contact with konsole maintainers?</pre>
</blockquote>
<p>On December 25th, 2011, 9:24 a.m., <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;">Use
!args->isSet("fork");
The option is named fork and default to true, --no-fork is the way to set it to false.
According to git log the last one from `konsole --author` who committed to the code was Kurt Hindenburg \
<kurt.hindenburg@gmail.com>. Just a month ago, so he should still be around :-)</pre> \
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: \
-o-pre-wrap; word-wrap: break-word;">Thanks, David. I send e-mail to Kurt. \
!args->isSet("fork") doesn't work, too. I tried it. Option "--fork" or \
"--no-fork" is (as I said) in another container. Not in "KCmdLineArgs::parsedArgs()", \
but in "kuniqueapplication::parsedArgs()" or something like that. But if I write the following:
return (isatty(1) || !kuniqueapplication::parsedArgs()->isSet("fork")) && \
!KCmdLineArgs::parsedArgs()->isSet("new-tab");
program crashes with fatal error or segmentation fault</pre>
<br />
<p>- Askar</p>
<br />
<p>On December 25th, 2011, 9:01 a.m., Askar Safin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: \
url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: \
left top; background-repeat: repeat-x; border: 1px black solid;"> <tr>
<td>
<div>Review request for kdelibs and Konsole.</div>
<div>By Askar Safin.</div>
<p style="color: grey;"><i>Updated Dec. 25, 2011, 9:01 a.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid \
#b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">See \
https://bugs.kde.org/show_bug.cgi?id=288200</pre> </td>
</tr>
</table>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=288200">288200</a>
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>kdeui/kernel/kuniqueapplication.cpp <span style="color: grey">(777fc35)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/103333/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
_______________________________________________
konsole-devel mailing list
konsole-devel@kde.org
https://mail.kde.org/mailman/listinfo/konsole-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic