<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/128790/">https://git.reviewboard.kde.org/r/128790/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 28th, 2016, 1:41 p.m. UTC, <b>Oswald Buddenhagen</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">uhm, and why do you think utempter is the preferred choice?
last time i checked, nobody was shipping konsole setgid utmp. with kdeinit, that's not even an option.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">there are several options how to deal with this:
- fork()/wait() around the utempter calls, so it can't mess with the signal handling of the current process. though i seem to remmber that the addToUtmp() call actually uses the PID
- re-implement the libutempter calls with QProcess. that's actually how it was originally, but was changed because there were incompatible versions of utempter - but that seems like a minor concern compared to the status quo.
- drop utmp handling altogether, as it's been mostly superseded, first by consolekit, and now logind. however, respective bindings would have to be actually implemented, and i have no clue how things are supposed to be done. just some dbus calls?
-- what about non-linux systems?</p></pre>
</blockquote>
<p>On August 28th, 2016, 1:52 p.m. UTC, <b>Martin Tobias Holmedahl Sandsmark</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">uhm, and why do you think utempter is the preferred choice?
last time i checked, nobody was shipping konsole setgid utmp. with kdeinit, that's not even an option.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Why is the fallback code there at all, then?</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">fork()/wait() around the utempter calls, so it can't mess with the signal handling of the current process. though i seem to remmber that the addToUtmp() call actually uses the PID</li>
</ul>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Won't that break if another process exits at the wrong time?</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">re-implement the libutempter calls with QProcess. that's actually how it was originally, but was changed because there were incompatible versions of utempter - but that seems like a minor concern compared to the status quo.</li>
</ul>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I looked at that as well, the issue is finding the correct path for the utempter helper.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">drop utmp handling altogether, as it's been mostly superseded, first by consolekit, and now logind. however, respective bindings would have to be actually implemented, and i have no clue how things are supposed to be done. just some dbus calls?</li>
</ul>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There seems to be a simple dbus call to register with logind at least. I tried looking briefly at it, but I couldn't quickly find any logind code that did utmp stuff. I didn't look very hard, though.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">-- what about non-linux systems?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">libutempter only supports Linux and FreeBSD, the fallback code seems to at least try to be compatible with other platforms.</p></pre>
</blockquote>
<p>On August 28th, 2016, 2:33 p.m. UTC, <b>Oswald Buddenhagen</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Why is the fallback code there at all, then?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">it was there first. it will actually work if somebody runs konsole as root. which nobody does, of course.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Won't that break if another process exits at the wrong time?</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">not if the parent doesn't do any messing with the signal handling. which it doesn't need to, as the defaults (and what qprocess does) are perfectly fine.
the likely pid problem remains. one could wrap only the unregistration, but then a hypothetical race between utempter registration calls and unrelated qprocess exits remains.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I looked at that as well, the issue is finding the correct path for the utempter helper.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">the configure test could run 'strings' over libutempter.so and grep for relevant patterns. ^^
or just try to divine it from the libutempter location based on typical directory structures.
the last resort would be letting the user specify it.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I tried looking briefly at it, but I couldn't quickly find any logind code that did utmp stuff</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">logind doesn't do the legacy stuff any more (consolekit still did, iirc). you're supposed to use loginctl.
but i don't know whether one is actually supposed to register pseudo ttys in the first place.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">libutempter only supports Linux and FreeBSD</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">the two dashes were to meant to illustrate a sub-point. i.e., what about non-systemd systems?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">a whole different approach would be providing an own kutempter - quite similar to kcheckpass (which is mostly redundant with pam's unix_chkpwd). or actually, to kgrantpty, which is redundant with the pt_chown helper some grantpt() implementations use.
i actually once had a todo item about that ...</p></pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">the configure test could run 'strings' over libutempter.so and grep for relevant patterns. ^^
or just try to divine it from the libutempter location based on typical directory structures.
the last resort would be letting the user specify it.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yeah, I started on a patch that checks KDE_INSTALL_FULL_LIBEXECDIR, KDE_INSTALL_FULL_LIBDIR, as well as /usr/lib and /usr/libexec explicitly, with and without a /utempter/ suffix to the paths.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The problem is that the utempter helper expects its standard stdin/stdout/stderr to be the masterFd, which I guess we need to hack around somehow. I see that the old solution used setupChildProcess() to do this.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">logind doesn't do the legacy stuff any more (consolekit still did, iirc). you're supposed to use loginctl.
but i don't know whether one is actually supposed to register pseudo ttys in the first place.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't think so, the logind documentation is pretty explicit about clients not supposed to be using CreateSession() at least, it should only be used by pam_systemd. AttachDevice() might do what we want, but as you said, it's Linux only.</p>
<blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">a whole different approach would be providing an own kutempter - quite similar to kcheckpass (which is mostly redundant with pam's unix_chkpwd). or actually, to kgrantpty, which is redundant with the pt_chown helper some grantpt() implementations use.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Yeah, I thought about that as well, but it seems like too much work for something that I don't really think is important. I don't really see the value of getting an entry in utmp every time I open a konsole tab in the first place.</p></pre>
<br />
<p>- Martin Tobias Holmedahl</p>
<br />
<p>On August 28th, 2016, 1:13 p.m. UTC, Martin Tobias Holmedahl Sandsmark wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks, David Faure, Kurt Hindenburg, Rex Dieter, and Thiago Macieira.</div>
<div>By Martin Tobias Holmedahl Sandsmark.</div>
<p style="color: grey;"><i>Updated Aug. 28, 2016, 1:13 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=364779">364779</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kpty
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">According to the investigation in https://bugs.kde.org/show_bug.cgi?id=364779 utempter does stuff in a way that isn't compatible with QProcess/KProcess/KPtyProcess (calling sigaction() before launching its child process). So remove it, and rely on the fallback methods already implemented.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>CMakeLists.txt <span style="color: grey">(3e17cac)</span></li>
<li>KF5PtyConfig.cmake.in <span style="color: grey">(66f8c43)</span></li>
<li>cmake/FindUTEMPTER.cmake <span style="color: grey">(a3ea06a)</span></li>
<li>src/CMakeLists.txt <span style="color: grey">(caab96f)</span></li>
<li>src/ConfigureChecks.cmake <span style="color: grey">(ded08f4)</span></li>
<li>src/config-pty.h.cmake <span style="color: grey">(aaaf8d9)</span></li>
<li>src/kpty.cpp <span style="color: grey">(15c3b81)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/128790/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>