Re: Should we stop distributing source tarballs?

[Thiago Macieira]

On Thursday 4 April 2024 04:26:57 PDT Sune Vuorela wrote:
> 'does it use autotools?'

The outcome of this is "please migrate off Autotools".

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Principal Engineer - Intel DCAI Cloud Engineering

Re: Switching to Qt 5.15.11 KDE patched

[Thiago Macieira]

On Sunday, 5 November 2023 17:26:37 -03 Rafael Sadowski wrote:
> I am currently stuck and maybe someone can explain this to me. Upstream
> Qt project ships which a "includes" directory in the tarball. The
> QtBase/KDE repository does not contain the include directory which is
> fine, but then I wonder about the includes:

You have to force the recreation of the includes dir. Just:

 touch .git

Before running configure.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DCAI Cloud Engineering

Re: Deprecation of QSharedMemory and QSystemSemaphore

[Thiago Macieira]

On Friday, 1 December 2023 14:01:14 CET Kai Uwe Broulik wrote:
> QSystemSemaphore:
> * Solid optical disc handling
> * Digikam server starter
> 
> QSharedMemory:
> * Marble
> * KMemFile used by KSycoca
> * Solid again
> * Snorenotify
> * Amarok
> * Kdenlive
> 
> At a glance it looks like some (e.g. Solid) can probably use QLockFile
> or are Linux-specific and can use memfd
> 
> Qt folks are aware of QtSingleApplication but we couldn’t really speak
> of the other use cases, in particular KSycoca.

KSycoca is supposed to be real sharing of memory.

For the other ones, the question is what they're using QSharedMemory for (how 
many processes, what's the ownership model).

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DCAI Cloud Engineering

Re: drkonqi's many debuggers

[Thiago Macieira]

On Thursday, 31 August 2023 03:42:59 PDT Halla Rempt wrote:
> On dinsdag 29 augustus 2023 05:22:56 CEST Thiago Macieira wrote:
> > True, but the majority of our user base is still Linux, so if we had to
> > choose only one, it would have to be gdb.
> 
> Um, no? The majority is on Windows.

Yes, we have certain hero apps (like Krita) that would skew the statistics. So 
maybe you're right.

But that doesn't change the conclusion because the vast majority of Windows 
users won't have a debugger installed. Those few that do won't have the 
debugger in the standard $PATH because it's just not the way things are done 
on Windows.

So rephrasing what I meant to say, "the vast majority of users who have any 
debugger installed are Linux users with gdb".

But I maintain that we should support both GDB and LLDB.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DCAI Cloud Engineering

Re: drkonqi's many debuggers

[Thiago Macieira]

On Monday, 28 August 2023 21:33:25 PDT Nate Graham wrote:
> On 8/28/23 22:25, Thiago Macieira wrote:
> > It does because it might be missing in the system far more often than gdb.
> > We'd get more backtraces and therefore more data if we focused on gdb
> > 
> > Another point is that Linux distributions have been shipping gdb with
> > debuginfod support for a year or two. lldb support for it is still
> > pending:
> > https://github.com/llvm/llvm-project/issues/52732
> > 
> > Therefore, I repeat: if there's room for only one, it's gdb.
> > 
> > If we do support both, then on Linux gdb must be used in preference.
> 
> DrKonqi itself could declare a required build dependency on lldb, and
> then we would know it's present on the system. Distros that don't
> package it would then have to omit DrKonqi, or start packaging it.

Build dependencies don't necessarily translate to runtime dependencies. What 
you're asking for is package management, something we don't control. Yes, we 
could convince all packagers to add it, but

a) that's not a sure-fire guarantee
b) do we even want to? Do we want to make it so every user who installs Plasma 
Workspace must have a debugger?

No, debuggers are optional.

Then there's my argument about libdebuginfo integration.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DCAI Cloud Engineering

Re: drkonqi's many debuggers

[Thiago Macieira]

On Monday, 28 August 2023 20:30:36 PDT Harald Sitter wrote:
> I am not quite following. We can depend on any debugger we want,
> surely? I mean, there are practical implications to consider for sure,
> but gdb being the dominant debugger on Linux doesn't prevent us from
> depending on lldb instead.

It does because it might be missing in the system far more often than gdb. 
We'd get more backtraces and therefore more data if we focused on gdb

Another point is that Linux distributions have been shipping gdb with 
debuginfod support for a year or two. lldb support for it is still pending:
https://github.com/llvm/llvm-project/issues/52732

Therefore, I repeat: if there's room for only one, it's gdb.

If we do support both, then on Linux gdb must be used in preference.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DCAI Cloud Engineering
.

Re: drkonqi's many debuggers

[Thiago Macieira]

On Monday, 28 August 2023 12:59:01 PDT Adriaan de Groot wrote:
> (puts on FreeBSD hat) On the non-GNU side of the world, lldb is the thing
> that is "installed anyway" and gdb takes extra effort. Though I don't know
> if the lldb integration works -- I have both installed, and I don't know if
> I ever bother to interact with DrKonqi (sorry).

True, but the majority of our user base is still Linux, so if we had to choose 
only one, it would have to be gdb.

That's probably why we need to have both gdb and lldb supported.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DCAI Cloud Engineering

Re: drkonqi's many debuggers

[Thiago Macieira]

On Monday, 28 August 2023 08:23:00 PDT Harald Sitter wrote:
> On a related note: does anyone have opinions on using lldb instead of
> gdb?

This contradicts your earlier point of:

> I am wondering if we couldn't just focus on one debugger
> and get less code with a better experience (both in writing and
> reading it).

If you want *one* debugger, then it has to be GDB on Linux, because it's the 
one that most people will have installed.

$ lldb --version
zsh: command not found: lldb

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DCAI Cloud Engineering

Re: Clarification on QT and kde patch set

[Thiago Macieira]

On Friday, 25 August 2023 08:21:05 PDT Scarlett Moore wrote:
> Hi!
> Sorry if this is documented somewhere. I couldn't find it. Am I to
> understand only qt5 is patched? Also is there a repo with the patches
> or just the patched qt in the kde/5.15 branch?

Correct, only 5.15 seems to be getting patches now. In theory this could be 
done for 6.2 but there's no such branch right now. See
https://invent.kde.org/qt/qt/qtbase/-/branches?
state=all=updated_desc=kde%2F

If you want the patches themselves, extract them from the Git repository. See
https://community.kde.org/
Qt5PatchCollection#How_do_I_get_this_integrated_in_my_distribution?

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DCAI Cloud Engineering

Re: RFC: Library unit testing and symbol visibility

[Thiago Macieira]

On Sunday, 23 July 2023 07:16:05 PDT Stefan Brüns wrote:
> 3. ?
> 
> Kind regards,

Qt's solution is that it builds in a "build for testing" mode (it defines the 
QT_BUILD_INTERNAL macro, for historical reasons meaning "internal to 
Trolltech") in which extra symbols are exported using the Q_AUTOTEST_EXPORT 
macro. This allows developers and CI systems to enable the extra API without 
needing to affect users.

The CI also tests non-"internal build" builds, so the tests must check whether 
they are in that mode before using a autotest-exported symbol.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DCAI Cloud Engineering

Re: Major QDBus regression in Qt6 with potential impact on KDE

[Thiago Macieira]

On Thursday, 15 December 2022 18:34:54 -03 Kai Uwe Broulik wrote:
> thanks for bringing that up. Indeed, at least Solid, KDE’s Hardware
> Abstraction Framework, makes use of InterfacesAdded/InterfacesRemoved
> extensively when talking to UDisks for monitoring storage devices.
> 
> That said, I haven’t tried Solid on Qt 6 yet, so I don’t know if it is a
> real-world issue for this scenario.

The issue appears to be a race condition, because strace shows everything 
working as it should and the use of QDBUS_DEBUG=1 will affect timing. That also 
means the issue is likely a Heisenbug and difficult to debug.

It might also be a latent issue in Qt 5 that got exposed by faster 
functionality somewhere in QtCore, because there have been no changes to 
QtDBus to account for this (it has barely received any changes since 5.6).

Someone needs to debug it and I don't have the time.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DCAI Cloud Engineering

Re: Fwd: KDiff3 v 1.9.6 Windows crash

[Thiago Macieira]

On Wednesday, 16 November 2022 13:51:47 PST Michael Reeves wrote:
> This is the question  I need answered. Windows by default gives the most
> useless crash report I've ever seen. All I know from that is that the crash
> point is inside qt but that doesn't help at all with a Qt  based
> application. Worse a lot the documentation I find assumes you are a)
> debugging a drive and/or b) working with msvc project that you can
> reproduce the crash in.

The same as anywhere else: run the application inside the debugger (gdb if it 
is MinGW). When it crashes, type "bt".

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DCAI Cloud Engineering

Re: kdesrc -- stuck building kwayland

[Thiago Macieira]

On Monday, 12 September 2022 08:55:10 PDT Nicola Mingotti wrote:
> Therefore it seems the wisest option to me to wait till you guys update the
> CMakeLists.txt.

That's simply going to make the build fail earlier, with a different but more 
meaningful error message.

You need Wayland 1.20 if you want to build this version of kwayland.

> Thank you for kdesrc, there are a few quirks but in general it is great we
> can try new stuff in a stable system !

That's not always possible. This is such a case. You'll need an upgrade.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DCAI Cloud Engineering

Re: Challenge: adding new method overloads when existing consumers use {} with args

[Thiago Macieira]

On Thursday, 28 July 2022 05:38:38 PDT Friedrich W. H. Kossebau wrote:
> Thanks for the link, interesting (Sidenote: Not sure I agree that there are
> SiCs which are acceptable, I know I got upset a few times by such breakages,
> breaking is breaking after all.)

They're all some level of bad. The question is what we could accept in some 
circumstances that really require it, and those that we can't at all.

> Not sure I understand things correctly: Isn't the addition in our example
> rather category A, because it "can be worked around in user code without
> introducing [Qt] version checks"? In our example, by using A{}/A() or
> B{}/B() when invoking the method?

Strict reading of the QUIP would say it is Cat B because it adds overloads. 
But they can be easily worked around in code without issues, so long as 
obtaining the pointer to this function wasn't a requirement before, so more 
like Cat A.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering

Re: Challenge: adding new method overloads when existing consumers use {} with args

[Thiago Macieira]

[cross-posting to Qt dev ML - dunno if it'll arrive because I'm subscribed 
with different addresses]

On Wednesday, 27 July 2022 14:54:55 PDT Friedrich W. H. Kossebau wrote:
> And has no-one else yet run into this problem? E.g. Qt, anyone seen them
> adding new overloads, what did they do there, if?

This case can be considered a Category B source incompatible change as per 
https://quips-qt-io.herokuapp.com/quip-0006.html, because it clearly 
introduces ambiguity.

But {} is particularly special, so I don't know how we'd deal with it. I don't 
think this has come up for us yet. For one, the mailing list thread linked in 
the QUIP didn't address it.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering

Re: Challenge: adding new method overloads when existing consumers use {} with args

[Thiago Macieira]

On Wednesday, 27 July 2022 14:37:20 PDT Friedrich W. H. Kossebau wrote:
> * new overload method (and any further ones) will stay special
> * existing consumer code using {} is not that simple to map by human readers
> as to which overload will be used
> * needs C++17, so needs some additional markup when used in KF5 headers for
> those using them with C++11-compat needs

You can use std::enable_if (no _t).

Another trick is what we do in Qt with Q_WEAK_OVERLOAD: 

template 

It doesn't matter what the parameters are and that this new template function 
doesn't actually use the template parameter. As a template function, it will 
not get selected for conversion; the types must match exactly.

This is handy even without {}, when you want to down-prioritise some overloads 
over others for some reason, to avoid ambiguity. For example, you can't 
overload a QByteArray with QLatin1String (or Qt6's QByteArrayView or 
QAnyStringView or any such other combination) because it makes calling with a 
character literal ambiguous. But you can if one of them is a weak overload.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering

Re: NetworkManagerQt

[Thiago Macieira]

On Friday, 27 May 2022 21:44:10 PDT Matthias Klumpp wrote:
> In addition to the additions, I also would like to point out that
> NetworkManager itself is GPLv2-licensed (note the missing "L"!), so
> you will already have to comply with the GPL in your product.

Considering it's a Linux device, that was already the case.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering

Re: QtWebengine version

[Thiago Macieira]

On Friday, 20 May 2022 15:27:00 PDT Albert Astals Cid wrote:
> One solution that may make sense is:
>  * Us creating a kde/5.15 branch on 5.15 and make it so the generated cmake
> file doesn't exactly require 5.15.x but just generally require 5.15

That sounds like what upstream should be have been doing all along.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering

Re: The KDE Patchset Collection has been rebased on top of Qt 5.15.3

[Thiago Macieira]

On Sunday, 6 March 2022 15:26:21 PST Neal Gompa wrote:
>  In any
> case, I just want a git tag that Plasma developers indicate that we
> should *at least* have for a well-functioning KDE Plasma build.

You don't need a tag, you need a branch. The state that you need to have a 
well-functioning KDE Plasma build is all the patches that exist at the moment 
you ask that question.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering

Re: libksane seems to break QProcess::start calls

[Thiago Macieira]

On Saturday, 5 March 2022 00:31:43 PST Tobias Leupold wrote:
> f it's only the SANE backend that breaks this, you're of course right.

Unfortunately, we can't be sure of that. It's hard to get Unix signals right 
(we're discussing another instance of it in Qt right now, on [1])

> Within what I could grasp, I filed an issue:
> https://gitlab.com/sane-project/backends/-/issues/582
> 
> I fear fixing this is far beyond my programming skills. I hardly understand
> what's going on here at all ...

I'll add some comments.

[1] https://codereview.qt-project.org/c/qt/qtbase/+/380826

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering

Re: libksane seems to break QProcess::start calls

[Thiago Macieira]

On Friday, 4 March 2022 13:47:07 PST Tobias Leupold wrote:
> I don't know if this would be still possible for the Qt 5 docs, but a small
> hint like "If you subclass this, be aware that ..." woudn't hurt ... As
> said, I would never have thought that this would have such side-effects!

It shouldn't have any side-effects. In fact, the code exists to *avoid* side-
effects in the first place.

Of course, it assumes that there isn't buggy code elsewhere. If there is, then 
all bets are lost. I'm not going to document how to bugfix other code, 
especially complex code like this. And we've already established that the SANE 
backend is buggy.

> > Anyway, please note that simply your problem still exists even without
> > subclassing on kernels older than 5.4. For those, we have to use the
> > SIGCHLD handler anyway. This must be fixed in the SANE backend.
> 
> If you're really bored some time, maybe you want to file an issue for the
> SANE Pixma backend and tell the devs what exactly is wrong there and/or how
> to fix this?

I'm not. I'll explain if you file or if you want to actually fix, but I don't 
have that much free time available.

> > Or you can work around it by doing what it is doing: fork().
> 
> Well, I think, if I'm safe with Qt >=5.15 and Kernel >=5.4, for now, I'd
> stick with the simple implementation that works now. Forking some part of a
> Qt program is something I even know less about than what can happen if I
> call external programs via QProcess ... I'm quite happy to have it in a
> working state now!

Well, if you use QProcess to call the full SANE backend, that will also avoid 
the problem.

> Surely, the fact that some SANE backends may cause issues with process calls
> with older kernels should be mentioned in the still-to-write docs. But at
> the moment, I think I should first see if anybody besides me ever considers
> the project to be meaningful and if it would be worth it to make it safe
> for older environments. Although it may be worth it to mess with this stuff
> to learn more about process handling, threads and forks and so on.

"If you write buggy code, the application may misbehave" is not documentation.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering

Re: libksane seems to break QProcess::start calls

[Thiago Macieira]

On Friday, 4 March 2022 10:52:00 PST Tobias Leupold wrote:
> I simply made this a QObject subclass and rather then using start() etc.
> directly, I created a QProcess object and did the same stuff with this one.
> 
> No freezes anymore. No matter what scanner I use.
> 
> I never thought subclassing the QProcess would change the behavior in such a
> radical way :-O Is there some passage in the docs that says "Never ever
> subclass a QProcess unless you exactly know what you are doing, otherwise,
> you will experience the weirdest problems"? :-D

It's a side-effect. The problem is the QProcess::setupChildProcess virtual in 
Qt 5, that had been there since QProcess was introduced the the Paleolithic 
Era. When we use CLONE_PIDFD, we have to call clone() directly, which means 
the hooks installed by pthread_atfork() do not get run. In particular, this 
may mean the mutexes locked by other threads are all still locked, including 
one inside malloc(). That would mean the user's code in setupChildProcess() 
could deadlock depending on the kernel version.

To avoid this possibility, we needed to know if you'd overridden 
setupChildProcess(). When I introduced the CLONE_PIDFD content, we originally 
added a very hacky way of detecting that, but that was way too fragile. So it 
was simplified to "if you derive, assume setupChildProcess was overridden." Qt 
6 removed this virtual and replaced it with a callback using std::function 
(QProcess::setChildProcessModifier). So we can be much more sure that you have 
code that you want to run on the child side before execve().

Anyway, please note that simply your problem still exists even without 
subclassing on kernels older than 5.4. For those, we have to use the SIGCHLD 
handler anyway. This must be fixed in the SANE backend.

Or you can work around it by doing what it is doing: fork(). Run the SANE 
backend entirely in a child process, so it can't affect the main application's 
SIGCHLD handler. If you need to share memory, you can do that by memory-
mapping a file before fork(), as that gets shared between parent and child. A 
memfd would be ideal, but if that fails, you can use a regular QTemporaryFile. 
On Linux, QSharedMemory is just a memory-mapped file on a tmpfs (/dev/shm) 
after all, only it's hiding behind two or three layers of abstraction.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering

Re: libksane seems to break QProcess::start calls

[Thiago Macieira]

On Friday, 4 March 2022 09:02:37 PST Thiago Macieira wrote:
> > I uploaded the traces here:
> > https://l3u.de/tmp/strace_brscan.txt.xz
> > https://l3u.de/tmp/strace_plustek.txt.xz
> > 
> > Thanks again for all help!
> 
> I'll take a look. Let's see... the brscan trace doesn't have any use of
> PIDFD.

Let's see the plustek scan. It also has zero uses of PIDFD (probably libksane 
is using KProcess, which triggers the QProcess pidfd protection in Qt 5). So 
PIDFD is not the issue.

Let's see what it's doing. The first mention of SIGCHLD is here:

[pid 20601] clone(child_stack=NULL, flags=CLONE_CHILD_CLEARTID|
CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7fb873fff910) = 20602
[pid 20601] rt_sigaction(SIGCHLD, {sa_handler=0x7fb894726350, sa_mask=[CHLD], 
sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7fb8cd33a5a0}, 
{sa_handler=SIG_DFL, sa_mask=[], sa_flags=0}, 8) = 0

That's NOT QProcess. I can tell because the sequence is wrong, possibly buggy: 
it's a bad idea to install the SIGCHLD handler *after* you've fork()ed your 
child, because the child might die before you get there (unless you construct 
the child code so that it can't). QProcess also doesn't set the sa_mask, which 
this code did.

And there's no execve() in PID 20602, so it definitely isn't QProcess. This is 
just something using fork() to run some code that may or may not crash, may or 
may not hang so it may need to be kill()ed.

The second mention of SIGCHLD is this child dying:

[pid 20601] --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=20602, 
si_uid=1000, si_status=0, si_utime=3, si_stime=31} ---
[pid 20601] rt_sigreturn({mask=[ALRM]}) = 0

I don't know what the signal handler did, because it made no system calls 
before returning. That indicates a poorly written signal handler (it probably 
wrote to a volatile variable). That's also probably buggy.

The third mention of SIGCHLD is QProcess/forkfd:

[pid 20579] rt_sigaction(SIGCHLD, {sa_handler=0x7fb8cda513e0, sa_mask=[], 
sa_flags=SA_RESTORER|SA_SIGINFO|SA_NOCLDSTOP, sa_restorer=0x7fb8cd33a5a0}, 
{sa_handler=0x7fb894726350, sa_mask=[CHLD], sa_flags=SA_RESTORER|SA_RESTART, 
sa_restorer=0x7fb8cd33a5a0}, 8) = 0
[pid 20579] rt_sigaction(SIGPIPE, {sa_handler=SIG_IGN, sa_mask=[], 
sa_flags=SA_RESTORER, sa_restorer=0x7fb8cd33a5a0}, NULL, 8) = 0
[pid 20579] futex(0x7fb8cdd7f3e8, FUTEX_WAKE_PRIVATE, 2147483647) = 0
[pid 20579] pipe2([24, 25], O_CLOEXEC)  = 0
[pid 20579] eventfd2(0, EFD_CLOEXEC)= 26
[pid 20579] clone(child_stack=NULL, flags=CLONE_CHILD_CLEARTID|
CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7fb8ca53ba90) = 20605
[pid 20579] write(26, "*\0\0\0\0\0\0\0", 8) = 8

Note the same sequence as the previous commit, with a pipe and an eventfd, 
which gets value 42. Note also how the handler for SIGCHLD is installed before 
we fork(), with no mask.

This child's death is properly reported in the fourth mention of SIGCHLD:

[pid 20579] --- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_EXITED, si_pid=20605, 
si_uid=1000, si_status=0, si_utime=25, si_stime=0} ---
[pid 20579] waitid(P_ALL, 0, {si_signo=SIGCHLD, si_code=CLD_EXITED, 
si_pid=20605, si_uid=1000, si_status=0, si_utime=0, si_stime=0}, WNOHANG|
WEXITED|WNOWAIT, NULL) = 0
[pid 20579] wait4(20605, [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], WNOHANG, 
{ru_utime={tv_sec=0, tv_usec=248891}, ru_stime={tv_sec=0, tv_usec=3966}, ...}) 
= 20605
[pid 20579] write(25, "\1\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0;
\314\3\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 152) = 152
[pid 20579] close(25)   = 0
[pid 20579] waitid(P_ALL, 0, 0x7ffc6caac790, WNOHANG|WEXITED|WNOWAIT, NULL) = 
-1 ECHILD (Keine Kind-Prozesse)
[pid 20579] rt_sigreturn({mask=[]}) = 2

And here you see a proper SIGCHLD handler, that actually does some work before 
returning.

The fifth mention of SIGCHLD is the same as the first:

[pid 20636] clone(child_stack=NULL, flags=CLONE_CHILD_CLEARTID|
CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7fb8720ab910) = 20637
[pid 20636] rt_sigaction(SIGCHLD, {sa_handler=0x7fb894726350, sa_mask=[CHLD], 
sa_flags=SA_RESTORER|SA_RESTART, sa_restorer=0x7fb8cd33a5a0}, 
{sa_handler=0x7fb8cda513e0, sa_mask=[], sa_flags=SA_RESTORER|SA_SIGINFO|
SA_NOCLDSTOP, sa_restorer=0x7fb8cd33a5a0}, 8) = 0

This installs *again* the same SIGCHLD handler at pointer address 
0x7fb894726350 after fork(). This is the first certain bug in this code, third 
possible bug overall. Since it never uninstalled its handler, it shouldn't 
attempt to install it again. There's NO condition under doing that is a 
correct thing to do in a multithreaded application. Zero.

The sixth mention is the same as the second.

The seventh mention is forkfd again, but like a proper SIGCHLD citizen, it's 
different from the fourth mention by not installing the handler again. It just 
starts the child via fork():

[pid 20579] pipe2([24, 25], O_CLOEXEC)  = 0
[pid 20579] eventfd2(0, EFD_CLOEXEC)= 26
[pid 20579] clone(child_stack=NULL, f

Re: libksane seems to break QProcess::start calls

[Thiago Macieira]

On Thursday, 3 March 2022 23:32:11 PST Tobias Leupold wrote:
> I did it like this. I created one strace for the scanner not causing
> problems and scanned two pages. This resulted in 27,096 lines of output.
> 
> Then I created one for the scanner causing the issue, also scanning two
> pages. This yielded a solid 270,807 lines :-O
> 
> To be honest, I don't have a clue for what I search in those files ...
> would you be so kind to have a look? Or can I lessen the output, so this
> becomes more readable?
> 
> I uploaded the traces here:
> https://l3u.de/tmp/strace_brscan.txt.xz
> https://l3u.de/tmp/strace_plustek.txt.xz
> 
> Thanks again for all help!

I'll take a look. Let's see... the brscan trace doesn't have any use of PIDFD. 
Having *zero* uses means usually means it isn't QProcess because QProcess 
(forkfd) always tries it at least once, to determine if the support is there. 
But it certainly looks like QProcess:

[pid 20386] pipe2([12, 17], O_CLOEXEC)  = 0
[pid 20386] write(5, "\1\0\0\0\0\0\0\0", 8) = 8
[pid 20386] write(5, "\1\0\0\0\0\0\0\0", 8) = 8
[pid 20386] pipe2([18, 19], O_CLOEXEC)  = 0
[pid 20386] write(5, "\1\0\0\0\0\0\0\0", 8) = 8
[pid 20386] pipe2([20, 21], O_CLOEXEC)  = 0
[pid 20386] write(5, "\1\0\0\0\0\0\0\0", 8) = 8
[pid 20386] pipe2([22, 23], O_CLOEXEC)  = 0
[pid 20386] write(5, "\1\0\0\0\0\0\0\0", 8) = 8

That's four pipes with a write of 64-bit value 1 interspersed. That's really 
QProcess and the writes are the QSocketNotifier creations (the write() are 
telling the event loop that something has changed, so it can't go to sleep).

And here's forkfd:

[pid 20386] pipe2([24, 25], O_CLOEXEC)  = 0
[pid 20386] eventfd2(0, EFD_CLOEXEC)= 26
[pid 20386] clone(child_stack=NULL, flags=CLONE_CHILD_CLEARTID|
CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7f5cce3eea90) = 20425
[pid 20386] write(26, "*\0\0\0\0\0\0\0", 8 

This is very distinctive: one more pipe, one eventfd and writing of value 42 
into it (the answer to life, the universe and everything). Making the code 
sequence unique pays off (I can fingerprint it in straces).

So the question is why this brscan didn't even attempt to use pidfd. There's 
some code in Qt5's QProcess to attempt to detect whether you've subclassed 
QProcess and skip using the pidfd feature:

if (typeid(*q) != typeid(QProcess))
ffdflags |= FFD_USE_FORK;

https://code.qt.io/cgit/qt/qtbase.git/tree/src/corelib/io/qprocess_unix.cpp?
h=5.15#n462

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering

Re: libksane seems to break QProcess::start calls

[Thiago Macieira]

On Thursday, 3 March 2022 11:00:07 PST Tobias Leupold wrote:
> Am Donnerstag, 3. März 2022, 19:06:21 CET schrieb Stefan Brüns:
> > Are you using either Qt5 < 5.15 or a kernel version which does not support
> > CLONE_FD? - then you are relying on SIGCHLD for process exit notification.
> > 
> > CLONE_FD: https://lwn.net/Articles/636646/
> > Qt5: https://codereview.qt-project.org/c/qt/qtbase/+/108456/
> > 
> > sane-backends/backend/plustek-usbhw.c messes with the signal handlers and
> > fails to restore it: `sigaction(..., ..., NULL)`

If CLONE_PIDFD is active, we don't use the SIGCHLD handler to be notified of 
process exit, but SIGCHLD must not be ignored with SIG_IGN. That causes the 
kernel not to notify anything.

https://gitlab.com/sane-project/backends/-/blob/master/backend/plustek-usbhw.c

I see sigaction, but it is only messing with SIGALRM. Yes, it should restore 
previous the handler (probably SIG_DFL) after the alarm is done, but this 
isn't the issue. It also messes with the signal block mask, but I don't see a 
problem there either: it blocks and unblocks SIGALRM.

> I use Qt 5.15.2 and Gentoo's kernel 5.15.16. Is there something I can check
> to figure out if this could cause the problem? Apparently, there's no
> kernel option I can set?

You can strace the process. Add the options -f -bexecve to see the current 
process and its forks & threads, but stop when it executes a child process. 
You should see SIGCHLD line, like this:

kill(669903, SIGTERM)   = 0
poll([{fd=3, events=0}, {fd=5, events=0}, {fd=6, events=POLLIN}, {fd=8, 
events=POLLIN}, {fd=12, events=POLLIN}], 5, 5000) = 4 ([{fd=5, 
revents=POLLERR}, {fd=6, revents=POLLHUP}, {fd=8, revents=POLLHUP}, {fd=12, 
revents=POLLIN}])
--- SIGCHLD {si_signo=SIGCHLD, si_code=CLD_KILLED, si_pid=669903, si_uid=1000, 
si_status=SIGTERM, si_utime=0, si_stime=0} ---
fcntl(12, F_GETFL)  = 0x802 (flags O_RDWR|O_NONBLOCK)
waitid(P_PIDFD, 12, {si_signo=SIGCHLD, si_code=CLD_KILLED, si_pid=669903, 
si_uid=1000, si_status=SIGTERM, si_utime=0, si_stime=0}, WNOHANG|WEXITED, 
NULL) = 0

fd 12 is the pidfd in this case.

Anyway, search for SIGCHLD in the trace and see if anything is setting a 
handler for it. On kernels >= 5.4, Qt doesn't do it, so if you see any mention 
of it aside from deliveries like above, it came from elsewhere.

> Apart from that, the scanner in question actually does use the plustek
> backend! I fear there's no way to fix this in my code?!

Fix the problem where the problem is.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering

Re: Please check your QProcess/KProcess invokations

[Thiago Macieira]

On Monday, 31 January 2022 08:46:19 PST Albert Astals Cid wrote:
> QProcess has this ?undocumented? feature that when you do
> QProcess::start("bloblo") it will start bloblo from the current working
> directory (CWD) if it's there and it's not in PATH (at least on Linux)

That's a bug.

> But we have those programming mistakes in lots of places (probably not as
> easily exploitable), so I would like to ask everyone to check as many apps
> as they can when they think that they are using Q/KProcess to make sure we
> call QStandardPaths::findExecutable before QProcess.

Make sure you're not making the same mistake we are:

qprocess_unix.cpp's resolveExecutable:

if (!program.contains(QLatin1Char('/'))) {
QString exeFilePath = QStandardPaths::findExecutable(program);
if (!exeFilePath.isEmpty())
return exeFilePath;
}
return program;

The issue is handling findExecutable() not finding anything. Instead of 
refusing to run, we return program; which is the name that you had passed, and 
that goes straight into execve().

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering

Re: global shortcuts

[Thiago Macieira]

On Saturday, 22 January 2022 04:33:24 PST Martin Koller wrote:
> Hi,
> 
> which component is responsible for acting on global shortcuts, e.g.
> what triggers that Alt-F2 is opening krunner ?

kglobalacceld

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering

Re: Trying to build knotifications framework, kdesrc-build and qt5

[Thiago Macieira]

On Tuesday, 3 August 2021 05:29:22 PDT Colin Williams wrote:
> I don't have a clue regarding 'reduce_relocations'. Is there a way to
> disable it and would it make sense to do so?

Check the config.log

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering

Re: RFC: Konsole profile groups

[Thiago Macieira]

On Saturday, 31 July 2021 06:33:24 PDT David Hurka wrote:
> > > > Is anyone using Konsole profile groups? [...]
> > 
> > No, "profile groups" are a different concept; from reading the code in
> > ProfileGroup.cpp it's like a group of profiles having one parent profile,
> > and when you change the settings it can be applied to all the child
> > profiles, or to specific ones...
> 
> I use profiles to distinguish different windows by background color. I have
> some application menu entries that launch Konsole with a specific command to
> execute, and these windows will use a profile with a different background
> color, so I will know later what this window is good for.
> 
> I am not aware of any way to use profile groups, or how to create them.
> There is no such button in “Manage Profiles...”. So I guess, if you remove
> this feature, users will not notice it. :D

Same here. I have a couple of profiles that differ by minimal amounts, but in 
all other aspects they are the same.

I would have used this feature if I had known it existed or had found it. That 
would allow me to change the font size in all profiles in the same place. But 
it looks like it's too well hidden.


-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering

Re: Does Kwin script's callDBus support a{sv} argument type?

[Thiago Macieira]

On Wednesday, 10 March 2021 03:43:44 PST Phương Lê Hoàng wrote:
> I tried to read the source code of callDBus and failed to understand
> whether it supports a{sv} or not, since I'm not familiar with Qt and C++.
> https://invent.kde.org/plasma/kwin/-/blob/master/src/scripting/scripting.cpp
> #L253

This should have worked. Searching the source code shows that 

for (const QJSValue  : qAsConst(jsArguments)) {
 dbusArguments << jsArgument.toVariant();
 } 

Should end up creating a QVariantMap and QtDBus should have understood them to 
create an a{sv}.

Something is broken. You'll need to debug.
-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering

Re: Safely storing an application's API keys

[Thiago Macieira]

On Monday, 18 January 2021 03:21:30 PST Jean-Baptiste Mardelle wrote:
> Hi all,
> 
> For Kdenlive, we are planning to expand the use of online services to
> download ambiance music or videos for use in personal projects. To this
> purpose, most online services provide us an API key that is used to
> identify our app (Kdenlive) when querying their API.
> 
> Does anyone have experience / advice on how to protect these API keys so
> that they are not publicly available ? Is there any KDE online service or
> framework helping to achieve that ?

If you MUST do that, then the only secure place to store an API key is the 
TPM. That thing is protected by system security itself and gets disabled if 
the system doesn't pass its own security checks.

Which of course most Linux systems don't. Anyone who builds a custom kernel or 
disables secure booting will fail this.

You have the following options, which are complementary:
1) put an API key in the source code, which means public and visible to 
everyone

2) allow is not present) the user to create a new key and store it in the 
config file

#1 allows your software to run for everyone, regardless of how they build it. 
If it is missing, #2 becomes mandatory and your users will need to obtain a 
key of their own prior to the functionality becoming useful.

A compromise with the API providers is that the key from #1 can be a "low 
rate" one. That is, one that is limited in bandwidth or how many requests per 
second it is allowed to make. This happens for example to the rclone API key 
to use Microsoft One Drive.

Please reach out to the services in question and advise them that your 
software is open source and therefore cannot hide the key at all, and present 
these two possibilities.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering

Re: A question relating Arora

[Thiago Macieira]

On Friday, 25 December 2020 19:21:43 -03 Aaron Dewes wrote:
> The original README contained this: Arora is a fork of the Demo Browser
> that was shipped with Qt 4.4.0. All new development is no longer going
> into the demo browser, but here. Some have critizized the demo browser
> for being to large already and should have been kicked out earlier. I'm
> interested about the "All new development is no longer going into the
> demo browser, but here." part. Does this mean this browser was supported
> by Qt or the KDE foundation and officially moved or does this just mean
> the original creator just moved all of his new development from the demo
> browser to Arora? was especially wondering about the involvement of KDE
> because the author of arora also was the author of kaudiocreator and had
> more contributions to KDE, so he was involved KDE.

Hello Aaron

Thank you for the interest!

The above is essentially is correct. The author of the browser is the author 
of the demo browser. At the time, he was working for Trolltech in Oslo (at the 
same time as I was there) and he was just having too much fun with the demo 
browser. That's when we decided that he shouldn't add more things to the demo 
browser, since it would make the thing grow in size and complexity, taking it 
definitely beyond what a demo should be. We made the decision that he could 
continue exploring the full application in a separate repository, outside of 
the Qt source code.

Trolltech did use to hire talented developers from the KDE community, which is 
how quite a few of us ended up there, like Matthias Ettrich (KDE's founder), 
Lars Knoll (khtml's original author), Simon Hausmann, Roberto Raggi and Harald 
Fernengel (from KDevelop), Thomas Zander (KOffice), Zack Rusin, etc.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering

Re: How do you deal with incomplete commits?

[Thiago Macieira]

On Saturday, 31 October 2020 08:25:40 PST Thomas Friedrichsmeier wrote:
> thanks for your answer (also to Nate). But to clarify, my question is
> really: How do I _force_ myself to clean up in time?

If you're pushing to a code review system of any kind, it doesn't matter. 
First, you should always review what you've sent for review anyway and you can 
notice you pushed something incomplete. At that point don't create the review 
request or write that it isn't yet ready for review.

Second, your reviewers should notice it's incomplete and won't approve.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering

Re: KPluginLoader UBSAN warnings (object has invalid vptr)

[Thiago Macieira]

On Thursday, 15 October 2020 07:22:59 PDT Milian Wolff wrote:
> I have the feeling that this might be a limitation of UBSAN? Or is this an
> actual problem - does anyone know?

I've seen similar warnings that were impossible. See
https://bugreports.qt.io/browse/QTBUG-85398

I'm not sure yet if this is an UBSan bug or if there was some weird, duplicate 
symbol problem.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel DPG Cloud Engineering

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

Re: Is kdeinit still actual?

[Thiago Macieira]

On Monday, 27 April 2020 03:53:38 PDT Alexander Volkov wrote:
> As for KIO slaves, they are started from processes that are already
> linked with many KDE libraries and there is no
> 
> much benefit in starting them from kdeinit.

That's an incorrect conclusion. Your premise is correct: the process that 
launches them links to many KDE libraries. But since that launch is a regular 
fork() + exec(), the premise is irrelevant. The KDE libraries will be loaded 
again, relocated and initialised, before the ioslave code is run.

A quick test here shows kdeinit and one ioslave (file.so, that was already 
running) are sharing 2380 kB between them. The ioslave has a total of 548 kB 
of additional private memory that isn't shared with anything else in the 
system.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel System Software Products

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

Re: ELF Dissector in kdereview

[Thiago Macieira]

On Tuesday, 1 October 2019 05:06:57 PDT Jonathan Riddell wrote:
> -isystem
> /usr/include/capstone/..
[...]
> /usr/include/c++/7/cstdlib:75:15: fatal error: stdlib.h: No such file or
> directory

That -isystem is the mistake.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel System Software Products

Re: Cmake and dbus daemon on the system bus

[Thiago Macieira]

On Monday, 16 September 2019 12:56:32 PDT David Edmundson wrote:
> You want to look for dbus-activation and set User=root in the
> activation desktop file that you put in
> /usr/share/dbus-1/system-services

Make sure you REALLY need to run as root. If you can run as some other, non-
privileged user, that's better. That can be as a result of dropping privileges 
with setuid() or capset() after acquiring the resource you need.

If you do that, connect to the bus AFTER you've dropped privileges.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel System Software Products

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

Re: Invoking "kcheckpass" from the terminal

[Thiago Macieira]

On Thursday, 8 August 2019 12:00:34 PDT Franklin, Jason wrote:
> However, after trying several invocations, I can't get the tool to behave as
> expected (i.e., take a password on stdin and exit with 0/1 on success/
> failure).

That's because the tool does not take the password on stdin.

$ /usr/lib64/libexec/kcheckpass   
Only binary protocol supported

You need to pass a file descriptor number with the -S option.

kcheckpass.c also makes debugging difficult, by setting a bunch of options to 
prevent unauthorised attaching to the process. You need to modify the source 
to turn those off.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel System Software Products

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

Re: How is the Font DPI Scale stored?

[Thiago Macieira]

On Wednesday, 12 June 2019 08:53:50 PDT Elias Mårtenson wrote:
> Thank you. That works beautifully. I do wonder, however, if it's
> deprecated, what is meant to replace it?

Properly? Wayland.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   Software Architect - Intel System Software Products

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

Re: KWin testing

[Thiago Macieira]

On Thursday, 31 January 2019 21:41:05 PST atul bisht wrote:
> Hi
> 
> How can I test dbus calls which are not released.
> I was working on KCM which requires dbus properties .
> Properties are also not present in qdbusviewer.

Call the Get and Set methods in the org.freedesktop.DBus.Properties interface, 
on that object.

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

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

Re: Time formats / LC_TIME challenge for 4-digit year support

[Thiago Macieira]

On Tuesday, 4 December 2018 11:42:41 PST Jaroslaw Staniek wrote:
> I did not know there are such differences for the locale system. But by no
> means my email is a call to "patch" the Qt-level problem by having a KF5
> solution. And I am skeptical if Qt can be patched in version 5 with such
> incompatible change. Hopefully these poor man analysis help some other app
> developers 

If you want that, please submit your patch to unicode.org. They need to update 
the CLDR to have 4-digit years for the locale of your choice.

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

Re: Transitioning CI builds of all non-Frameworks from Qt 5.9

[Thiago Macieira]

On Monday, 3 December 2018 01:30:25 PST René J.V. Bertin wrote:
> Can't you just configure the CI to use Qt 5.10? I think it's not good to
> hardcode an "overzealous" (for lack of a better word) Qt version in
> projects that don't require them AND I think that one should support the
> current LTS release in as many projects as possible as a general rule of
> principle.
> 
> There's a reason why those LTS releases exist and that should probably be
> taken into consideration ESPECIALLY for the KF5 Frameworks (remember why
> kdelibs4 was split up)!

Which is exactly why 5.11.3 (released today) should be picked. It contains all 
fixes that 5.9.7 contains, whereas 5.10.1 does not. Moving from 5.9.7 to 
5.10.1 means regressing all those fixes.

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

Re: auto QString(Builder) considered VERY HARMFUL

[Thiago Macieira]

On Thursday, 27 September 2018 12:30:30 PDT Elvis Angelaccio wrote:
> is it possible to fix Qt instead?

No, the C++ language needs to be fixed. Search for "operator auto" for 
proposals.

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

Re: network connectivity with proxy

[Thiago Macieira]

On Wednesday, 19 September 2018 15:20:27 PDT Christoph Feck wrote:
> On 20.09.2018 00:03, Albert Astals Cid wrote:
> > El divendres, 14 de setembre de 2018, a les 18:04:59 CEST, Thiago Macieira 
va escriure:
> >> On Wednesday, 12 September 2018 04:17:35 PDT Harald Sitter wrote:
> >>> If I am not mistaken Qt already looks in the environment variables for
> >>> http_proxy, https_proxy etc. etc. (I don't think Plasma sets those
> >>> from the KCM though). To that end a no-code solution is setting those.
> >>> Obviously that's not very integrated in plasma.
> >> 
> >> The correct way is to configure your system properly. Qt will use
> >> libproxy and will get the settings.
> > 
> > So this should be a bug reported against systemsettings or kded or
> > something then?
> libproxy can be compiled with KF5 support, so it can automatically read
> the proxy settings that kcm_proxy writes. The problem is that this
> support is/was disabled by default, because of
> https://bugreports.qt.io/browse/QTBUG-52252
> 
> Distributions might need to check if they need to re-enable it.

libproxy tries to detect if you're running a Qt5 application.

In any case, I recommend using the pacrunner's libproxy instead of libproxy's 
libproxy. The proxy settings won't be read from KDE's. Instead, they will be 
centralised in the pacrunner daemon. KDE should provision it (it's just a D-
Bus call, plasma-nm can do it).

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

Re: network connectivity with proxy

[Thiago Macieira]

On Wednesday, 12 September 2018 04:17:35 PDT Harald Sitter wrote:
> If I am not mistaken Qt already looks in the environment variables for
> http_proxy, https_proxy etc. etc. (I don't think Plasma sets those
> from the KCM though). To that end a no-code solution is setting those.
> Obviously that's not very integrated in plasma.

The correct way is to configure your system properly. Qt will use libproxy and 
will get the settings.

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

Re: Installing qml caches

[Thiago Macieira]

On Sunday, 8 July 2018 09:06:58 PDT Simon Hausmann wrote:
> And use the QtQuickCompiler build
> option with 5.11.

Is that just running qmlcachegen on all the *.qml?

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

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 (const T , const T ) { 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

Re: Q_ASSERT(!FalseSecurity)

[Thiago Macieira]

On Saturday, 10 March 2018 00:53:24 PDT Michael Heidelbach wrote:
> Hi!
> 
> Am I getting something wrong? Or is
> 
> "Q_ASSERT(m_writeTrans);
> 
> m_writeTrans->commit();"
> 
> providing false security?

This is not false security. It's not security, period.

You use a Q_ASSERT when a violation indicates a bug that needs to be fixed, 
instead of a normal condition. So the above code is fine.

> Shouldn't it better be
> 
> "Q_ASSERT(m_writeTrans);
> 
> if (m_writeTrans) {
> 
>  m_writeTrans->commit(); ?

It should definitely NOT be this.

Use either Q_ASSERT or the if, not both.

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

Re: Unable to run the unit test - receiving signal 11

[Thiago Macieira]

On Saturday, 24 February 2018 16:26:24 PST Dileep Sankhla wrote:
> GNU gdb (GDB) 8.0.1
> .
> .
> .
> 
> (gdb) = End of stack trace ==
> QFATAL : MainShellTest::testShell(open file) Received signal 11
> Function time: 646ms Total time: 649ms
> FAIL!  : MainShellTest::testShell(open file) Received a fatal error.
>   Loc: [Unknown file(0)]
> Totals: 2 passed, 1 failed, 0 skipped, 0 blacklisted, 845ms
> * Finished testing of MainShellTest *
> Aborted (core dumped)
> 
> 
> What is going wrong? How to resolve it?

The interesting part in the message was replaced by . . . 

Bring the information back and, if necessary, install debug symbols for the 
libraries in that backtrace.

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

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 )
>  : 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 ,
> -const QVariant 
>  = QVariant(),
> -const QVariant 
>  = QVariant());
> -PrivilegeOperationReturnValue tryOpen(QFile , const QByteArray , 
> int flags, int mode);
> +PrivilegeOperationReturnValue execWithElevatedPrivilege(ActionType 
> action, const QVariantList , int errcode);
> +PrivilegeOperationReturnValue tryOpen(QFile , const QByteArray , 
> 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=113699766611213=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

Re: releaseme new requirement: non-conflicting files on case-insensitive FS/OS

[Thiago Macieira]

On segunda-feira, 20 de novembro de 2017 09:59:10 PST Reindl Harald wrote:
> you missed "what do you gain from uppercase chars in filenames?"

Why should we constrain ourselves to lowercase?

We've had uppercase library names for over 12 years (starting with Qt 4.0). 
And that's not including libX11, libGL, libGLES, libGLU, libICE, libSM...

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

Re: releaseme new requirement: non-conflicting files on case-insensitive FS/OS

[Thiago Macieira]

On segunda-feira, 20 de novembro de 2017 07:58:06 PST Reindl Harald wrote:
> hopefully this means also that filenames in the future don't contain
> uppercase chars

Why would we want that?

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

Re: c++11 and workspace

[Thiago Macieira]

On Thursday, 17 August 2017 08:52:21 PDT Martin Flöser wrote:
> Am 2017-08-17 16:48, schrieb Hugo Pereira Da Costa:
> > Hi,
> > 
> > Quick question on the status of c++11 features that I can include in
> > breeze. Are std::function allowed ?
> 
> In workspace you can/should use C++11, KWin/master even requires C++14.

Confirm if you mean the core language or the standard library.

-- 
Thiago Macieira - thiago (AT) macieira.info - thiago (AT) kde.org
   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 );
> +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

Re: QProcess::startDetached and bash stdout capture

[Thiago Macieira]

On sábado, 25 de março de 2017 09:03:28 PDT David Faure wrote:
> On samedi 25 mars 2017 16:50:37 CET Thiago Macieira wrote:
> > One problem is because the launched process inherits the pipes to stdout
> > and stderr
> 
> Really, even with startDetached? I assumed that was more "detached" than
> that 

But you may want:

int fd = open("/dev/null", O_RDWR);
dup2(fd, STDIN_FILENO);
dup2(fd, STDOUT_FILENO);
dup2(fd, STDERR_FILENO);
close(fd);

Like I said, this may be useful to add as a flag to QProcess::startDetached 
too.

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

Re: QProcess::startDetached and bash stdout capture

[Thiago Macieira]

On sábado, 25 de março de 2017 09:03:28 PDT David Faure wrote:
> On samedi 25 mars 2017 16:50:37 CET Thiago Macieira wrote:
> > One problem is because the launched process inherits the pipes to stdout
> > and stderr
> 
> Really, even with startDetached? I assumed that was more "detached" than
> that ;)

Right, nothing in it closes stdout, because people want to see the output. 
When we're connected to the terminal, that's usually useful.

If we're connected to a pipe... not so much.

> > but the shell sees the process group it launched exiting.
> 
> I never understood much about "process group". Is it the issue here? Is the
> helper considered part of the same process group as kdialog?

startDetached does setsid(), so it starts a new session and process group. I 
have to confess not understanding SID and PGID very well either, but I do know 
that each process the shell launches is in its own process group and the shell 
waits for the process group to exit, not just the process it launched.

> > It will probably close the pipe. Normally, this process would exit soon
> > with SIGPIPE, but since a lot of code relating to sockets and pipes turns
> > SIGPIPE off, it's possible that the detached process never receives
> > SIGPIPE and simply remains forever.
> 
> I'm confused as to which one you call the "detached process" here,
> kdialog or the helper.

The helper.

> Are you saying what I need is a call to sigaction to re-enable exiting on
> SIGPIPE, in kdialog, just before returning from main()?

No, that won't help.

> Could it be related to SIGCHLD, rather? I know QProcess[Manager] plays with
> that, and it sounds like what the shell is waiting for from kdialog. But
> surely inhibiting the -receiving- of SIGCHLD in kdialog shouldn't break the
> parent shell receiving SIGCHLD when kdialog terminates?

No, SIGCHLD isn't the case here. SIGCHLD is reset to default state by exec() 
and even if it were ignored in the helper, it wouldn't make a difference.

> > > I tried close(1) like the kde4 kdialog was doing (due to forking,
> > > though),
> > > no change.
> > 
> > There's another problem: you have a race condition between the parent
> > (kdialog) exiting and the grandchild (helper) writing its information
> > before closing stdout.
> 
> No no, kdialog is the one writing to stdout. It's just writing out the dbus
> service name that it knows the helper will use.
> It's much simpler that way, it avoids all the issues you mention.

Hmm... ok, then I don't understand. I need a bit more of information.

1) does the shell read properly from kdialog and get the service address? Is 
it correct?

2) did kdialog exit? Is kialog the process in  state?

3) is the helper still running? Can you paste the ps j for it? I'd like to see 
the PGID and SID. If you can show the shell, the zombie process and ps itself, 
it would help comparison.

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

Re: QProcess::startDetached and bash stdout capture

[Thiago Macieira]

On sábado, 25 de março de 2017 08:50:37 PDT Thiago Macieira wrote:
> One problem is because the launched process inherits the pipes to stdout and
> stderr, but the shell sees the process group it launched exiting. It will
> probably close the pipe. Normally, this process would exit soon with
> SIGPIPE, but since a lot of code relating to sockets and pipes turns
> SIGPIPE off, it's possible that the detached process never receives SIGPIPE
> and simply remains forever.

Just found out that startDetached ignores SIGPIPE for all its child processes. 
And that has been there forever (see [1]). It's one of those old bug reports 
we used to get that the launched process received SIGPIPE after trying to 
write to stdout (like it should!) and then we decided to "help" the customer 
without understanding the issue properly.

The problem is that this is very common: launched processes often write 
debugging information to stderr... So we can't change it now. Instead, we 
could add a flag to startDetached and to indicate we should close stdin, stdout 
and stderr, reopening with /dev/null, and let SIGPIPE back to the default.

[1] http://code.qt.io/cgit/qt/qt.git/tree/src/corelib/io/qprocess_unix.cpp?
h=v4.5.1#n1316

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