D28745: Skip caching thumbnails on encrypted filesystems

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

D28745: Skip caching thumbnails on encrypted filesystems

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

D28745: Skip caching thumbnails on encrypted filesystems

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(); >

D28745: Skip caching thumbnails on encrypted filesystems

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,

D28745: Skip caching thumbnails on encrypted filesystems

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")); > +

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

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

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

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(), > +

D27802: smb: fix ipv6 support

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,

D27802: smb: fix ipv6 support

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,

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

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

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

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 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 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,

D21855: [IdUtils] Fix aliasing warning

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 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

D21317: Manipulate bytes instead of characters

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 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 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 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

D19107: Write valid UTF8 characters without escaping.

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

D17015: Fix the Qt doc creation with Qt 5.12.

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,

D14302: Don't block forever in ensureKdeinitRunning

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.

D14302: Don't block forever in ensureKdeinitRunning

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.

D14302: Don't block forever in ensureKdeinitRunning

thiago added a comment. In D14302#297159 , @jtamate wrote: > (gdb) disass > Dump of assembler code for function _ZN9QLockFile7tryLockEi: > 0x7f54be8bc752 <+2>: mov$0x,%eax > ... > 0x7f54be8bc76d

D14302: Don't block forever in ensureKdeinitRunning

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" > >

D14302: Don't block forever in ensureKdeinitRunning

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.

D14302: Don't block forever in ensureKdeinitRunning

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 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); > }

D14302: Don't block forever in ensureKdeinitRunning

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));

D14302: Don't block forever in ensureKdeinitRunning

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

D10273: Create proper SocketAddress

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

D10273: Create proper SocketAddress

thiago added a comment. Looks good. INLINE COMMENTS > fdsender.cpp:24 > > FdSender::FdSender(const std::string ) > : m_socketDes(-1) The problem here is the API. Why is it using std::string in the first place? REPOSITORY R241 KIO REVISION DETAIL

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

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

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

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

Re: kcrash, fork, and stdout/stderr

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

ild_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.or

Re: kcrash, fork, and stdout/stderr

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 -

Re: kcrash, fork, and stdout/stderr

66611213=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

Re: kcrash, fork, and stdout/stderr

y 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 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

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

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

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

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,

D5865: Add missing KDE_ENABLE_NAMED_OPERATORS function

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

D5865: Add missing KDE_ENABLE_NAMED_OPERATORS function

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.

D5865: Add missing KDE_ENABLE_NAMED_OPERATORS function

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

D5865: Add missing KDE_ENABLE_NAMED_OPERATORS function

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

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

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().

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

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

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

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:

Re: Review Request 128790: Call utempter manually

> 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, vis

Re: [kde-community] Usage of QNetworkAccessManager

> > 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 - th

Re: Deadlock in kded whith Qt 5.6

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 canno

Re: Deadlock in kded whith Qt 5.6

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

Re: Deadlock in kded whith Qt 5.6

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? > > > >

Re: Deadlock in kded whith Qt 5.6

e 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 6EF

Re: Deadlock in kded whith Qt 5.6

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) kd

Re: Deadlock in kded whith Qt 5.6

nd 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 fafa4b3b89a02e136f

Re: Deadlock in kded whith Qt 5.6

ve 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: 0x6EF4

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

(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

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

, 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

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

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

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

--- 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

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

://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

Re: KAuth and KF5

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

Re: Review Request 118614: Make KIO thread-safe

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

Re: Review Request 118614: Make KIO thread-safe

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

Re: Review Request 118614: Make KIO thread-safe

/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

Re: Memory allocation overhead (Re: KFileItem)

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

Re: naturalCompare Qt5 task

://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

Re: naturalCompare Qt5 task

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

Re: API review: KDBusApplicationStarter

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