Re: Review Request 128790: Remove usage of utempter
> On Aug. 28, 2016, 1:41 p.m., Oswald Buddenhagen wrote: > > 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. > > > > 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? > > Martin Tobias Holmedahl Sandsmark wrote: > > 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. > > Why is the fallback code there at all, then? > > > > - 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 > > Won't that break if another process exits at the wrong time? > > > > - 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. > > I looked at that as well, the issue is finding the correct path for the > utempter helper. > > > > - 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? > > 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. > > > > -- what about non-linux systems? > > libutempter only supports Linux and FreeBSD, the fallback code seems to > at least try to be compatible with other platforms. > > Oswald Buddenhagen wrote: > > Why is the fallback code there at all, then? > > it was there first. it will actually work if somebody runs konsole as > root. which nobody does, of course. > > > Won't that break if another process exits at the wrong time? > > 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. > > > I looked at that as well, the issue is finding the correct path for the > utempter helper. > > 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. > > > I tried looking briefly at it, but I couldn't quickly find any logind > code that did utmp stuff > > 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. > > > libutempter only supports Linux and FreeBSD > > the two dashes were to meant to illustrate a sub-point. i.e., what about > non-systemd systems? > > 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 ... > 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. 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. The problem is that the utempter
Re: Review Request 128790: Remove usage of utempter
> On Aug. 28, 2016, 1:41 p.m., Oswald Buddenhagen wrote: > > 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. > > > > 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? > > Martin Tobias Holmedahl Sandsmark wrote: > > 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. > > Why is the fallback code there at all, then? > > > > - 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 > > Won't that break if another process exits at the wrong time? > > > > - 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. > > I looked at that as well, the issue is finding the correct path for the > utempter helper. > > > > - 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? > > 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. > > > > -- what about non-linux systems? > > libutempter only supports Linux and FreeBSD, the fallback code seems to > at least try to be compatible with other platforms. > Why is the fallback code there at all, then? it was there first. it will actually work if somebody runs konsole as root. which nobody does, of course. > Won't that break if another process exits at the wrong time? 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. > I looked at that as well, the issue is finding the correct path for the > utempter helper. 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. > I tried looking briefly at it, but I couldn't quickly find any logind code > that did utmp stuff 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. > libutempter only supports Linux and FreeBSD the two dashes were to meant to illustrate a sub-point. i.e., what about non-systemd systems? 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 ... - Oswald --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128790/#review98742 --- On Aug. 28, 2016, 1:13 p.m., Martin Tobias Holmedahl Sandsmark wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128790/ > --- > > (Updated Aug. 28, 2016, 1:13 p.m.) > > > Review request for KDE Frameworks, David Faure, Kurt Hindenburg, Rex Dieter, > and Thiago Macieira.
Re: Review Request 128790: Remove usage of utempter
> On Aug. 28, 2016, 1:41 p.m., Oswald Buddenhagen wrote: > > 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. > > > > 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? > 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. Why is the fallback code there at all, then? > - 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 Won't that break if another process exits at the wrong time? > - 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. I looked at that as well, the issue is finding the correct path for the utempter helper. > - 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? 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. > -- what about non-linux systems? libutempter only supports Linux and FreeBSD, the fallback code seems to at least try to be compatible with other platforms. - Martin Tobias Holmedahl --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128790/#review98742 --- On Aug. 28, 2016, 1:13 p.m., Martin Tobias Holmedahl Sandsmark wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128790/ > --- > > (Updated Aug. 28, 2016, 1:13 p.m.) > > > Review request for KDE Frameworks, David Faure, Kurt Hindenburg, Rex Dieter, > and Thiago Macieira. > > > Bugs: 364779 > https://bugs.kde.org/show_bug.cgi?id=364779 > > > Repository: kpty > > > Description > --- > > According to the investigation in https://bugs.kde.org/show_bug.cgi?id=364779 > utempter does stuff in a way that isn't compatible with > QProcess/KProcess/KPtyProcess (calling sigaction() before launching its child > process). So remove it, and rely on the fallback methods already implemented. > > > Diffs > - > > CMakeLists.txt 3e17cac > KF5PtyConfig.cmake.in 66f8c43 > cmake/FindUTEMPTER.cmake a3ea06a > src/CMakeLists.txt caab96f > src/ConfigureChecks.cmake ded08f4 > src/config-pty.h.cmake aaaf8d9 > src/kpty.cpp 15c3b81 > > Diff: https://git.reviewboard.kde.org/r/128790/diff/ > > > Testing > --- > > > Thanks, > > Martin Tobias Holmedahl Sandsmark > >
Re: Review Request 128790: Remove usage of utempter
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128790/#review98742 --- 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. 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? - Oswald Buddenhagen On Aug. 28, 2016, 1:13 p.m., Martin Tobias Holmedahl Sandsmark wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/128790/ > --- > > (Updated Aug. 28, 2016, 1:13 p.m.) > > > Review request for KDE Frameworks, David Faure, Kurt Hindenburg, Rex Dieter, > and Thiago Macieira. > > > Bugs: 364779 > https://bugs.kde.org/show_bug.cgi?id=364779 > > > Repository: kpty > > > Description > --- > > According to the investigation in https://bugs.kde.org/show_bug.cgi?id=364779 > utempter does stuff in a way that isn't compatible with > QProcess/KProcess/KPtyProcess (calling sigaction() before launching its child > process). So remove it, and rely on the fallback methods already implemented. > > > Diffs > - > > CMakeLists.txt 3e17cac > KF5PtyConfig.cmake.in 66f8c43 > cmake/FindUTEMPTER.cmake a3ea06a > src/CMakeLists.txt caab96f > src/ConfigureChecks.cmake ded08f4 > src/config-pty.h.cmake aaaf8d9 > src/kpty.cpp 15c3b81 > > Diff: https://git.reviewboard.kde.org/r/128790/diff/ > > > Testing > --- > > > Thanks, > > Martin Tobias Holmedahl Sandsmark > >
Review Request 128790: Remove usage of utempter
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/128790/ --- Review request for KDE Frameworks, David Faure, Kurt Hindenburg, Rex Dieter, and Thiago Macieira. Bugs: 364779 https://bugs.kde.org/show_bug.cgi?id=364779 Repository: kpty Description --- According to the investigation in https://bugs.kde.org/show_bug.cgi?id=364779 utempter does stuff in a way that isn't compatible with QProcess/KProcess/KPtyProcess (calling sigaction() before launching its child process). So remove it, and rely on the fallback methods already implemented. Diffs - CMakeLists.txt 3e17cac KF5PtyConfig.cmake.in 66f8c43 cmake/FindUTEMPTER.cmake a3ea06a src/CMakeLists.txt caab96f src/ConfigureChecks.cmake ded08f4 src/config-pty.h.cmake aaaf8d9 src/kpty.cpp 15c3b81 Diff: https://git.reviewboard.kde.org/r/128790/diff/ Testing --- Thanks, Martin Tobias Holmedahl Sandsmark
