[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-frameworks-devel
Subject: Re: Review Request 130058: Make kwalletd5 service both org.kde.kwalletd5 and org.kde.kwalletd
From: René J.V. Bertin <rjvbertin () gmail ! com>
Date: 2017-06-15 10:43:13
Message-ID: 20170615104313.7530.90858 () mimi ! kde ! org
[Download RAW message or body]
--===============3082876655911388726==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
> On April 1, 2017, 9:59 p.m., David Faure wrote:
> > Makes sense to me, +1.
>
> Andreas Sturmlechner wrote:
> Thanks, anyone else who wants to +1?
>
> I've tried to test migration today but it didn't work. May as well have nothing to \
> do with te patch and be caused by the permanently troubled migration agent \
> though... KMail happily gets its IMAP password from kwallet5 though after manually \
> export/import via XML files.
> Andreas Sturmlechner wrote:
> As suspected, on my test system migration is broken regardless of with these \
> patches or not.
> David Faure wrote:
> Are you planning on looking into that? ;-)
>
> These patches are related to migration, it feels a bit wrong to commit changes \
> around migration and still leave it broken.
> Andreas Sturmlechner wrote:
> I don't feel like I'm up to that task. Also, the reason for why it works for some, \
> but not all the systems, has afaik never been established.
> Andreas Sturmlechner wrote:
> Could we push it to give it some testing by others until the upcoming release?
>
> René J.V. Bertin wrote:
> Was this tested, in the end?
>
> I only became aware of the change after I installed 5.35.0 on my FrankenStux box \
> with a Plasma4 desktop and KF5 installed into /opt/local, and I had to re-grant \
> access to my wallets to all applications accessing them.
> I cannot yet vouch that my wallets were migrated completely. I think they were, but \
> how do I verify that now? Even the KDE4 kwalletmanager goes through kwalletd5. It \
> too had to be granted access.
> Confusingly, it still shows the old access list, which clearly it shouldn't if the \
> information is stale.
> Andreas Sturmlechner wrote:
> If you see your old wallets now in kwalletd5, then migration has worked. I've never \
> heard about issues with partial migration; if it does not work, it fails to migrate \
> completely.
That's the impression I had, but I never had to check. Migration was done about once \
after every time I logged in after a reboot. I never had to check though, kwalletd5 \
only served KF5 applications of which very few ever need wallet access in my case.
We'll see how this works out when I log in and /opt/local isn't available (which in \
theory is supposed to be possible).
To be seen also how this affects KDE/Mac users who have KDELibs4 configured to use my \
Keychain "backend". There's no kwalletd4 there, no migration either, and first \
impressions suggest the 2 systems continue to function in perfect isolation.
- René J.V.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/130058/#review102982
-----------------------------------------------------------
On May 27, 2017, 4:32 p.m., Andreas Sturmlechner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/130058/
> -----------------------------------------------------------
>
> (Updated May 27, 2017, 4:32 p.m.)
>
>
> Review request for KDE Frameworks and Stefan Brüns.
>
>
> Repository: kwallet
>
>
> Description
> -------
>
> These are not my own patches, I'm creating this review request after having been \
> made aware of *kwalletd4_dbus_compat* branch in kwallet.git, which I simply rebased \
> on top of current master (author of course preserved) to be able to test it. I \
> think it would be a great improvement over the current situation that is rather \
> confusing to the users.
> The changes are organised in 5 commits:
>
> - Check for unique applicaton instance as early as possible
> Exit before KWalletD and the MigrationAgent has been initialized.
> The return value is changed, but concurrent instatiation of kwalletd is
> not a fault.
>
> - Only start timer for migration agent if necessary
> - Whitespace fixup
> - Signal completion of migration agent
> - Replace kwalletd4 after migration has finished
> kwalletd5 can service both org.kde.kwalletd5 and org.kde.kwalletd
>
>
> Diffs
> -----
>
> src/runtime/kwalletd/kwalletd.h 3571535cd8bd78415002795f9b61adf9f6cfb8c1
> src/runtime/kwalletd/kwalletd.cpp 18ef9fa7e6ddaeba6e0b32deae3de1dae39df5bb
> src/runtime/kwalletd/main.cpp ff9620886fa1959e14b594be6bbb4644b637c000
> src/runtime/kwalletd/migrationagent.h 0f6467c1753ef34b7f7f7e282503ec5607927db9
> src/runtime/kwalletd/migrationagent.cpp f3da94743ecd83fe406e058f560d4238caec1be8
>
> Diff: https://git.reviewboard.kde.org/r/130058/diff/
>
>
> Testing
> -------
>
> Migration itself was not tested so far, but a legacy application like ksirk was \
> able to create a new wallet just fine and can access it as well. I do not have \
> kwalletd4 installed anymore.
>
> Thanks,
>
> Andreas Sturmlechner
>
>
--===============3082876655911388726==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 8bit
<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/130058/">https://git.reviewboard.kde.org/r/130058/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On April 1st, 2017, 9:59 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;">Makes sense to me, +1.</p></pre> </blockquote>
<p>On April 8th, 2017, 2:39 p.m. CEST, <b>Andreas Sturmlechner</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;">Thanks, anyone else who wants to +1?</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I've \
tried to test migration today but it didn't work. May as well have nothing to do with \
te patch and be caused by the permanently troubled migration agent though... KMail \
happily gets its IMAP password from kwallet5 though after manually export/import via \
XML files.</p></pre> </blockquote>
<p>On April 9th, 2017, 9:20 p.m. CEST, <b>Andreas Sturmlechner</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;">As \
suspected, on my test system migration is broken regardless of with these patches or \
not.</p></pre> </blockquote>
<p>On April 15th, 2017, 10:16 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;">Are \
you planning on looking into that? ;-)</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">These patches are \
related to migration, it feels a bit wrong to commit changes around migration and \
still leave it broken.</p></pre> </blockquote>
<p>On April 16th, 2017, 7:54 p.m. CEST, <b>Andreas Sturmlechner</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 \
don't feel like I'm up to that task. Also, the reason for why it works for some, but \
not all the systems, has afaik never been established.</p></pre> </blockquote>
<p>On May 27th, 2017, 1:58 p.m. CEST, <b>Andreas Sturmlechner</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;">Could \
we push it to give it some testing by others until the upcoming release?</p></pre> \
</blockquote>
<p>On June 15th, 2017, 10:46 a.m. CEST, <b>René J.V. Bertin</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;">Was \
this tested, in the end?</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">I only became aware of the change after \
I installed 5.35.0 on my FrankenStux box with a Plasma4 desktop and KF5 installed \
into /opt/local, and I had to re-grant access to my wallets to all applications \
accessing them. </p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">I cannot yet vouch that my wallets were \
migrated completely. I think they were, but how do I verify that now? Even the KDE4 \
kwalletmanager goes through kwalletd5. It too had to be granted access.</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Confusingly, it still shows the old access list, which clearly it shouldn't \
if the information is stale.</p></pre> </blockquote>
<p>On June 15th, 2017, 11:10 a.m. CEST, <b>Andreas Sturmlechner</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;">If \
you see your old wallets now in kwalletd5, then migration has worked. I've never \
heard about issues with partial migration; if it does not work, it fails to migrate \
completely.</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;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">That's the impression I had, but I never had to check. Migration was done \
about once after every time I logged in after a reboot. I never had to check though, \
kwalletd5 only served KF5 applications of which very few ever need wallet access in \
my case.</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">We'll see how this works out when I log in and \
/opt/local isn't available (which in theory is supposed to be possible).</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">To be seen also how this affects KDE/Mac users who have KDELibs4 configured \
to use my Keychain "backend". There's no kwalletd4 there, no migration either, and \
first impressions suggest the 2 systems continue to function in perfect \
isolation.</p></pre> <br />
<p>- René J.V.</p>
<br />
<p>On May 27th, 2017, 4:32 p.m. CEST, Andreas Sturmlechner 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 Stefan Brüns.</div>
<div>By Andreas Sturmlechner.</div>
<p style="color: grey;"><i>Updated May 27, 2017, 4:32 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kwallet
</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;">These are not my own patches, I'm creating this review \
request after having been made aware of <em style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: \
normal;">kwalletd4_dbus_compat</em> branch in kwallet.git, which I simply rebased on \
top of current master (author of course preserved) to be able to test it. I think it \
would be a great improvement over the current situation that is rather confusing to \
the users.</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">The changes are organised in 5 commits:</p> <ul \
style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: \
inherit;white-space: normal;"> <li style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: normal;"> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">Check for unique \
applicaton instance as early as possible Exit before KWalletD and the MigrationAgent \
has been initialized. The return value is changed, but concurrent instatiation of \
kwalletd is not a fault.</p>
</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: normal;"> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">Only start timer for migration agent if \
necessary</p> </li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: normal;">Whitespace fixup</li> <li style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Signal \
completion of migration agent</li> <li style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: normal;">Replace kwalletd4 after \
migration has finished kwalletd5 can service both org.kde.kwalletd5 and \
org.kde.kwalletd</li> </ul></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;">Migration itself was not tested so far, but a legacy \
application like ksirk was able to create a new wallet just fine and can access it as \
well. I do not have kwalletd4 installed anymore.</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/runtime/kwalletd/kwalletd.h <span style="color: \
grey">(3571535cd8bd78415002795f9b61adf9f6cfb8c1)</span></li>
<li>src/runtime/kwalletd/kwalletd.cpp <span style="color: \
grey">(18ef9fa7e6ddaeba6e0b32deae3de1dae39df5bb)</span></li>
<li>src/runtime/kwalletd/main.cpp <span style="color: \
grey">(ff9620886fa1959e14b594be6bbb4644b637c000)</span></li>
<li>src/runtime/kwalletd/migrationagent.h <span style="color: \
grey">(0f6467c1753ef34b7f7f7e282503ec5607927db9)</span></li>
<li>src/runtime/kwalletd/migrationagent.cpp <span style="color: \
grey">(f3da94743ecd83fe406e058f560d4238caec1be8)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/130058/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============3082876655911388726==--
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic