<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/125483/">https://git.reviewboard.kde.org/r/125483/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 2nd, 2015, 10:35 p.m. CEST, <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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Sending Activate to a running Unique process is not a failure, so NoExitOnFailure should not affect it.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If you need a "no exit, ever" flag, then I would recommend adding a separate flag for that. I'm not sure I understand the use case though.</p></pre>
</blockquote>
<p>On October 3rd, 2015, 3:02 a.m. CEST, <b>Martin Klapetek</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In terms of the this code, the "failure" is "!d->registered" (see line 147 or 153 with this patch), so it is a failure to register the service (this is also supported by the docs saying "Indicates that the application should not exit if it failed to register with D-Bus."). So even if it sends Activate, it still failed to register the service and therefore the NoExitOnFailure should apply. No?</p></pre>
</blockquote>
<p>On October 3rd, 2015, 10:01 a.m. CEST, <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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">While that might be one way to literally interpret the docs, it's definitely not what I had in mind when I wrote them.
See "Already running so it's ok!", the fact that there is no "errorMessage" set in this case, and the fact that the current use of NoExitOnFailure happens right before qCritical() + exit.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Process already running is not an error or a failure, it's a perfectly normal situation.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What I can't really remember is the use case for NoExitOnFailure; I think it was "being able to start the app even if there's no DBus running at all", at the expense of losing the Unique behavior of course.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Back to your use case, I wonder why "pass data" can't be done with the CommandLine method. Or are we talking about different data than the command line? Which other data could there be, in an application we just started ?</p></pre>
</blockquote>
<p>On October 7th, 2015, 7:09 p.m. CEST, <b>Martin Klapetek</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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Process already running is not an error or a failure, it's a perfectly normal situation.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It isn't, the "failure" in this context of KDBusService as I understand it from the docs and the code is just the failure to register the unique dbus service, which I thought the NoExitOnFailure is meant for.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I mean it would be nice if the user had a choice to not forcefully exit when it cannot get the unique dbus service.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Anyways, the main reason for this patch comes from kwalletd, which has this code:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: #008000; font-weight: bold">KDBusService</span> <span style="color: #008000; font-weight: bold">dbusUniqueInstance</span><span style="color: #666666">(</span><span style="color: #008000; font-weight: bold">KDBusService</span><span style="color: #666666">:</span><span style="color: #AA22FF">:Unique</span> <span style="color: #666666">|</span> <span style="color: #008000; font-weight: bold">KDBusService</span><span style="color: #666666">:</span><span style="color: #AA22FF">:NoExitOnFailure</span><span style="color: #666666">);</span>
<span style="color: #666666">//</span> <span style="color: #008000; font-weight: bold">NOTE</span><span style="color: #666666">:</span> <span style="color: #008000; font-weight: bold">the</span> <span style="color: #008000; font-weight: bold">command</span> <span style="color: #008000; font-weight: bold">should</span> <span style="color: #008000; font-weight: bold">be</span> <span style="color: #008000; font-weight: bold">parsed</span> <span style="color: #008000; font-weight: bold">only</span> <span style="color: #008000; font-weight: bold">after</span> <span style="color: #008000; font-weight: bold">KDBusService</span> <span style="color: #008000; font-weight: bold">instantiation</span>
<span style="color: #008000; font-weight: bold">QCommandLineParser</span> <span style="color: #008000; font-weight: bold">cmdParser</span><span style="color: #666666">;</span>
<span style="color: #008000; font-weight: bold">aboutdata</span><span style="color: #0000FF; font-weight: bold">.setupCommandLine</span><span style="color: #666666">(&</span><span style="color: #008000; font-weight: bold">cmdParser</span><span style="color: #666666">);</span>
<span style="color: #008000; font-weight: bold">cmdParser</span><span style="color: #0000FF; font-weight: bold">.process</span><span style="color: #666666">(</span><span style="color: #008000; font-weight: bold">app</span><span style="color: #666666">);</span>
<span style="color: #008000; font-weight: bold">if</span> <span style="color: #666666">(!</span><span style="color: #008000; font-weight: bold">dbusUniqueInstance</span><span style="color: #0000FF; font-weight: bold">.isRegistered</span><span style="color: #666666">())</span> {
qDebug() <span style="color: #666666"><<</span> <span style="color: #BA2121">"kwalletd is already running!"</span>;
return <span style="color: #666666">1</span>;
}
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Now the code exits at the first line and never reaches the if below, which I originally wanted to extend to instead check for a locked wallet and unlock it with PAM (in case this kwalletd was executed from PAM) and only then actually exit.</p></pre>
</blockquote>
<p>On October 7th, 2015, 7:14 p.m. CEST, <b>Martin Klapetek</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The other usecase for this patch can be seen in the code excerpt above - returning custom values in main(). Eg. kwalletd would return 1 in case it couldn't register on dbus, but KDBusService exits with 0 instead.</p></pre>
</blockquote>
<p>On October 7th, 2015, 8:10 p.m. CEST, <b>Martin Klapetek</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">(I just reread my reply above and the first "It isn't," was meant as a reply to the "...is not an error or a failure", not to "it's a perfectly normal situation". #context)</p></pre>
</blockquote>
<p>On October 10th, 2015, 3:47 p.m. CEST, <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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I think I know better than you what I meant when I wrote the code and the docu :-) I agree that the docu might be unclear, but don't tell me I meant something else :-)
By failure I meant an error (like no dbus session found).
"Unique app already running" is <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">not</em> a failure. Look at the kdelib4s times: kmail & ; kmail & -> return code 0.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't see a strong reason for kwalletd to return 1 instead of 0 if it's already running. The user wanted kwalletd to run, it happens to run already, there's no error there.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For your use case of unlocking with PAM, it seems to me that there's an alternative (possibly better) solution than a NoExit flag anyway. Before all the code above,
1) use KWallet API to check if the wallet is opened (if kwalletd isn't running, it's not; but maybe kwalletd is running and the wallet is closed).
(so using KDBusService to check if it's running only does half the job anyway, so better do what you need to do right away)
2) if the wallet is closed, open it with PAM
3) and <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">then</em> create the KDBusService.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Does that make sense? [I don't know much about PAM; and in all cases I'm not sure how an already running kwalletd suddenly sees the wallet being open]</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It just seems to me that you're trying to abuse "server-side" API (KDBusService) for a client-side check (is the daemon already running?).</p></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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It just seems to me that you're trying to abuse "server-side" API (KDBusService) for a client-side check (is the daemon already running?).</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Hmm, perhaps. Regardless of kwallet, I still think it'd make sense to give more control to the user of KDBusService rather than force-exit. But that's fine, I'll see what alternatives we can do for kwallet.</p></pre>
<br />
<p>- Martin</p>
<br />
<p>On October 2nd, 2015, 7:52 p.m. CEST, Martin Klapetek wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks and David Faure.</div>
<div>By Martin Klapetek.</div>
<p style="color: grey;"><i>Updated Oct. 2, 2015, 7:52 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdbusaddons
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If KDBusService fails to register Unique service because other instance is already running and if the user specified NoExitOnFailure, don't exit after calling "Activate" on the other instance.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">App no longer exits and can do some other tasks like calling a different dbus method to eg. pass data and then exit on its own.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/kdbusservice.cpp <span style="color: grey">(ea7727d)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/125483/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>