D28745: Skip caching thumbnails on encrypted filesystems

[Thiago Macieira]

thiago added a comment.


  BTW, have you checked if the thumbnails are still generated for non-removable 
but encrypted filesystems? My whole system is encrypted (except for /boot), so 
it would be a performance loss if no thumbnails were ever cached.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28745

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure
Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Thiago Macieira]

thiago added inline comments.

INLINE COMMENTS

> marcingu wrote in thumbnail.cpp:776
> "-1 isn't exactly a great value for an unsigned int :-)"
> 
> I believe "-1" guaranties unsigned to be set to its maximum value, due to how 
> type conversion works, but I could be wrong.
> 
> Do you know if size of dev_st is fixed or depends on architecture? I'd like 
> to define const rather than use literals for checks, but I don't know if I 
> can simply define it as 0x or if it's more complicated.

unsigned(-1)

is guaranteed to be added to the modulo value until it becomes positive. That 
is, it is

  (UINT_MAX + 1) - 1

Note that this will generate a warning with some compilers about change of 
sign, so either make the cast explicit or simply use ~0U.

As for what type it is... that's why we have std::numeric_limits.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28745

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns, dfaure
Cc: dfaure, thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Thiago Macieira]

thiago added a comment.


  >   for (Device device: list) {
  >   StorageAccess *storageAccess = device.as();
  >   if (canonPath.startsWith(storageAccess->filePath()) && 
storageAccess->filePath().size() > match_length) {
  >   match_length = storageAccess->filePath().size();
  >   match = device;
  >   }
  >
  
  This search is affected by the same sibling match bug that QStorageInfo was. 
See https://bugreports.qt.io/browse/QTBUG-49498.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28745

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns
Cc: thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Thiago Macieira]

thiago added a comment.


  Use Solid and ask it if the device is on an encrypted device. If Solid does 
not have such an API, add it.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28745

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns
Cc: thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, 
waitquietly, azyx, nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, 
cblack, fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, 
navarromorales, firef, andrebarros, emmanuelp, rdieter, mikesomov

D28745: Skip caching thumbnails on encrypted filesystems

[Thiago Macieira]

thiago added inline comments.

INLINE COMMENTS

> thumbnail.cpp:743
> +const auto mount =  mountsList.findByPath(filePath);
> +allowCache = !(mount->mountType() == 
> QLatin1String("fuse.cryfs") || mount->mountType() == 
> QLatin1String("fuse.encfs"));
> +}

How about encrypted block storage, which is superior to those encrypted fs and 
the recommended solution?

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D28745

To: marcingu, ivan, broulik, #dolphin, ngraham, meven, bruns
Cc: thiago, bruns, meven, ngraham, kde-frameworks-devel, kfm-devel, azyx, 
nikolaik, pberestov, iasensio, aprcela, fprice, LeGast00n, cblack, 
fbampaloukas, alexde, Codezela, feverfew, michaelh, spoorun, navarromorales, 
firef, andrebarros, emmanuelp, rdieter, mikesomov

D27804: smb: add hack to support spaces in workgroup names

[Thiago Macieira]

thiago added a comment.


  In D27804#621988 , @sitter wrote:
  
  > In D27804#621970 , @thiago wrote:
  >
  > > Still want to see that round-trip.
  >
  >
  > But why? Converting an smbcUrl to a QUrl would literally be useless code.
  
  
  Are you saying you never convert the URL coming from the library so it can be 
displayed in KIO/Dolphin? Are you sure you always decompose its parts and 
recompose a QUrl from it?

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D27804

To: sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, thiago, pberestov, iasensio, fprice, 
LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D27804: smb: add hack to support spaces in workgroup names

[Thiago Macieira]

thiago added a comment.


  Still want to see that round-trip.

INLINE COMMENTS

> smburltest.cpp:112
> +// % character - run through .url() to simulate behavior of our 
> listDir()
> +
> QCOMPARE(SMBUrl(QUrl(QUrl("smb://?kio-workgroup=HAX%MAX").url())).toSmbcUrl(),
> + "smb://HAX%25MAX/");

Please don't use QUrl's URL-correction in the constructor. This is not a valid 
URL. Use %25 here.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D27804

To: sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, thiago, pberestov, iasensio, fprice, 
LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D27802: smb: fix ipv6 support

[Thiago Macieira]

thiago added a comment.


  looks good to me.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D27802

To: sitter, ngraham
Cc: thiago, kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, 
LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D27802: smb: fix ipv6 support

[Thiago Macieira]

thiago added a comment.


  STD3 also forbids domain labels ending in dash, so can you add a test for 
`smb://[fe80::]/` too? You'll need to adjust your code

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D27802

To: sitter, ngraham
Cc: thiago, kde-frameworks-devel, kfm-devel, pberestov, iasensio, fprice, 
LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D27804: smb: add hack to support spaces in workgroup names

[Thiago Macieira]

thiago added a comment.


  Looks good, but testing should be extended. I suggest:
  
  1. A workgroup with non-US-ASCII characters in the name, if that's permitted
  2. A workgroup with % in the name: the URL should have "%25"
  3. Roundtrip checking. The test only does toSmbcUrl(), so please test the 
reverse.
  4. Error-checking the conversion from query to hostname: things like %00 (a 
NUL byte), URL delimiters (':', '/', '?' and '#'), byte sequences that do not 
encode UTF-8 (like %FF).
  
  I think that you'll find that % works in one direction only. I have no idea 
what you'll find for #4.

INLINE COMMENTS

> smburl.cpp:134
> +QUrlQuery query(sambaUrl);
> +const QString workgroup = query.queryItemValue("kio-workgroup");
> +if (workgroup.isEmpty()) {

For the % issue, you may want to pass `QUrl::FullyDecoded` as the second 
argument to queryItemValue.

REPOSITORY
  R320 KIO Extras

REVISION DETAIL
  https://phabricator.kde.org/D27804

To: sitter, ngraham
Cc: kde-frameworks-devel, kfm-devel, thiago, pberestov, iasensio, fprice, 
LeGast00n, cblack, MrPepe, fbampaloukas, alexde, GB_2, Codezela, feverfew, 
meven, michaelh, spoorun, navarromorales, firef, ngraham, andrebarros, bruns, 
emmanuelp, mikesomov

D25996: KRemoteEncoding::encoding: don't return temporary. Found with clazy.

[Thiago Macieira]

thiago added a comment.


  Good catch.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D25996

To: chehrlic, thiago, dfaure
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D17737: [CopyJob] Create clones in btrf/xfs mount

[Thiago Macieira]

thiago added a comment.


  QFile::copy already implements FICLONE, sendfile() and Darwin's clonefile().

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17737

To: chinmoyr, dfaure, #frameworks, thiago, ossi
Cc: ngraham, bruns, kde-frameworks-devel, LeGast00n, GB_2, michaelh

D21855: [IdUtils] Fix aliasing warning

[Thiago Macieira]

thiago added a comment.


  Alternative: return (quint64(devId) << 32) | inode;
  
  Not endian-dependent, like the memcpy solution. Make sure the other side is 
adapted to match.

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D21855

To: bruns, #baloo, ngraham, astippich, poboiko, thiago
Cc: kde-frameworks-devel, LeGast00n, fbampaloukas, domson, ashaposhnikov, 
michaelh, astippich, spoorun, ngraham, bruns, abrahams

D21855: [IdUtils] Fix aliasing warning

[Thiago Macieira]

thiago added a comment.


  +1

REPOSITORY
  R293 Baloo

REVISION DETAIL
  https://phabricator.kde.org/D21855

To: bruns, #baloo, ngraham, astippich, poboiko, thiago
Cc: kde-frameworks-devel, LeGast00n, fbampaloukas, domson, ashaposhnikov, 
michaelh, astippich, spoorun, ngraham, bruns, abrahams

D21317: Manipulate bytes instead of characters

[Thiago Macieira]

thiago added a comment.


  As for truncating UTF-8 multibyte sequences in the middle, when you convert 
back using QFile::decodeName, it'll be nonsensical. But I don't think it really 
matters since you're truncating and using the display string anyway, so you're 
already losing data. This wasn't a recoverable conversion anyway.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D21317

To: chinmoyr, bruns, dfaure, thiago
Cc: ngraham, kde-frameworks-devel, michaelh, bruns

D21317: Manipulate bytes instead of characters

[Thiago Macieira]

thiago added a comment.


  I cringe a little when you apply QFile to a URL, but this is probably safe 
enough.
  
  LGTM

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D21317

To: chinmoyr, bruns, dfaure, thiago
Cc: ngraham, kde-frameworks-devel, michaelh, bruns

D19107: Write valid UTF8 characters without escaping.

[Thiago Macieira]

thiago added a comment.


  It looks good to me.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D19107

To: vandenoever, dfaure, arichardson, apol, #frameworks, thiago
Cc: rapiteanu, kde-frameworks-devel, michaelh, ngraham, bruns

D19107: Write valid UTF8 characters without escaping.

[Thiago Macieira]

thiago added a comment.


  Much better, thanks.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D19107

To: vandenoever, dfaure, arichardson, apol, #frameworks, thiago
Cc: rapiteanu, kde-frameworks-devel, michaelh, ngraham, bruns

D19107: Write valid UTF8 characters without escaping.

[Thiago Macieira]

thiago added inline comments.

INLINE COMMENTS

> vandenoever wrote in kconfigini.cpp:683
> A check for U+10 > value is needed.
> Overlong sequences are caught on line 696 (count < 4).

That's not what an overlong sequence is. You can produce 2-, 3- and 4-byte 
overlong sequences. See the examples in the Qt test, like 0xc0 0x80.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D19107

To: vandenoever, dfaure, arichardson, apol, #frameworks, thiago
Cc: rapiteanu, kde-frameworks-devel, michaelh, ngraham, bruns

D19107: Write valid UTF8 characters without escaping.

[Thiago Macieira]

thiago added inline comments.

INLINE COMMENTS

> kconfigini.cpp:683
> +// When an additional byte leads to an invalid character, return 
> false.
> +bool addByte(unsigned char b) {
> +if (count == 0) {

This function does operate properly to find valid syntax UTF-8 sequences, but 
it is neither catching overlong sequences nor UTF-8 content above U+10 
(UTF-8 can encode 0x11000 in 4 bytes).

See 
https://code.woboq.org/qt5/qtbase/tests/auto/corelib/codecs/utf8/utf8data.cpp.html#_Z19loadInvalidUtf8Rowsv
 for potential UTF-8 pitfalls.

REPOSITORY
  R237 KConfig

REVISION DETAIL
  https://phabricator.kde.org/D19107

To: vandenoever, dfaure, arichardson, apol, #frameworks, thiago
Cc: rapiteanu, kde-frameworks-devel, michaelh, ngraham, bruns

D17015: Fix the Qt doc creation with Qt 5.12.

[Thiago Macieira]

thiago added a comment.


  qhelpgenerator is coming back in 5.12.1. You may simply tell people to skip 
the .0 release and upgrade.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D17015

To: cgiboudeaux, kossebau
Cc: thiago, tcberner, kde-frameworks-devel, kde-buildsystem, michaelh, ngraham, 
bruns

D14302: Don't block forever in ensureKdeinitRunning

[Thiago Macieira]

thiago added a comment.


  In D14302#297515 , @dfaure wrote:
  
  > The idea of the old code was: if I can't get the lock, then someone else is 
already in the process of starting kdeinit, so I'll just wait for that to 
happen, by locking again, i.e. blocking on purpose, and then checking that the 
DBus name is up, i.e. the other process did manage to do it successfully.
  
  
  It's Kind of silly to tryLock() then lock(). Why not just lock()? Was it the 
optimisation to avoid the D-Bus call? If so, a comment would be warranted.
  
  > I said from the start that it wasn't tryLock() that was blocking but 
lock(), good to see that this is now confirmed, however we're back to square 
one: why is lock never returning? Surely the other process which is executing 
this method is releasing the lock after the QProcess::execute, right?
  
  Yeah, I trusted the patch too. The frame for lock() is missing because it's a 
tail-call jump.
  
  Anyway, one theory for why the lock file is still present: it's stale from a 
previous boot, but the PID is taken by some process in the current boot. This 
is fixed in Qt 5.11 by saving the boot ID (commit 
772863355a0cf57a49e93608790dfd17c8fd82da). So question to @jtamate , what Qt 
version is this? Can you paste here the contents of the lock file itself, as 
well as what process (if any) the PID in that file points to.

REPOSITORY
  R271 KDBusAddons

REVISION DETAIL
  https://phabricator.kde.org/D14302

To: jtamate, dfaure, #frameworks, thiago
Cc: lvsouza, kde-frameworks-devel, michaelh, ngraham, bruns

D14302: Don't block forever in ensureKdeinitRunning

[Thiago Macieira]

thiago added inline comments.

INLINE COMMENTS

> kdeinitinterface.cpp:46
>  QLockFile lock(QDir::tempPath() + QLatin1Char('/') + 
> QLatin1String("startkdeinitlock"));
> -if (!lock.tryLock()) {
> +if (!lock.tryLock(timeout)) {
>  lock.lock();

This line doesn't need changing. You're getting a tryLock() failure because the 
lock file already exists. Adding a 5 second timeout is not going to change that.

> kdeinitinterface.cpp:47
> +if (!lock.tryLock(timeout)) {
>  lock.lock();
>  if 
> (dbusDaemon->isServiceRegistered(QStringLiteral("org.kde.klauncher5"))) {

The problem is here. So we failed to lock, then we try again to lock, forever. 
Why is this code doing that?

REPOSITORY
  R271 KDBusAddons

REVISION DETAIL
  https://phabricator.kde.org/D14302

To: jtamate, dfaure, #frameworks, thiago
Cc: lvsouza, kde-frameworks-devel, michaelh, ngraham, bruns

D14302: Don't block forever in ensureKdeinitRunning

[Thiago Macieira]

thiago added a comment.


  In D14302#297159 , @jtamate wrote:
  
  >   (gdb) disass
  >   Dump of assembler code for function _ZN9QLockFile7tryLockEi:
  >  0x7f54be8bc752 <+2>: mov$0x,%eax
  >   ...
  >  0x7f54be8bc76d <+29>:test   %esi,%esi
  >   ...
  >  0x7f54be8bc772 <+34>:cmovs  %eax,%esi
  >   ...
  >  0x7f54be8bc78c <+60>:movslq %esi,%rsi
  >  0x7f54be8bc78f <+63>:callq  0x7f54be962150 

  >
  
  
  This is entirely correct: in +29 it checks  parameter (in %esi) and in +34 if 
it has the sign bit set (read: is negative), moves -1 to it. Then it does a 
sign extension from 32- to 64-bit in +60, before placing the call.
  
  qMax is working properly.
  
  >   Dump of assembler code for function 
_ZN16KDEInitInterface20ensureKdeinitRunningEv:
  >   ...
  >  0x7f54c0a23ae1 <+497>:   xor%esi,%esi
  >  0x7f54c0a23ae3 <+499>:   mov%r12,%rdi
  >  0x7f54c0a23ae6 <+502>:   callq  0x7f54c0a1e960 
<_ZN9QLockFile7tryLockEi@plt>
  
  Also correct: this passed a zero as the parameter to tryLock().
  
  Now here's the interesting thing:
  
  >  0x7f54c0a23aeb <+507>:   test   %al,%al
  >  0x7f54c0a23aed <+509>:   jne0x7f54c0a23b8d 
<_ZN16KDEInitInterface20ensureKdeinitRunningEv+669>
  >  0x7f54c0a23af3 <+515>:   mov%r12,%rdi
  >  0x7f54c0a23af6 <+518>:   callq  0x7f54c0a1e9c0 
<_ZN9QLockFile4lockEv@plt>
  >   => 0x7f54c0a23afb <+523>:   mov%rbx,%rdi
  
  Note where the => is pointing to. It's not to the return from tryLock(), but 
to the return to lock(). We've been looking at the wrong line.
  
  That also means the patch doesn't make sense, because it's changing the line 
that is already working.

REPOSITORY
  R271 KDBusAddons

REVISION DETAIL
  https://phabricator.kde.org/D14302

To: jtamate, dfaure, #frameworks, thiago
Cc: lvsouza, kde-frameworks-devel, michaelh, ngraham, bruns

D14302: Don't block forever in ensureKdeinitRunning

[Thiago Macieira]

thiago added a comment.


  Quickly checking on openSUSE Tumbleweed
  
  In D14302#297185 , @lvsouza wrote:
  
  > I think I know what is happening. This line
  >
  > QDeadlineTimer timer(qMax(timeout, -1));// QDT only takes -1 as 
"forever"
  >
  > passes the result of qMax() to QDeadlineTimer's constructor. That 
constructor receives a quint64. Since qMax() is a template:
  
  
  The constructor takes a qint64, not a quint64.
  
  > inline const T &qMax(const T &a, const T &b) { return (a < b) ? b : a; }
  > 
  > it will use the type of the assigned variable (quint64 in this case) as T 
and casting -1 to INT64_MAX. Changing the line to:
  
  Again, incorrect. It will use the type T, which must be the same for both 
arguments. The timeout parameter is int and the -1 literal is int. So the 
comparison is performed in int, which should return 0.

REPOSITORY
  R271 KDBusAddons

REVISION DETAIL
  https://phabricator.kde.org/D14302

To: jtamate, dfaure, #frameworks, thiago
Cc: lvsouza, kde-frameworks-devel, michaelh, ngraham, bruns

D14302: Don't block forever in ensureKdeinitRunning

[Thiago Macieira]

thiago added a comment.


  > timer = {t1 = 9223372036854775807, t2 = 0, type = 1}
  
  This is indeed Forever. How did it get there?
  
  I showed in my debug session that the QDeadlineTimer is passed zero, and then 
it does initialise properly. So I'm now beginning to question the compiler. 
Specifically, this line:
  
QDeadlineTimer timer(qMax(timeout, -1));// QDT only takes -1 as 
"forever"
  
  Is it possible that the compiler miscompiled qMax and caused a value of -1 to 
be passed?
  
  Alternatively, could timeout be -1? The call site is (before your patch):
  
if (!lock.tryLock()) {
  
  Which should get the default value of 0. Could it be getting -1 somehow? 
Maybe your distribution patched Qt?
  
  Can you provide the disassembly of those two functions? Just run "disass" in 
gdb from those two frames and paste here.
  
  > In D14302#297119 , @thiago wrote:
  > 
  >> No, because your statement is incorrect. setPreciseRemainingTime **does** 
assign to t1:
  >>
  >>   t1 += secs + toSecsAndNSecs(nsecs).first;
  >>   
  > 
  > 
  > Yes, but this is assuming t1 = 0, I mean, it is not t1 = secs (not with 
+=).
  
  Wrong line. It does assign here:
  
*this = current(timerType);
  
  And even if it didn't, the value in the timer  is very specific (t1 == 
INT64_MAX and t2 == 0). The chance of getting that from uninitialised data is 
too small to be considered.

REPOSITORY
  R271 KDBusAddons

REVISION DETAIL
  https://phabricator.kde.org/D14302

To: jtamate, dfaure, #frameworks, thiago
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14302: Don't block forever in ensureKdeinitRunning

[Thiago Macieira]

thiago added a comment.


  Can you print the contents of the timer object inside tryLock()?

REPOSITORY
  R271 KDBusAddons

REVISION DETAIL
  https://phabricator.kde.org/D14302

To: jtamate, dfaure, #frameworks, thiago
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14302: Don't block forever in ensureKdeinitRunning

[Thiago Macieira]

thiago added a comment.


  In D14302#296671 , @jtamate wrote:
  
  > Another hypothesis:
  >
  >   QDeadlineTimer::QDeadlineTimer(qint64 msecs, Qt::TimerType type) 
Q_DECL_NOTHROW
  >   : t2(0)
  >   {
  >   setRemainingTime(msecs, type);
  >   }
  >  
  >
  >
  > As t1 is not initialized and QDeadlineTimer::setPreciseRemainingTime only 
add to t1, it could be any value instead of 0?.
  >
  > Should I open a Qt bug?
  
  
  No, because your statement is incorrect. setPreciseRemainignTime *does* 
assign to t1:
  
*this = current(timerType);
if (QDeadlineTimerNanosecondsInT2) {
t1 += secs + toSecsAndNSecs(nsecs).first;
t2 += toSecsAndNSecs(nsecs).second;

REPOSITORY
  R271 KDBusAddons

REVISION DETAIL
  https://phabricator.kde.org/D14302

To: jtamate, dfaure, #frameworks, thiago
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14302: Don't block forever in ensureKdeinitRunning

[Thiago Macieira]

thiago added a comment.


  Quick testing via gdb:
  
Breakpoint 1, QLockFile::tryLock (this=0x7fffc6d8, timeout=0) at 
/home/tjmaciei/src/qt/qt5/qtbase/src/corelib/io/qlockfile.cpp:241
241 Q_D(QLockFile);
(gdb) n
242 QDeadlineTimer timer(qMax(timeout, -1));// QDT only takes 
-1 as "forever"
(gdb) 
243 int sleepTime = 100;
(gdb) 
245 d->lockError = d->tryLock_sys();
(gdb) 
246 switch (d->lockError) {
(gdb) 
254 if (!d->isLocked && d->isApparentlyStale()) {
(gdb) 
265 break;
(gdb) 
268 int remainingTime = timer.remainingTime();
(gdb) p timer
$4 = {t1 = 39043, t2 = 76902106, type = 1}
(gdb) s
QDeadlineTimer::remainingTime (this=0x7fffc590) at 
/home/tjmaciei/src/qt/qt5/qtbase/src/corelib/kernel/qdeadlinetimer.cpp:426
426 qint64 ns = remainingTimeNSecs();
(gdb) s
QDeadlineTimer::remainingTimeNSecs (this=0x7fffc590) at 
/home/tjmaciei/src/qt/qt5/qtbase/src/corelib/kernel/qdeadlinetimer.cpp:440
440 if (isForever())
(gdb) n
442 qint64 raw = rawRemainingTimeNSecs();
(gdb) n
443 return raw < 0 ? 0 : raw;
(gdb) p raw
$5 = -24344165069
(gdb) fin
Run till exit from #0  QDeadlineTimer::remainingTimeNSecs 
(this=0x7fffc590)
at 
/home/tjmaciei/src/qt/qt5/qtbase/src/corelib/kernel/qdeadlinetimer.cpp:443
0x77a1d014 in QDeadlineTimer::remainingTime (this=0x7fffc590)
at 
/home/tjmaciei/src/qt/qt5/qtbase/src/corelib/kernel/qdeadlinetimer.cpp:426
426 qint64 ns = remainingTimeNSecs();
Value returned is $6 = 0
(gdb) 
Run till exit from #0  0x77a1d014 in QDeadlineTimer::remainingTime 
(this=0x7fffc590)
at 
/home/tjmaciei/src/qt/qt5/qtbase/src/corelib/kernel/qdeadlinetimer.cpp:426
QLockFile::tryLock (this=0x7fffc6d8, timeout=0) at 
/home/tjmaciei/src/qt/qt5/qtbase/src/corelib/io/qlockfile.cpp:268
268 int remainingTime = timer.remainingTime();
Value returned is $7 = 0

REPOSITORY
  R271 KDBusAddons

REVISION DETAIL
  https://phabricator.kde.org/D14302

To: jtamate, dfaure, #frameworks, thiago
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D14302: Don't block forever in ensureKdeinitRunning

[Thiago Macieira]

thiago added a comment.


  In D14302#296479 , @dfaure wrote:
  
  > I agree about tryLock(0) should return immediately, tryLock(-1) should 
block forever -- I wrote that code and that docu ;-)
  >
  > Thiago wrote QDeadLineTimer later on though, and ported QLockFile to it. 
Thiago, any input?
  
  
  QDT has no escape path for 0. The constructor calls setRemainngTime(0), which 
calls setPreciseRemainingTime(0, 0), which will get the current time, add zero, 
and store it.
  
  After all of that, the QDT should return that the remaining time is zero, 
since it's expired.
  
  remainingTime() calls remainingTimeNSecs(), which calls 
rawRemainingTimeNSecs(), which should return a negative value. 
remainingTimeNSecs() should detect the negative and return 0; remainingTime 
detects the zero and returns it.
  
(gdb) print timer.remainingTime()
$11 = -1
  
  That's not supposed to happen.

REPOSITORY
  R271 KDBusAddons

REVISION DETAIL
  https://phabricator.kde.org/D14302

To: jtamate, dfaure, #frameworks, thiago
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D10273: Create proper SocketAddress

[Thiago Macieira]

thiago added a comment.


  That doesn't make sense. There's QFile::encodeName in the code.
  
  Is there a section of the library that is used in elevated privilege code, 
but not all of it?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10273

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D10273: Create proper SocketAddress

[Thiago Macieira]

thiago added a comment.


  Looks good.

INLINE COMMENTS

> fdsender.cpp:24
>  
>  FdSender::FdSender(const std::string &path)
>  : m_socketDes(-1)

The problem here is the API. Why is it using std::string in the first place?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D10273

To: chinmoyr, #frameworks
Cc: ossi, thiago, dfaure, michaelh, ngraham

D9966: [KIO] Fix issues with sharing of file descriptor

[Thiago Macieira]

thiago added a comment.


  The patch that I can see accomplishes what the description says it should do. 
And I agree with the idea of the patch.

INLINE COMMENTS

> dfaure wrote in fdreceiver.cpp:88
> This is UNIX-only, but indeed OSX is missing. Does that support getpeereid?

__APPLE__ is there. My question is about OpenBSD, NetBSD and DragonflyBSD. Are 
you breaking them? You may not know the answer, but you need to document the 
issue if you intentionally cause a Failure To Build From Sources.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D9966

To: chinmoyr, #frameworks, thiago, dfaure, ossi
Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

D9966: [KIO] Fix issues with sharing of file descriptor

[Thiago Macieira]

thiago added a comment.


  Why do you ask for a rewview then not wait for the review?
  
  Belated -1. No actual review of the change done, because I can't tell what 
you've done.

INLINE COMMENTS

> fdreceiver.cpp:34
>  {
> +SocketAddress addr(m_path.toStdString());
> +if (!addr.address()) {

Don't use toStdString(). I know this is what it used to do, but you can take 
the opportunity to fix the issue.

> fdreceiver.cpp:88
> +}
> +#else
> +#error Cannot get socket credentials!

Where's the support for other OSes?

> fdreceiver.h:45
>  int m_fileDes;
> +QString m_path;
>  };

Nitpick: always sort your members by size.

> file.h:106
>  bool privilegeOperationUnitTestMode();
> -PrivilegeOperationReturnValue execWithElevatedPrivilege(ActionType 
> action, const QVariant &arg1,
> -const QVariant 
> &arg2 = QVariant(),
> -const QVariant 
> &arg3 = QVariant());
> -PrivilegeOperationReturnValue tryOpen(QFile &f, const QByteArray &path, 
> int flags, int mode);
> +PrivilegeOperationReturnValue execWithElevatedPrivilege(ActionType 
> action, const QVariantList &args, int errcode);
> +PrivilegeOperationReturnValue tryOpen(QFile &f, const QByteArray &path, 
> int flags, int mode, int errcode);

This change should be in a separate commit. I can't tell what you're doing 
specifically to fix the bug with so much noise in this commit.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D9966

To: chinmoyr, #frameworks, thiago, dfaure, ossi
Cc: ngraham, fvogt, lbeltrame, dfaure, michaelh

Re: kcrash, fork, and stdout/stderr

[Thiago Macieira]

On Sunday, 3 December 2017 02:54:21 PST David Faure wrote:
> I'm trying to fix a bug (caught by CI) where KCrash, with the AutoRestart
> flag, restarts the crashing process directly (fork+execve) rather than via
> kdeinit.
> 
> The first process then exits, and whenever the child writes to stderr, it
> gets a SIGPIPE. Oops.
> 
> How do I set things up so that the child's stderr is basically what the
> parent's stderr was, in a way that works even if the parent exits?

That's the default. You have to figure out what you've done to change that.

You didn't say you're using QProcess.

[continued on the other email]

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center

Re: kcrash, fork, and stdout/stderr

[Thiago Macieira]

On Tuesday, 5 December 2017 04:12:30 PST David Faure wrote:
> > forking inside a signal handler is a bad idea because it may deadlock. If
> > the crash happens while glibc holds some mutexes relating to
> > pthread_atfork, then you'll have a problem.
> 
> I see. But how should one implement a crash handler that autorestarts an
> app, then, in a "standalone application" use case, i.e. no kdeinit or other
> daemon running in the background?

Use syscall(SYS_clone) instead of fork(). The system call always works, it's 
glibc's fork() internals that are known to have problems.

Linux-only, of course.

>From forkfd.c:

/* start the child - can't use fork() because it may deadlock */
#if defined(__NR_clone2)
child_pid = syscall(__NR_clone2, SIGCHLD, NULL, 0, NULL, NULL, NULL);
#elif defined(__cris__) || defined(__s390__)
child_pid = syscall(__NR_clone, NULL, SIGCHLD, NULL, NULL, 0L);
#else
child_pid = syscall(__NR_clone, SIGCHLD, NULL, NULL, NULL, 0L);
#endif
if (child_pid == 0) {
    /* child process - restore state */

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center

Re: kcrash, fork, and stdout/stderr

[Thiago Macieira]

On Tuesday, 5 December 2017 04:12:30 PST David Faure wrote:
> I see. But how should one implement a crash handler that autorestarts an
> app, then, in a "standalone application" use case, i.e. no kdeinit or other
> daemon running in the background?

Wait, why are you forking in the first place?

Just exec the process again. It will replace the current process without 
closing the pipes. The parent won't notice a thing.

Of course, that may be a problem: the parent may see the output from the child 
process again.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center

Re: kcrash, fork, and stdout/stderr

[Thiago Macieira]

On Monday, 4 December 2017 00:26:55 PST David Faure wrote:
> > Or do you fork a child at that point? fork from inside a signal handler is
> > an incredibly bad idea, don't do it.
> 
> Well, I guess that's why it's the fallback from the main strategy which is
> "ask kdeinit to restart that application" (even if it doesn't provide a
> kdeinit module). But kdeinit might not be running, outside plasma workspace.
> 
> This has always been like that, it goes back to 2006 (Luboš), it was
> discussed with you
> https://marc.info/?l=kde-core-devel&m=113699766611213&w=2 ("There's still a
> fallback to use fork() in case using kdeinit fails for any reason.")

Well, I've learnt a lot in the last 11 years.

forking inside a signal handler is a bad idea because it may deadlock. If the 
crash happens while glibc holds some mutexes relating to pthread_atfork, then 
you'll have a problem.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center

Re: kcrash, fork, and stdout/stderr

[Thiago Macieira]

On Sunday, 3 December 2017 12:12:53 PST David Faure wrote:
> > This should already work, unless the O_CLOEXEC flag has been set on the
> > file descriptor for stderr.  The kernel should duplicate open
> > descriptors on fork(2), and exec(2) should maintain its association to
> > any already-open descriptors.
> 
> Hmm then I don't understand the SIGPIPE.

SIGPIPE is sent to the writing process whenever the write would end in EPIPE. 
That's a legacy of Unix that makes sense for command-line applications that 
should exit if the downstream pipeline closed, but in non-graphical 
applications it makes no sense. It makes zero sense to do that for sockets...

I have a pending Linux kernel patch to disable that per file descriptor, 
following OpenBSD's API.

Anyway, moving on...

> > But, which process was the one writing to the terminal?  Do we know that
> > the destination of stderr was a PTY instead of being filtered through
> > some kind of kdeinit magic?
> 
> Yes, no kdeinit involved. Here's the setup:
> 
> $ bin/kcrashtest testAutoRestartDirectly
>   starts test_crasher (with QProcess, using waitForFinished())
>   which crashes :)
>   KCrash catches that, and since the Autorestart flag is set,
>  sets the env var KCRASH_AUTO_RESTARTED,
>  and starts test_crasher again (it checks that env var, so no
> infinite loop)
> 
> The first test_crasher then exits (after waitpid(), but that's just waiting
> for the child to start, right?).

Sorry, more details here. Can you provide the strace for clone, pipe, pipe2, 
dup, dup2, dup3, execve, waitpid, waitid, wait4?

QProcess starts a process. 

Does that process start something else? Or is QProcess's direct child that 
crashes?

The crash handler is in-process, correct? Then you execve in place?

Or do you fork a child at that point? fork from inside a signal handler is an 
incredibly bad idea, don't do it.

Or does the signal handler write to another process which in turn restarts the 
process?

> Is this because QProcess set up something for catching stderr, which no
> longer exists when the process exits?

Possibly. QProcess creates a pipe for stderr and it exists only so long as the 
direct child exists. Once the child exits, QProcess closes the reading end of 
the pipe and any process that tries to write to it will get SIGPIPE.

> Oh.
> proc.setProcessChannelMode(QProcess::ForwardedChannels);
> fixes it!

That makes QProcess not create a pipe and instead pass its own stderr to the 
child. Then it's SEP.

-- 
Thiago Macieira - thiago.macieira (AT) intel.com
  Software Architect - Intel Open Source Technology Center

D6709: [RFC] Add support for sharing file descriptor between KIO slave and KAuth helper

[Thiago Macieira]

thiago added a comment.


  This class isn't hooked up to anything. It's technically correct as an FD 
sender and receiver. What I want to see is how you use it, because that's 
extremely important to get right.
  
  I can confirm to you that anonymous namespace sockets do not work on BSDs 
(which include macOS).

INLINE COMMENTS

> sharefd.cpp:112
> +
> +m_socketDes = ::socket(AF_LOCAL, SOCK_STREAM, 0);
> +if (m_socketDes == -1)

If SOCK_NONBLOCK is defined, OR it o SOCK_STREAM and then you don't have call 
setNonBlocking below.

> sharefd.cpp:117
> +KSockaddrUn addr(path);
> +bool binded = bind(m_socketDes, addr.address(), addr.length()) != -1;
> +bool listening = listen(m_socketDes, 5) != -1;

"binded" is not English. You mean "bound".

> sharefd.h:33
> +
> +bool startListening(const std::string &path);
> +int fileDescriptor() const;

Use QString, not std::string.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D6709

To: chinmoyr, thiago, #frameworks
Cc: davidedmundson, elvisangelaccio, shortstheory

D5972: Set (and unset, as necessary) QT_NO_EXCEPTIONS for Clang (and ICC)

[Thiago Macieira]

thiago added a comment.


  In https://phabricator.kde.org/D5972#112994, @rjvbb wrote:
  
  > Yes, here too, haven't had time to read back up on and go through the whole 
Qt code review process. What branch should I target, anyway?
  
  
  5.9, with a possible backport to 5.6. There's still time for 5.6.3.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D5972

To: rjvbb, #build_system, #frameworks, kfunk
Cc: thiago, #frameworks, #build_system

D5972: Set (and unset, as necessary) QT_NO_EXCEPTIONS for Clang (and ICC)

[Thiago Macieira]

thiago added a comment.


  Makes sense to work around older versions of Qt without the fix.
  
  But it needs a fix. That is still pending.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D5972

To: rjvbb, #build_system, #frameworks, kfunk
Cc: thiago, #frameworks, #build_system

D5865: Add missing KDE_ENABLE_NAMED_OPERATORS function

[Thiago Macieira]

thiago added a comment.


  In https://phabricator.kde.org/D5865#112747, @rjvbb wrote:
  
  > In https://phabricator.kde.org/D5865#112743, @thiago wrote:
  >
  > > Fix the source code.
  > >
  > > If certain third-party libraries can't compile under MSVC 2015 (or even 
2017!) with default compiler options, they don't deserve to be used. Blacklist 
them.
  >
  >
  > That may work for Qt but is not an acceptable attitude for ECM, IMHO - and 
MSVC is problematic enough to build KF5 code for other reasons not to jump 
through hoops for it. You cannot expect FOSS projects to avoid Boost because it 
uses less common feature of the standard, big ole bad M$ cannot be bothered to 
write a proper compiler. I'd be all for blacklisting such compilers, if 
anything had to be blacklisted.
  
  
  This is in a kde.org codereview of a macro with KDE in the name. I think it's 
acceptable. Don't waste time with code that doesn't try to be cross-platform. 
Just let it fail to compile.
  
  And submit bug reports to those libraries. It's very easy for them to fix.
  
  > The projects I'm aware of that use "incriminated" boost code do provide 
MSWin builds but using a properly standards-compliant compiler.
  > 
  > My proposal is thus to drop MSVC support in the new macro, printing a 
warning should be enough.
  
  That's a fine solution. If you're going to print a warning and you're serious 
about cross-platformness support, I suggest printing the warning for ALL 
compilers: "Warning: using C++ operator names is not compatible with MSVC prior 
to version 2017 (19.1). You should reconsider using this macro if your code is 
meant to be cross-platform".

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D5865

To: rjvbb, #frameworks, #build_system, cgilles, kfunk
Cc: thiago, kfunk

D5865: Add missing KDE_ENABLE_NAMED_OPERATORS function

[Thiago Macieira]

thiago added a comment.


  In https://phabricator.kde.org/D5865#112737, @rjvbb wrote:
  
  > In https://phabricator.kde.org/D5865#112697, @thiago wrote:
  >
  > > Why are we spending time in named operators? It's much easier to fix the 
source code not to use them and then the problem goes away.
  >
  >
  > This is about adding a convenience macro for projects that have 
dependencies using named operators, like certain Boost modules.
  
  
  Fix the source code.
  
  If certain third-party libraries can't compile under MSVC 2015 (or even 
2017!) with default compiler options, they don't deserve to be used. Blacklist 
them.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D5865

To: rjvbb, #frameworks, #build_system, cgilles, kfunk
Cc: thiago, kfunk

D5865: Add missing KDE_ENABLE_NAMED_OPERATORS function

[Thiago Macieira]

thiago added a comment.


  In https://phabricator.kde.org/D5865#112679, @rjvbb wrote:
  
  > > That option only exists in MSVC 2017.
  >
  > Should we have deduce that from the docs I cite in the CMake comments? I'm 
willing to believe that I misread as far as support for named operators is 
concerned, but they do mention 2015 Update 3 specifically as the appearance of 
`/permissive-`.
  
  
  Documentation to the rescue:
  
  - MSVC 2015: https://msdn.microsoft.com/en-us/library/fwkeyyhe.aspx
  - MSVC 2017: 
https://docs.microsoft.com/en-us/cpp/build/reference/permissive-standards-conformance
  
  >> But that has nothing to do with the named keywords. And the problem is not 
the compiler, it's Microsoft's Windows SDK headers.
  > 
  > In what sense? Do they use reserved keywords or is the named operator 
support implemented in header files with macros or who knows what?
  
  Yes, and more. For example, MSVC 2013 had -Zs:strictStrings, but we couldn't 
turn it on because the headers would then fail to compile. This improved in 
MSVC 2015, so we turned it on.
  
  > And just to be clear, there is thus no reliable way to obtain support for 
named operators in MSVC, not even with v. 2017 (and foreseeable newer versions)?
  
  Why are we spending time in named operators? It's much easier to fix the 
source code not to use them and then the problem goes away.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D5865

To: rjvbb, #frameworks, #build_system, cgilles, kfunk
Cc: thiago, kfunk

D5865: Add missing KDE_ENABLE_NAMED_OPERATORS function

[Thiago Macieira]

thiago added a comment.


  In https://phabricator.kde.org/D5865#112549, @rjvbb wrote:
  
  > If a compiler is problematic in general it may not be worth it trying to 
account for it in a macro, trying to coerce it into doing something it cannot 
handle. I don't think that's a reason not to implement the macro at all though. 
Even if we cannot figure out from what version onwards the /fpermissive- option 
works it could still be useful for cross-platform code that really needs named 
operator support to let the macro print a warning (or raise an error) when it 
is built with MSVC.
  
  
  That option only exists in MSVC 2017. Qt 5.9.1 will use it for its own build. 
So far, so good.
  
  But that has nothing to do with the named keywords. And the problem is not 
the compiler, it's Microsoft's Windows SDK headers.

REPOSITORY
  R240 Extra CMake Modules

REVISION DETAIL
  https://phabricator.kde.org/D5865

To: rjvbb, #frameworks, #build_system, cgilles, kfunk
Cc: thiago, kfunk

[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists

[thiago (Thiago Macieira)]

thiago added a comment.


  In https://phabricator.kde.org/D2545#69187, @kfunk wrote:
  
  > In https://phabricator.kde.org/D2545#69091, @thiago wrote:
  >
  > > There doesn't seem to be a way of doing some clean up before the threads 
are forcibly killed.
  > >
  > > Maybe if I abuse qtmain().
  >
  >
  > This would only fix it for non-console GUI applications if I understand 
correctly. That's what we have here, but we'd still keep console applications 
buggy. There's no way to inject code before ExitProcess() with a plain console 
application.
  
  
  I was reading the ucrt source code yesterday and it does look like it 
processes atexit() registrations prior to ExitProcess(). So I'll go for that.
  
  There is a side-effect: I have to bring back the patch that makes QtDBus DLL 
non-unloadable, since otherwise it could crash during exit if had already been 
unloaded (was loaded by a plugin).
  
  BTW, do you know if this problem happens with MinGW?

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D2545

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: kfunk, vonreth, dfaure
Cc: thiago, albertvaka, mutlaqja, arrowdodger, #frameworks

[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists

[thiago (Thiago Macieira)]

thiago added a comment.


  More information on this Windows behaviour:
  
  - https://blogs.msdn.microsoft.com/oldnewthing/20070503-00/?p=27003
  - https://blogs.msdn.microsoft.com/oldnewthing/20070502-00/?p=27023/#2375204
  
  There doesn't seem to be a way of doing some clean up before the threads are 
forcibly killed.
  
  Maybe if I abuse qtmain().

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D2545

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: kfunk, vonreth, dfaure
Cc: thiago, albertvaka, mutlaqja, arrowdodger, #frameworks

[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists

[thiago (Thiago Macieira)]

thiago added a comment.


  In https://phabricator.kde.org/D2545#69083, @kfunk wrote:
  
  > > Here's the other problem: it's possible for threads to simply disappear 
on Windows. Given that I see "dllmain" in the backtrace (though not DllMain), I 
can't rule out that this has happened. Qt 5.6 has a workaround to another 
deadlock caused by Windows. Can you try to cherry-pick 
3ec57107cedb154f256e3ad001ea5475cc64fa94 from 5.8?
  >
  > With 3ec57107cedb154f256e3ad001ea5475cc64fa94 applied: Still dead-locking, 
same backtrace.
  
  
  Ok, so I guess this was a different, but similar issue, that got fixed by 
that commit.
  
  The root cause is that at some point during the process shut down (after 
ExitProcess is called), the Windows runtime kills all other threads, without 
giving them a chance to clean up. Because of that, when static destructors run, 
the other threads are no longer running, even though the objects that managed 
them (QThread) says they are. The application is in an inconsistent state.

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D2545

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: kfunk, vonreth, dfaure
Cc: thiago, albertvaka, mutlaqja, arrowdodger, #frameworks

[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists

[thiago (Thiago Macieira)]

thiago added a comment.


  In https://phabricator.kde.org/D2545#66904, @thiago wrote:
  
  > Commit ad66dbe305cff72443f4d3484191872d56e6dfbb in qtbase did try to solve 
this by disconnecting the senders when the objects were getting deleted. 
closeConnection() is called before the thread exits (from 
QDBusConnectionManager::run), so I don't know how there's still a 
BlockingQueuedConnection active.
  >
  > Is it possible that the service began being watched during destruction?
  
  
  I don't see how that's possible because there's another 
BlockingQueuedConnection protecting that: the QObject connection is only done 
in QDBusConnectionPrivate::addSignalHook, which is only run in the 
QDBusConnectionManager thread. So either the thread was still running, in which 
case we should have disconnected, or the thread wasn't running and the 
destroyed() signal should have got disconnected.
  
  Here's the other problem: it's possible for threads to simply disappear on 
Windows. Given that I see "dllmain" in the backtrace (though not DllMain), I 
can't rule out that this has happened. Qt 5.6 has a workaround to another 
deadlock caused by Windows. Can you try to cherry-pick 
3ec57107cedb154f256e3ad001ea5475cc64fa94 from 5.8?

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D2545

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: kfunk, vonreth, dfaure
Cc: thiago, albertvaka, mutlaqja, arrowdodger, #frameworks

[Differential] [Commented On] D2545: Cleanup KSharedUiServerProxy before qApp exists

[thiago (Thiago Macieira)]

thiago added a comment.


  In https://phabricator.kde.org/D2545#66545, @kfunk wrote:
  
  > In https://phabricator.kde.org/D2545#65976, @dfaure wrote:
  >
  > > Actually, I think Thiago's still waiting for a backtrace of all threads.
  >
  >
  > I think I've already said this via other channels: There's only one thread 
left at this point.
  
  
  That's troubling. That means the QDBusConnectionManager thread has exited but 
the QDBusConnectionPrivate object still exists. That is refcounted and deletes 
itself, though QDBusServicesWatcher is not the reason for refcounting.
  
  Commit ad66dbe305cff72443f4d3484191872d56e6dfbb in qtbase did try to solve 
this by disconnecting the senders when the objects were getting deleted. 
closeConnection() is called before the thread exits (from 
QDBusConnectionManager::run), so I don't know how there's still a 
BlockingQueuedConnection active.
  
  Is it possible that the service began being watched during destruction?

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D2545

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: kfunk, vonreth, dfaure
Cc: thiago, albertvaka, mutlaqja, arrowdodger, #frameworks

Re: Review Request 128790: Call utempter manually

[Thiago Macieira]

> On Sept. 4, 2016, 2:13 p.m., Thiago Macieira wrote:
> >

What happens if one builds on a machine without utempter? Are we still linking 
to libutempter?


- Thiago


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128790/#review98873
---


On Sept. 4, 2016, 10:20 a.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128790/
> ---
> 
> (Updated Sept. 4, 2016, 10:20 a.m.)
> 
> 
> Review request for KDE Frameworks, David Faure, Kurt Hindenburg, Oswald 
> Buddenhagen, Rex Dieter, and Thiago Macieira.
> 
> 
> Bugs: 364779
> https://bugs.kde.org/show_bug.cgi?id=364779
> 
> 
> Repository: kpty
> 
> 
> Description
> ---
> 
> According to the investigation in https://bugs.kde.org/show_bug.cgi?id=364779 
> utempter does stuff in a way that isn't compatible with 
> QProcess/KProcess/KPtyProcess (calling sigaction() before launching its child 
> process).
> 
> So instead we invoke it manually with QProcess.
> 
> 
> Diffs
> -
> 
>   cmake/FindUTEMPTER.cmake a3ea06a 
>   src/kpty.cpp 15c3b81 
>   src/kpty_p.h 192bf1c 
> 
> Diff: https://git.reviewboard.kde.org/r/128790/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

Re: Review Request 128790: Call utempter manually

[Thiago Macieira]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128790/#review98873
---




src/kpty.cpp (line 85)
<https://git.reviewboard.kde.org/r/128790/#comment66569>

Document why you're using 3 instead of the expected 2, here.


- Thiago Macieira


On Sept. 4, 2016, 10:20 a.m., Martin Tobias Holmedahl Sandsmark wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128790/
> ---
> 
> (Updated Sept. 4, 2016, 10:20 a.m.)
> 
> 
> Review request for KDE Frameworks, David Faure, Kurt Hindenburg, Oswald 
> Buddenhagen, Rex Dieter, and Thiago Macieira.
> 
> 
> Bugs: 364779
> https://bugs.kde.org/show_bug.cgi?id=364779
> 
> 
> Repository: kpty
> 
> 
> Description
> ---
> 
> According to the investigation in https://bugs.kde.org/show_bug.cgi?id=364779 
> utempter does stuff in a way that isn't compatible with 
> QProcess/KProcess/KPtyProcess (calling sigaction() before launching its child 
> process).
> 
> So instead we invoke it manually with QProcess.
> 
> 
> Diffs
> -
> 
>   cmake/FindUTEMPTER.cmake a3ea06a 
>   src/kpty.cpp 15c3b81 
>   src/kpty_p.h 192bf1c 
> 
> Diff: https://git.reviewboard.kde.org/r/128790/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Martin Tobias Holmedahl Sandsmark
> 
>

Re: [kde-community] Usage of QNetworkAccessManager

[Thiago Macieira]

On quinta-feira, 14 de julho de 2016 18:33:37 PDT Ben Cooksley wrote:
> Hi all,
> 
> Just my regular reminder regarding usage of QNetworkAccessManager in
> your applications and libraries, especially when it comes to
> interacting with kde.org infrastructure.
> 
> Unfortunately, from it's first iteration in Qt 4 QNetworkAccessManager
> was shipped with a severe and fundamental defect in that it does not
> follow HTTP redirects by default. Due to Qt behavioural and other
> compatibility promises they can never change this behaviour, not even
> in Qt 6.
> 
> Please therefore ensure your application handles redirects
> appropriately (the form of the code will depend on the version of Qt
> in use) if you decide to use QNAM.

You do that by setting the attribute FollowRedirectsAttribute in your 
QNetworkRequests.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Deadlock in kded whith Qt 5.6

[Thiago Macieira]

On Friday 22 January 2016 09:21:12 David Faure wrote:
> On Thursday 21 January 2016 13:54:32 Thiago Macieira wrote:
> > Can I limit the spy hook to method calls? That is, exclude signals.
> 
> Yes, for the purpose of kded/kiod that is fine
> A not-loaded-yet-module cannot possibly connect to a dbus signal.
> 
> > Another attempt, also untested.
> 
> It works !
> 
> Even the proxyscout module with its usage of system bus in the module ctor
> gets autostarted+autoloaded just fine.
> 
> Thanks Thiago.

You're welcome. This version is simpler too.

I've just uploaded PS 4 with this version and an updated commit message.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
  PGP/GPG: 0x6EF45358; fingerprint:
  E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Deadlock in kded whith Qt 5.6

[Thiago Macieira]

On Thursday 21 January 2016 08:37:38 David Faure wrote:
> On Wednesday 20 January 2016 08:41:31 Thiago Macieira wrote:
> > On Wednesday 20 January 2016 09:08:38 David Faure wrote:
> > > > Can you run with QDBUS_DEBUG=1 and tell me the messages it prints?
> > 
> > [cut]
> > 
> > I didn't see any "invoking message spies" of the debug I left. Are you
> > sure
> > this had the hook installed?
> 
> You mean the messageFilter? That's exactly the issue, that it's not getting
> called. Therefore autoload doesn't happen.

Another attempt, also untested.
-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
  PGP/GPG: 0x6EF45358; fingerprint:
  E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358
>From 10191ce9c6d31225442fde85c6d4470681d4f511 Mon Sep 17 00:00:00 2001
From: Thiago Macieira 
Date: Tue, 19 Jan 2016 22:29:33 -0800
Subject: [PATCH 1/1] Call out to QtDBus message spies in the main thread

Whenever there are spies installed, we call out to the main thread to
call to the kded/kiod message spies. This allows the spy code to do just
about anything, where previously it was restricted in what it could do
to avoid deadlocking or triggering assertions if it recursed back into
QDBusConnection code in the manager thread.

After the spies are done with, the message is re-inserted into the
QDBusConnection processing pipeline. That means it may be delayed again
if the dispatching is disabled.

Change-Id: I3d11545be52c43119f0f142b0e9d447415c2
---
 src/dbus/qdbusconnection_p.h |  3 ++-
 src/dbus/qdbusintegrator_p.h | 19 ++
 src/dbus/qdbusintegrator.cpp | 48 +---
 3 files changed, 57 insertions(+), 13 deletions(-)

diff --git a/src/dbus/qdbusconnection_p.h b/src/dbus/qdbusconnection_p.h
index f030a3f..c77daf7 100644
--- a/src/dbus/qdbusconnection_p.h
+++ b/src/dbus/qdbusconnection_p.h
@@ -1,7 +1,7 @@
 /
 **
 ** Copyright (C) 2015 The Qt Company Ltd.
-** Copyright (C) 2015 Intel Corporation.
+** Copyright (C) 2016 Intel Corporation.
 ** Contact: http://www.qt.io/licensing/
 **
 ** This file is part of the QtDBus module of the Qt Toolkit.
@@ -281,6 +281,7 @@ private slots:
 
 signals:
 void dispatchStatusChanged();
+void spyHooksFinished(const QDBusMessage &msg);
 void messageNeedsSending(QDBusPendingCallPrivate *pcall, void *msg, int timeout = -1);
 void signalNeedsConnecting(const QString &key, const QDBusConnectionPrivate::SignalHook &hook);
 bool signalNeedsDisconnecting(const QString &key, const QDBusConnectionPrivate::SignalHook &hook);
diff --git a/src/dbus/qdbusintegrator_p.h b/src/dbus/qdbusintegrator_p.h
index 2038ac9..e266512 100644
--- a/src/dbus/qdbusintegrator_p.h
+++ b/src/dbus/qdbusintegrator_p.h
@@ -1,6 +1,7 @@
 /
 **
 ** Copyright (C) 2015 The Qt Company Ltd.
+** Copyright (C) 2016 Intel Corporation.
 ** Contact: http://www.qt.io/licensing/
 **
 ** This file is part of the QtDBus module of the Qt Toolkit.
@@ -65,6 +66,7 @@
 QT_BEGIN_NAMESPACE
 
 class QDBusConnectionPrivate;
+class QDBusMessage;
 
 // Really private structs used by qdbusintegrator.cpp
 // Things that aren't used by any other file
@@ -133,6 +135,23 @@ private:
 bool handled;
 };
 
+class QDBusSpyCallEvent : public QMetaCallEvent
+{
+public:
+typedef void (*Hook)(const QDBusMessage&);
+QDBusSpyCallEvent(QDBusConnectionPrivate *cp, const QDBusConnection &c, const QDBusMessage &msg,
+  const Hook *hooks, int count)
+: QMetaCallEvent(0, 0, Q_NULLPTR, cp, 0), conn(c), msg(msg), hooks(hooks), hookCount(count)
+{}
+~QDBusSpyCallEvent();
+void placeMetaCall(QObject *) Q_DECL_OVERRIDE;
+
+QDBusConnection conn;   // keeps the refcount in QDBusConnectionPrivate up
+QDBusMessage msg;
+const Hook *hooks;
+int hookCount;
+};
+
 QT_END_NAMESPACE
 
 Q_DECLARE_METATYPE(QDBusSlotCache)
diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp
index dffdfe2..ebf42b0 100644
--- a/src/dbus/qdbusintegrator.cpp
+++ b/src/dbus/qdbusintegrator.cpp
@@ -1,7 +1,7 @@
 /
 **
 ** Copyright (C) 2015 The Qt Company Ltd.
-** Copyright (C) 2015 Intel Corporation.
+** Copyright (C) 2016 Intel Corporation.
 ** Contact: http://www.qt.io/licensing/
 **
 ** This file is part of the QtDBus module of the Qt Toolkit.
@@ -120,8 +120,7 @@ void qdbusDefaultThreadDebug(int action, int condition, QDBusConnectionPrivate *
 qdbusThreadDebugFunc qdbusThreadDebug = 0;
 #endif
 
-typedef void (*QDBusSpyHook)(const QDBusMessage&);
-typedef QVarLengthArray QDBusSpyHookList;
+typedef QVa

Re: Deadlock in kded whith Qt 5.6

[Thiago Macieira]

On Thursday 21 January 2016 08:37:38 David Faure wrote:
> > I didn't see any "invoking message spies" of the debug I left. Are you
> > sure
> > this had the hook installed?
> 
> You mean the messageFilter? That's exactly the issue, that it's not getting
> called. Therefore autoload doesn't happen.
> 
> It might be a consequence of your previous commit (1f6fa1f37a14742dd),
> not necessarily a problem in the current patch under test.

I don't see how. The spy hooks are called before the queue would get 
suspended. We can invert the order to see if it helps, but I don't see how it 
would help or hinder. It would make the code more complex.

Can I limit the spy hook to method calls? That is, exclude signals.

> > For that matter, can you apply the attached patch so we also get feedback
> > on the queued delivery?
> 
> With this patch applied, here's one log for the first call
> (autostart+autoload, doesn't work)
> 
> http://www.davidfaure.fr/2016/first_call.txt

This one says "delivery is suspended" a couple of times. But as I said, the 
spy processing happens *before* this, with conditional:

if (Q_UNLIKELY(qDBusSpyHookList.exists()) && qApp) {

The list exists if you've installed a hook. Is it possible that this is 
happening before the QCoreApplication is created?

Note the race condition: you should install the hook *before* connecting to 
the bus. But even if that were the case, we should still be seeing the spy 
later.

> and one log for the second call (kded already started, autoload works)
> 
> http://www.davidfaure.fr/2016/second_call.txt

This one shows "invoking message spies"
-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
  PGP/GPG: 0x6EF45358; fingerprint:
  E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Deadlock in kded whith Qt 5.6

[Thiago Macieira]

On Wednesday 20 January 2016 09:08:38 David Faure wrote:
> > Can you run with QDBUS_DEBUG=1 and tell me the messages it prints?
> 
[cut]

I didn't see any "invoking message spies" of the debug I left. Are you sure 
this had the hook installed?

For that matter, can you apply the attached patch so we also get feedback on 
the queued delivery?

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
  PGP/GPG: 0x6EF45358; fingerprint:
  E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358
--- src/dbus/qdbusintegrator.cpp
+++ src/dbus/qdbusintegrator.cpp
@@ -525,6 +525,7 @@ bool QDBusConnectionPrivate::handleMessage(const QDBusMessage &amsg)
 return false;
 if (!dispatchEnabled && !QDBusMessagePrivate::isLocal(amsg)) {
 // queue messages only, we'll handle them later
+qDBusDebug() << this << "delivery is suspended";
 pendingMessages << amsg;
 return amsg.type() == QDBusMessage::MethodCallMessage;
 }
@@ -1119,8 +1120,10 @@ void QDBusConnectionPrivate::doDispatch()
 // dispatch previously queued messages
 PendingMessageList::Iterator it = pendingMessages.begin();
 PendingMessageList::Iterator end = pendingMessages.end();
-for ( ; it != end; ++it)
+for ( ; it != end; ++it) {
+qDBusDebug() << this << "got queued message" << *it;
 handleMessage(qMove(*it));
+}
 pendingMessages.clear();
 }
 }
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Deadlock in kded whith Qt 5.6

[Thiago Macieira]

On Wednesday 20 January 2016 08:55:17 David Faure wrote:
> Doesn't seem to support service autostart + module autoload together.

> but if I kill kded5 again and do this right away:
> $ qdbus org.kde.kcookiejar5  /modules/kcookiejar
> then it doesn't work: kded5 gets started, but the call gets this reply
> Error: org.freedesktop.DBus.Error.UnknownObject
> No such object path '/modules/kcookiejar'
> 

> The messageFilter doesn't run at all, when doing autostart+autoload
> together.

I need a little more debugging. The limited debugging I did here shows that 
the spy hooks do get called and in the right (main) thread. And since the 
hooks are processed before the main dispatcher queue, it shouldn't affect the 
previous code.

Can you run with QDBUS_DEBUG=1 and tell me the messages it prints?

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
  PGP/GPG: 0x6EF45358; fingerprint:
  E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Deadlock in kded whith Qt 5.6

[Thiago Macieira]

On Monday 18 January 2016 09:23:51 David Faure wrote:
> Thiago: indeed the BlockingQueuedConnection call from messageFilter to the
> main thread occasionnally leads to deadlocks, we need your
> "postpone incoming messages" idea.

Please try the attached patch, also found at
https://codereview.qt-project.org/146639
-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
  PGP/GPG: 0x6EF45358; fingerprint:
  E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358
>From fafa4b3b89a02e136f8f4af8455ee2fd8a7cf9f4 Mon Sep 17 00:00:00 2001
From: Thiago Macieira 
Date: Tue, 19 Jan 2016 22:29:33 -0800
Subject: [PATCH 1/1] Call out to QtDBus message spies in the main thread

Whenever there are spies installed, we call out to the main thread to
call to the kded/kiod message spies. This allows the spy code to do just
about anything, where previously it was restricted in what it could do
to avoid deadlocking or triggering assertions if it recursed back into
QDBusConnection code in the manager thread.

After the spies are done with, the message is re-inserted into the
QDBusConnection processing pipeline. That means it may be delayed again
if the dispatching is disabled.

Change-Id: I3d11545be52c43119f0f142b0e9d447415c2
---
 src/dbus/qdbusconnection_p.h |  4 +++-
 src/dbus/qdbusintegrator_p.h | 19 +
 src/dbus/qdbusintegrator.cpp | 51 ++--
 3 files changed, 62 insertions(+), 12 deletions(-)

diff --git a/src/dbus/qdbusconnection_p.h b/src/dbus/qdbusconnection_p.h
index f030a3f..d7787fa 100644
--- a/src/dbus/qdbusconnection_p.h
+++ b/src/dbus/qdbusconnection_p.h
@@ -1,7 +1,7 @@
 /
 **
 ** Copyright (C) 2015 The Qt Company Ltd.
-** Copyright (C) 2015 Intel Corporation.
+** Copyright (C) 2016 Intel Corporation.
 ** Contact: http://www.qt.io/licensing/
 **
 ** This file is part of the QtDBus module of the Qt Toolkit.
@@ -228,6 +228,7 @@ public:
 void registerService(const QString &serviceName);
 void unregisterService(const QString &serviceName);
 
+bool preHandleMessage(const QDBusMessage &msg);
 bool handleMessage(const QDBusMessage &msg);
 
 QDBusMetaObject *findMetaObject(const QString &service, const QString &path,
@@ -281,6 +282,7 @@ private slots:
 
 signals:
 void dispatchStatusChanged();
+void messageNeedsHandling(const QDBusMessage &msg);
 void messageNeedsSending(QDBusPendingCallPrivate *pcall, void *msg, int timeout = -1);
 void signalNeedsConnecting(const QString &key, const QDBusConnectionPrivate::SignalHook &hook);
 bool signalNeedsDisconnecting(const QString &key, const QDBusConnectionPrivate::SignalHook &hook);
diff --git a/src/dbus/qdbusintegrator_p.h b/src/dbus/qdbusintegrator_p.h
index 2038ac9..e266512 100644
--- a/src/dbus/qdbusintegrator_p.h
+++ b/src/dbus/qdbusintegrator_p.h
@@ -1,6 +1,7 @@
 /
 **
 ** Copyright (C) 2015 The Qt Company Ltd.
+** Copyright (C) 2016 Intel Corporation.
 ** Contact: http://www.qt.io/licensing/
 **
 ** This file is part of the QtDBus module of the Qt Toolkit.
@@ -65,6 +66,7 @@
 QT_BEGIN_NAMESPACE
 
 class QDBusConnectionPrivate;
+class QDBusMessage;
 
 // Really private structs used by qdbusintegrator.cpp
 // Things that aren't used by any other file
@@ -133,6 +135,23 @@ private:
 bool handled;
 };
 
+class QDBusSpyCallEvent : public QMetaCallEvent
+{
+public:
+typedef void (*Hook)(const QDBusMessage&);
+QDBusSpyCallEvent(QDBusConnectionPrivate *cp, const QDBusConnection &c, const QDBusMessage &msg,
+  const Hook *hooks, int count)
+: QMetaCallEvent(0, 0, Q_NULLPTR, cp, 0), conn(c), msg(msg), hooks(hooks), hookCount(count)
+{}
+~QDBusSpyCallEvent();
+void placeMetaCall(QObject *) Q_DECL_OVERRIDE;
+
+QDBusConnection conn;   // keeps the refcount in QDBusConnectionPrivate up
+QDBusMessage msg;
+const Hook *hooks;
+int hookCount;
+};
+
 QT_END_NAMESPACE
 
 Q_DECLARE_METATYPE(QDBusSlotCache)
diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp
index dffdfe2..652924f 100644
--- a/src/dbus/qdbusintegrator.cpp
+++ b/src/dbus/qdbusintegrator.cpp
@@ -1,7 +1,7 @@
 /
 **
 ** Copyright (C) 2015 The Qt Company Ltd.
-** Copyright (C) 2015 Intel Corporation.
+** Copyright (C) 2016 Intel Corporation.
 ** Contact: http://www.qt.io/licensing/
 **
 ** This file is part of the QtDBus module of the Qt Toolkit.
@@ -120,8 +120,7 @@ void qdbusDefaultThreadDebug(int action, int condition, QDBusConnectionPrivate *
 qdbusThreadDebugFunc qdbusThreadDebug = 0;
 #endif
 
-typedef void (*QDBusSpyHook)(const QDBusMessage&

Re: Deadlock in kded whith Qt 5.6

[Thiago Macieira]

On Tuesday 19 January 2016 15:56:47 you wrote:
> I briefly talked about this with David yesterday and looked at the code.
> What happened previously in kded5:
> - it installs a message filter using libdbus API

QtDBus API. In fact, it's QtDBus API that exists and has existed only for kded 
(and now kiod).

> - when a message to a not-yet-loaded module comes along, the module
>   is loaded on the fly and directly from the message filter function
>   before the message is allowed to continue on its way
> 
> What happens now:
> - the D-Bus connection lives in its own thread, in order to facilitate
>   using the connection from arbitrary threads in the application
> - this puts the message filter in the D-Bus thread
> - multithreading issues ensue

Because the new QtDBus code assumes that certain functions will always be 
called from any-thread-but-QDBusConnectionManager's, as the code is very 
careful to not ever call to user code in the manager's thread. The spy 
function breaks that promise and I didn't catch it when designing the threaded 
model.

> At the moment, you can pick between various poisons and eventually you
> have two threads waiting for each other and kded5 is mostly deadlocked.
> The suggested solution is probably to *queue* messages to not-yet-loaded
> modules and to continue processing other D-Bus message while the
> required modules for the queued message are loading. That would break
> reasonable expectations about message ordering and blocking the least, I
> guess.

Actually, we'll queue them all, since we don't know which messages trigger 
loading and which ones don't. So for every incoming message, we'll call out to 
the spy function in the main thread and, after it returns, we'll reinject the 
message into the normal QtDBus processing.

> Regarding the current situation, the synchronous module loading was okay
> before the Qt change unless modules somehow made D-Bus calls to
> something else than the daemon (to register their address) just by being
> loaded.
> We have had lots of kded5 issues due to blocking calls over the years
> but I don'r remember seeing module loading in any backtraces.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
  PGP/GPG: 0x6EF45358; fingerprint:
  E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 119011: KInit: call setgroups(0, 0) before calling setgid()

[Thiago Macieira]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/119011/#review88231
---


This broke the ability of users to have more than one group (usermod), for 
groups like vboxusers and systemd-journal. Now, start_kdeinit unconditionally 
drops all groups and that's wrong.

It should call getgrouplist(3) and set those groups on the user.

Besides, I'm not convinced the rpmlint warning was correct.

- Thiago Macieira


On Julho 1, 2014, 10:21 a.m., Daniel Vrátil wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119011/
> ---
> 
> (Updated Julho 1, 2014, 10:21 a.m.)
> 
> 
> Review request for KDE Frameworks.
> 
> 
> Repository: kinit
> 
> 
> Description
> ---
> 
> While packaging kinit, we got a warning from rpmlint that start_kdeinit calls 
> setgid() without calling setgroups() first. From rpmlint:
> 
>This executable is calling setuid and setgid without setgroups or 
> initgroups.
>There is a high probability this mean it didn't relinquish all groups, and
>this would be a potential security issue to be fixed. Seek POS36-C on the 
> web
>for details about the problem.
> 
> The reasoning is that when you drop privileges from root to regular user, 
> there might be some extra groups left that, if not cleared, might grant the 
> process privileges to do superuser things.
> 
> The code does not check for return value, as the call will fail if we are not 
> a superuser.
> 
> This oneliner makes rpmlint happy and maybe prevents a security issue.
> 
> 
> Diffs
> -
> 
>   src/start_kdeinit/start_kdeinit.c 07a28d3 
> 
> Diff: https://git.reviewboard.kde.org/r/119011/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Daniel Vrátil
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124856: Call newInstance from the child on first invocation

[Thiago Macieira]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124856/#review85847
---


David: please note that the QtDBus changes made it in for 5.6, not 5.7. If you 
want to discuss suspending the execution of the thread, please start that 
discussion. I have the beginnings of a hackish patch for that...

- Thiago Macieira


On Set. 23, 2015, 7:44 p.m., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124856/
> ---
> 
> (Updated Set. 23, 2015, 7:44 p.m.)
> 
> 
> Review request for KDE Frameworks and Thiago Macieira.
> 
> 
> Repository: kdelibs4support
> 
> 
> Description
> ---
> 
> For the first invocation, call newInstance ourselves instead of relying on 
> the parent process to do it.
> 
> With the new threading patches in qt dbus the current code races as the 
> parent may end up calling newInstance before the child has set up the object 
> that handles it.
> 
> 
> Diffs
> -
> 
>   src/kdeui/kuniqueapplication.cpp 713c6f4 
>   src/kdeui/kuniqueapplication_p.h de4b328 
> 
> Diff: https://git.reviewboard.kde.org/r/124856/diff/
> 
> 
> Testing
> ---
> 
> konsole starts fine with this + Thiago's Qt dbus threading patches
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 125220: KService: use a real global static for QThreadStorage rather than Q_GLOBAL_STATIC.

[Thiago Macieira]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/125220/#review85338
---


Aye, QThreadStorage isn't meant to be used in a Q_GLOBAL_STATIC. I don't 
understand what you meant by id: QThreadStorageData::get can only be called 
once the construction has finished, in which case there aren't multiple threads 
racing.

- Thiago Macieira


On Set. 13, 2015, 10:10 p.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/125220/
> ---
> 
> (Updated Set. 13, 2015, 10:10 p.m.)
> 
> 
> Review request for KDE Frameworks, Olivier Goffart and Thiago Macieira.
> 
> 
> Repository: kservice
> 
> 
> Description
> ---
> 
> Creating QThreadStorage on demand leads to race conditions because the
> id of the storagedata gets assigned once multiple threads are running,
> and read from other threads without locking. I almost submitted a fix for
> Qt when I realized the indirection I had here was unnecessary and wrong.
> 
> QThreadStorage must be created in the main thread, before any thread is 
> running.
> 
> 
> Diffs
> -
> 
>   src/services/kservicetypefactory.cpp 
> 62fd230b4f7a6558c707d257796c5967f39c3607 
>   src/services/kservicegroupfactory.cpp 
> 08e0bdcd765ab08afdb71cabc640fd21a73f4218 
>   src/services/kservicefactory.cpp 9b0e0c199818fea774c08a4f8fab5aca417927c8 
>   src/services/kmimetypefactory.cpp ba07aa0bc9b5d528454cf426b1feadb049402123 
> 
> Diff: https://git.reviewboard.kde.org/r/125220/diff/
> 
> 
> Testing
> ---
> 
> helgrind ksycocathreadtest no longer complains about a race on 
> QThreadStorageData::id
> 
> 
> Thanks,
> 
> David Faure
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124856: Call newInstance from the child on first invocation

[Thiago Macieira]

> On Ago. 21, 2015, 8:08 a.m., David Faure wrote:
> > I thought the idea was "there's no race, because as long as the recipient 
> > doesn't go to the event loop, it won't be processing any incoming messages 
> > anyway"?
> 
> Albert Astals Cid wrote:
> But with Thiago's patches dbus runs in a thread so that's not true 
> anymore.
> 
> David Faure wrote:
> Why don't Thiago's patches preserve behavior compatibility by 
> starting/enabling the dbus thread only when reaching the event loop?
> 
> Otherwise it sounds like any case of "talking to an application that is 
> starting" will create races (am I making the call before or after the object 
> has been published?). Not just KUniqueApplication, but any other dbus call to 
> a starting app, including the dbus activation case.
> 
> Albert Astals Cid wrote:
> I don't know, he knows better, hope he has time to answer. 
> 
> And anyhow even if you disagree with his patches to Qt dbus, there's no 
> reason to discard this patch, saves us a dbus call on the startup case, makes 
> the app start marginally faster i guess :D

Right, the patch itself looks fine and I'd say "ship it".

As for the behaviour-compatibility... it's because I had not thought of it 
until now. And if it weren't for the QDaemonThread, it wouldn't be doing that. 
I guess we can discuss what to do -- back to the ML for this.


- Thiago


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124856/#review84129
---


On Ago. 21, 2015, 4:06 p.m., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124856/
> ---
> 
> (Updated Ago. 21, 2015, 4:06 p.m.)
> 
> 
> Review request for KDE Frameworks and Thiago Macieira.
> 
> 
> Repository: kdelibs4support
> 
> 
> Description
> ---
> 
> For the first invocation, call newInstance ourselves instead of relying on 
> the parent process to do it.
> 
> With the new threading patches in qt dbus the current code races as the 
> parent may end up calling newInstance before the child has set up the object 
> that handles it.
> 
> 
> Diffs
> -
> 
>   src/kdeui/kuniqueapplication.cpp 713c6f4 
>   src/kdeui/kuniqueapplication_p.h de4b328 
> 
> Diff: https://git.reviewboard.kde.org/r/124856/diff/
> 
> 
> Testing
> ---
> 
> konsole starts fine with this + Thiago's Qt dbus threading patches
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 124856: Call newInstance from the child on first invocation

[Thiago Macieira]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124856/#review84149
---


Makes sense!

- Thiago Macieira


On Ago. 21, 2015, 4:06 p.m., Albert Astals Cid wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124856/
> ---
> 
> (Updated Ago. 21, 2015, 4:06 p.m.)
> 
> 
> Review request for KDE Frameworks and Thiago Macieira.
> 
> 
> Repository: kdelibs4support
> 
> 
> Description
> ---
> 
> For the first invocation, call newInstance ourselves instead of relying on 
> the parent process to do it.
> 
> With the new threading patches in qt dbus the current code races as the 
> parent may end up calling newInstance before the child has set up the object 
> that handles it.
> 
> 
> Diffs
> -
> 
>   src/kdeui/kuniqueapplication.cpp 713c6f4 
>   src/kdeui/kuniqueapplication_p.h de4b328 
> 
> Diff: https://git.reviewboard.kde.org/r/124856/diff/
> 
> 
> Testing
> ---
> 
> konsole starts fine with this + Thiago's Qt dbus threading patches
> 
> 
> Thanks,
> 
> Albert Astals Cid
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: KAuth and KF5

[Thiago Macieira]

Em seg 30 jun 2014, às 14:14:10, Milian Wolff escreveu:
> On Monday 30 June 2014 00:05:10 šumski wrote:
> > On Thursday 26 of June 2014 12:14:49 Milian Wolff wrote:
> > > Hey,
> > > 
> > > did you run it through valgrind?
> > 
> > Here's what valgrind says:
> Sounds like a bug in Qt to me, I have to say. Looking at the code,
> QDBusConnectionPrivate::objectDestroyed looks pretty fragile, I mean it does
> this at the end:
> 
> obj->disconnect(this);
> 
> But from the code in QDBusConnectionPrivate::disconnectSignal nothing jumps
> out as dangerous directly. The fact, that valgrind is getting confused in
> the stack trace is not helping either ;-) Could you maybe try to compile
> qtbase in debug mode and reproduce the issue, such that we get a full
> stacktrace without optimizations etc.?
> 
> Anyways, maybe Thiago (CC'ed) can give us some insight on whats going on
> here.

This is happening in a global destructor during the use of a global static. My 
guess would be that the global static has already been destroyed, hence the 
issue.

Try this patch, which removes it. We have QStringLiteral nowadays.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
  PGP/GPG: 0x6EF45358; fingerprint:
  E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358
>From eda5c8ede9fd35117146d13f1b55775c9007b672 Mon Sep 17 00:00:00 2001
From: Thiago Macieira 
Date: Mon, 30 Jun 2014 08:31:22 -0700
Subject: [PATCH 1/1] Replace the const QString global static with a
 QStringLiteral

It was originally created to avoid allocating memory for the QString at
every turn, but we have QStringLiteral for that today. It has also
served a very good run by catching qatomic.h implementations that had
bad cv qualifications.

Change-Id: Id6d952b8cce363015ec2611d346b4cccedecf137
---
 src/dbus/qdbusintegrator.cpp | 14 ++
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/src/dbus/qdbusintegrator.cpp b/src/dbus/qdbusintegrator.cpp
index d4079e8..bc28d50 100644
--- a/src/dbus/qdbusintegrator.cpp
+++ b/src/dbus/qdbusintegrator.cpp
@@ -76,15 +76,21 @@ QT_BEGIN_NAMESPACE
 static bool isDebugging;
 #define qDBusDebug  if (!::isDebugging); else qDebug
 
-Q_GLOBAL_STATIC_WITH_ARGS(const QString, orgFreedesktopDBusString, (QLatin1String(DBUS_SERVICE_DBUS)))
+QString orgFreedesktopDBusString()
+{
+return QStringLiteral(DBUS_SERVICE_DBUS);
+}
 
 static inline QString dbusServiceString()
-{ return *orgFreedesktopDBusString(); }
+{
+return orgFreedesktopDBusString();
+}
+
 static inline QString dbusInterfaceString()
 {
 // it's the same string, but just be sure
-Q_ASSERT(*orgFreedesktopDBusString() == QLatin1String(DBUS_INTERFACE_DBUS));
-return *orgFreedesktopDBusString();
+Q_ASSERT(orgFreedesktopDBusString() == QLatin1String(DBUS_INTERFACE_DBUS));
+return orgFreedesktopDBusString();
 }
 
 static inline QDebug operator<<(QDebug dbg, const QThread *th)
-- 
1.8.4.5

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118614: Make KIO thread-safe

[Thiago Macieira]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118614/#review60288
---


Since there was no problem in QtDBus after all, this patch can be reverted.

- Thiago Macieira


On June 14, 2014, 4:59 p.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118614/
> ---
> 
> (Updated June 14, 2014, 4:59 p.m.)
> 
> 
> Review request for KDE Frameworks, Aleix Pol Gonzalez and Thiago Macieira.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Unittest for kiocore's thread-safety
> 
> 
> KIO thread safety: use one DBus connection per thread
> 
> 
> KIO thread safety: don't use static buffers.
> 
> They obviously lead to data races between threads.
> 
> KIO: create one instance of Scheduler per thread.
> 
> 
> Diffs
> -
> 
>   src/core/scheduler.cpp e762e369b8d74971831dc1e9f4d083bfd77b8e30 
>   src/core/connectionbackend.cpp 532141af5ab7676d1287379e56ff48bd7fa10c37 
>   autotests/threadtest.cpp PRE-CREATION 
>   autotests/CMakeLists.txt 81d261b8620e0a0512a5bc021ed189e910c0922a 
> 
> Diff: https://git.reviewboard.kde.org/r/118614/diff/
> 
> 
> Testing
> ---
> 
> helgrind'ing the unittest.
> 
> I found and fixed many races within Qt by doing that...
> Race on QMutex::id --> https://codereview.qt-project.org/86237
> Race in QThreadStorage --> https://codereview.qt-project.org/87047
> Race in QLoggingCategory --> https://codereview.qt-project.org/87054
> 
> Also reported QTBUG-39528 (QDBusConnection::sessionBus race)  (but we have 
> KDBusConnectionPool for that)
> 
> And there are still more races in Qt: 
> QMutex::isRecursive, qdbus_loadLibDBus, 
> QDBusConnectionPrivate::connectSignal, qt_message_print
> 
> And a few in our code:
> the getenv(KDE_FORK_SLAVES) in KIO, and the dbus interface for klauncher in 
> KDEInitInterface::ensureKdeinitRunning.
> 
> 
> Thanks,
> 
> David Faure
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118614: Make KIO thread-safe

[Thiago Macieira]

> On June 8, 2014, 1:20 p.m., Thiago Macieira wrote:
> > QDBusConnection is supposed to be threadsafe.
> 
> David Faure wrote:
> It's definitely not... 
> https://bugreports.qt-project.org/browse/QTBUG-39528
> 
> David Faure wrote:
> Thiago, can we agree on using KDBusConnectionPool for now, until the 
> races described in QTBUG-39528 are fixed?
> It's easy to switch later, once QDBusConnection::sessionBus isn't racy 
> anymore. I'd like multithreaded usage of KIO to be stable even with the 
> current Qt versions.

Yes, that is fine.


- Thiago


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118614/#review59565
---


On June 8, 2014, 10:11 a.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118614/
> ---
> 
> (Updated June 8, 2014, 10:11 a.m.)
> 
> 
> Review request for KDE Frameworks, Aleix Pol Gonzalez and Thiago Macieira.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Unittest for kiocore's thread-safety
> 
> 
> KIO thread safety: use one DBus connection per thread
> 
> 
> KIO thread safety: don't use static buffers.
> 
> They obviously lead to data races between threads.
> 
> KIO: create one instance of Scheduler per thread.
> 
> 
> Diffs
> -
> 
>   src/core/scheduler.cpp e762e369b8d74971831dc1e9f4d083bfd77b8e30 
>   src/core/connectionbackend.cpp 532141af5ab7676d1287379e56ff48bd7fa10c37 
>   autotests/threadtest.cpp PRE-CREATION 
>   autotests/CMakeLists.txt 81d261b8620e0a0512a5bc021ed189e910c0922a 
> 
> Diff: https://git.reviewboard.kde.org/r/118614/diff/
> 
> 
> Testing
> ---
> 
> helgrind'ing the unittest.
> 
> I found and fixed many races within Qt by doing that...
> Race on QMutex::id --> https://codereview.qt-project.org/86237
> Race in QThreadStorage --> https://codereview.qt-project.org/87047
> Race in QLoggingCategory --> https://codereview.qt-project.org/87054
> 
> Also reported QTBUG-39528 (QDBusConnection::sessionBus race)  (but we have 
> KDBusConnectionPool for that)
> 
> And there are still more races in Qt: 
> QMutex::isRecursive, qdbus_loadLibDBus, 
> QDBusConnectionPrivate::connectSignal, qt_message_print
> 
> And a few in our code:
> the getenv(KDE_FORK_SLAVES) in KIO, and the dbus interface for klauncher in 
> KDEInitInterface::ensureKdeinitRunning.
> 
> 
> Thanks,
> 
> David Faure
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 118614: Make KIO thread-safe

[Thiago Macieira]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118614/#review59565
---


QDBusConnection is supposed to be threadsafe.


src/core/scheduler.cpp
<https://git.reviewboard.kde.org/r/118614/#comment41516>

This isn't necessary


- Thiago Macieira


On June 8, 2014, 10:11 a.m., David Faure wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118614/
> ---
> 
> (Updated June 8, 2014, 10:11 a.m.)
> 
> 
> Review request for KDE Frameworks, Aleix Pol Gonzalez and Thiago Macieira.
> 
> 
> Repository: kio
> 
> 
> Description
> ---
> 
> Unittest for kiocore's thread-safety
> 
> 
> KIO thread safety: use one DBus connection per thread
> 
> 
> KIO thread safety: don't use static buffers.
> 
> They obviously lead to data races between threads.
> 
> KIO: create one instance of Scheduler per thread.
> 
> 
> Diffs
> -
> 
>   src/core/scheduler.cpp e762e369b8d74971831dc1e9f4d083bfd77b8e30 
>   src/core/connectionbackend.cpp 532141af5ab7676d1287379e56ff48bd7fa10c37 
>   autotests/threadtest.cpp PRE-CREATION 
>   autotests/CMakeLists.txt 81d261b8620e0a0512a5bc021ed189e910c0922a 
> 
> Diff: https://git.reviewboard.kde.org/r/118614/diff/
> 
> 
> Testing
> ---
> 
> helgrind'ing the unittest.
> 
> I found and fixed many races within Qt by doing that...
> Race on QMutex::id --> https://codereview.qt-project.org/86237
> Race in QThreadStorage --> https://codereview.qt-project.org/87047
> Race in QLoggingCategory --> https://codereview.qt-project.org/87054
> 
> Also reported QTBUG-39528 (QDBusConnection::sessionBus race)  (but we have 
> KDBusConnectionPool for that)
> 
> And there are still more races in Qt: 
> QMutex::isRecursive, qdbus_loadLibDBus, 
> QDBusConnectionPrivate::connectSignal, qt_message_print
> 
> And a few in our code:
> the getenv(KDE_FORK_SLAVES) in KIO, and the dbus interface for klauncher in 
> KDEInitInterface::ensureKdeinitRunning.
> 
> 
> Thanks,
> 
> David Faure
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Memory allocation overhead (Re: KFileItem)

[Thiago Macieira]

On domingo, 1 de setembro de 2013 10:46:45, David Faure wrote:
> On Friday 23 August 2013 12:57:15 Frank Reininghaus wrote:
> > http://code.woboq.org/userspace/glibc/malloc/malloc.c.html
> > 
> > A comment in that file [1] already says that the minimum allocated
> > size on a system with 8-byte pointers is 24/32 bytes (see below for an
> > explanation why it's not one fixed value).
> 
> Hi Frank,
> 
> Thanks for the detailed explanation!
> 
> I'm stunned. It means every class (in Qt and elsewhere) with a d pointer,
> uses 32 bytes for the d pointer, rather than the expected 8 bytes.

According to the comment, the a minimum allocation of 16 bytes includes the 
overhead. Since the pointer is also returned aligned to 8 bytes, it looks like 
the memory distribution is one of two possibilities:

|  ||   usable space   |
048  16

|  |   usable space|   |
04   12  16

For 64-bit platforms, the minimum allocation is 32 bytes, the alignment is 16 
bytes. So the above graph is still the same, but multiply the byte offsets by 
2.

The choice between the two depends on the alignment of the chunk of memory. If 
the alignment of the chunk is a multiple of 2*sizeof(void*), it's the first 
case. If it's not, it's the second case.

For value-type classes (no virtual table), usually sizeof(Klass) == 
sizeof(void*). For object classes, since they have a virtual table, 
sizeof(Klass) == 2*sizeof(void*). It's the case I drew above.

In any case, the allocation is always the same (4*sizeof(void*) = 16 or 32).

If QObject were sizeof(void*) bytes larger, the allocation would then be 
either 5*sizeof(void*) or 6*sizeof(void*), depending on the alignment of the 
free chunk. Those two are almost evenly distributed, so we could say the 
average allocation size is 5.5 * sizeof(void*).

For the value-type classes, I agree we should use a few more bytes.

> Maybe in some cases (small private class + class instanciated many times) we
> would save quite some memory by moving some stuff as private members, like
> 
> Private *d; // not used for now, always null
> int m_value;
> int m_min;
> int m_max;
> int m_step;

My branch already has 
sizeof(QString) == sizeof(QByteArray) == sizeof(QVector) == 3*sizeof(void*) 
and 
sizeof(QVariant) == 4 * sizeof(void*)

We'll implement that for Qt 6. It also enables "small string optimisation" for 
QString (5 characters) and QByteArray (11 characters).

For new value-type classes, we should start doing what you recommended. But we 
should design the class so that the d pointer gets used from the get go. 
Having it sit there and waste space is not going to help either.

> => one less allocation, 32 bytes saved per instance.
> 
> I wonder if all malloc implementations commonly used (i.e. including BSD,
> Mac and Windows) behave this way, though (but if they don't, the above
> doesn't lose anything anyway).
> 
> In the case of KFileItem, this obviously can't be done (huge private class).
> But making it a Q_MOVABLE_TYPE sounds right (since it is movable), we
> should just fix the code that relies on item pointers being stable across
> changes to the QList (e.g. by using QLinkedList).

If you care about memory overhead and efficiency, don't use QList.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
  PGP/GPG: 0x6EF45358; fingerprint:
  E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358


signature.asc
Description: This is a digitally signed message part.
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: naturalCompare Qt5 task

[Thiago Macieira]

On sexta-feira, 19 de julho de 2013 16.17.52, Frank Reininghaus wrote:
> But I thought that natural locale-based comparison is just what
> QCollator does if its "numericMode" is enabled?
> 
> http://qt.gitorious.org/qt/qtbase/blobs/v5.1.0/src/corelib/tools/qcollator.c
> pp#line401

Maybe. Check if it works for the usecases that you're interested in, like 
Dolphin sorting.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
  PGP/GPG: 0x6EF45358; fingerprint:
  E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358


signature.asc
Description: This is a digitally signed message part.
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: naturalCompare Qt5 task

[Thiago Macieira]

On sexta-feira, 19 de julho de 2013, às 13.17.26, you wrote:
> Hi,
> I was looking at that task in the Qt5 epics list and I didn't understand it
> fully.
>
> contribute natural-comparison to Qt5 (see KStringHandler). In Qt there is
> naturalCompare function but private and not as good as from KStringHandler.
> Thiago says: add the feature to QCollator.
>
> Here's the problems I see:
> - I don't know what's the goal of the task, so it's hard for me to decide
> what to do.

QCollator does locale-based sorting ("collating") in a much more efficient way
than element-by-element QString::localeAwareCompare can do.

The task is to add a set of methods to QCollator to do *natural* locale-based
comparison.

> - The one Qt5 has now, it's in QFileSystemModel

So it's easy to move the code around.

> - QCollator is also a private class, do we need to have it available from
> the public API? I guess the only use of it is in any tier1 module. If so,
> why QCollator?

QCollator is meant to be made public. It wasn't yet because of the pending
discussions on ICU.

> - In the code we have in KStringHandler we can find this comment:
> // This is based on the natural sort order code code by Martin Pool
> // http://sourcefrog.net/projects/natsort/
> // Martin Pool agreed to license this under LGPL or GPL.
>
> I don't feel very comfortable with moving this code over to the Qt project.

For good reason: you can't.

You need to take the one on QFSM and adapt it, or write everything anew.

--
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
  PGP/GPG: 0x6EF45358; fingerprint:
  E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358


signature.asc
Description: This is a digitally signed message part.
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 108889: Add QML_INSTALL_DIR variable to KDE pathes

[Thiago Macieira]

On segunda-feira, 11 de fevereiro de 2013 12.21.48, Sebastian Kügler wrote:
> On Monday, February 11, 2013 10:49:08 Sebastian Kügler wrote:
> > On Sunday, February 10, 2013 14:33:49 Thiago Macieira wrote:
> > > On domingo, 10 de fevereiro de 2013 21.22.05, Sebastian Kügler wrote:
> > > > Apparently not. I've looked at where the example imports from the Qt
> > > > codebase install these things, and that is indeed $PREFIX/qml, not
> > > > inside
> > > > the plugin directory. This location is also where these imports are
> > > > being
> > > > found automatically, i.e. without specifying additional import pathes
> > > > (which we have to do for Plasma in kdelibs 4).
> > >
> > > Technically, it's $ARCHDATADIR/qml. The Qt build makes
> > > ARCHDATADIR=PREFIX
> > > by  default.
> > >
> > >
> > >
> > > But everyone installing Qt to PREFIX=/usr should set it
> > > ARCHDATADIR=$LIBDIR/qt5.
> >
> > So that means that qtquick2 imports should go there, and that we should
> > advise  people in our build instructions to build with
> > --archdatadir=$LIBDIR/qt5?

If we advise --prefix=/usr, we should advise --archdatadir too, otherwise Qt's
build starts installing crap in /usr like /usr/plugins, /usr/mkspecs, /usr/qml
and /usr/imports. All four are controlled by the archdatadir option.

> > Which would bring me to my next question: How do I find out the ARCHDATA
> > dir  from a cmake script then? (Yes, noob question.)

I don't know. The info is hardcoded in qmake. Maybe cmake should keep a
similar list of paths?

A quick check shows that the cmake generated files are completely wrong already
for when someone plays with the dir options. They don't take --bindir into
account, for example.

--
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
  PGP/GPG: 0x6EF45358; fingerprint:
  E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358


signature.asc
Description: This is a digitally signed message part.
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: API review: KDBusApplicationStarter

[Thiago Macieira]

On terça-feira, 22 de maio de 2012 23.03.22, David Faure wrote:
> I'm about to write this class for libkdbus
> (or possibly for Qt, if thiago thinks this could go into libQtDBus?),
> to replace KToolInvocation::startService*.

Can't we use D-Bus service starting now? What is missing in the daemon?

--
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
  PGP/GPG: 0x6EF45358; fingerprint:
  E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358


signature.asc
Description: This is a digitally signed message part.
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: API review: KDBusApplicationStarter

[Thiago Macieira]

On quarta-feira, 23 de maio de 2012 10.18.26, David Faure wrote:
> On Wednesday 23 May 2012 01:29:52 Thiago Macieira wrote:
> > On terça-feira, 22 de maio de 2012 23.03.22, David Faure wrote:
> > > I'm about to write this class for libkdbus
> > > (or possibly for Qt, if thiago thinks this could go into libQtDBus?),
> > > to replace KToolInvocation::startService*.
> >
> > Can't we use D-Bus service starting now? What is missing in the daemon?
>
> The ability to locate /d/kde/inst/kde4/bin/kuiserver ?

D-Bus can find it if /d/kde/inst/kde4/share was on $XDG_DATA_DIRS at launch.

--
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel Open Source Technology Center
  PGP/GPG: 0x6EF45358; fingerprint:
  E067 918B B660 DBD1 105C  966C 33F5 F005 6EF4 5358


signature.asc
Description: This is a digitally signed message part.
___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel