GNOME Bugzilla – Bug 676181
Don't write log file
Last modified: 2013-01-22 16:56:50 UTC
Currently we redirect all stderr and stdout from the session to a file on disk, in the user's home directory. This has a lot of problems. The most obvious is that it can grow extremely large. It isn't rotated. It can block. There aren't tools to see it. The admin is totally unaware of it. Ultimately we want this information in the system journal where it can be probed with tools on the command line or https://live.gnome.org/Design/Apps/SystemLog Probably the easiest way to do this is to just use syslog.
I'd say a prerequisite for this is that we have a simple way to get back the same information that was once in .xsession-errors. A simple: journalctl _EXE=/usr/sbin/gdm-binary perhaps?
Talked with mccann and kay about this on irc. We concluded that using syslog() is not ideal. A better approach is to get the stdio fds from sd_journal_stream_fd() instead of open("~/.xsession-errors", ...)
*** Bug 680782 has been marked as a duplicate of this bug. ***
*** Bug 516879 has been marked as a duplicate of this bug. ***
Created attachment 230598 [details] [review] Remove handling of SIGSEGV/SIGFPE/SIGILL etc. Modern operating systems have "crash catching" functionality; for example, systemd comes with "systemd-coredump" which collects cores automatically. Attempting to handle these kinds of fatal signals internally is now much worse, because the original source of the problem will be masked. systemd won't collect a core file that would include a backtrace, for example. Also, with these removed, we can move forward porting to g_unix_signal_add().
Created attachment 230599 [details] [review] Port to g_unix_signal_add(), drop GdmSignalHandler The level of copy/paste going on here before is rather astonishing. For example, in some cases, I dropped spurious handling of SIGHUP, when the code didn't have any settings to reread. Anyways, the code is now clearer, and we get to drop all the bits of gdm-signal-handler.[ch] for the integrated GLib handling.
Previous discussion around these patches in https://bugzilla.gnome.org/show_bug.cgi?id=689569
Created attachment 233919 [details] [review] 0001-Log-output-to-systemd-journal-if-available.patch This patch actually turned out to be a lot simpler than I thought it would be. We keep the old code for logging to files for non-systemd systems. But applying this patch to the latest gnome-ostree and booting in qemu, I have no ~/.cache/gdm, and I do see the expected g_critical stuff logged under "gdm-1000" in "journalctl -b" I'm open to changing the "gdm-UID" identifier. Maybe gdm-username instead?
Review of attachment 233919 [details] [review]: hey thanks for looking at this! ::: daemon/gdm-session-worker.c @@ +1858,3 @@ } if (!worker->priv->is_program_session) { shouldn't this say if (sd_is_booted || !worker->prov->is_program_session) ? What i mean is, you don't open the program session log above anymore, so you need open it here, or we won't get greeter/initial-setup log messages, right?
(In reply to comment #9) > What i mean is, you don't open the program session log above anymore, so you > need open it here, or we won't get greeter/initial-setup log messages, right? Those get logged by inheriting the fds from the parent. That's what the second paragraph of the commit message explains.
i'm not talking about users with UID 1000. I'm talking about the greeter with UID 42. If is_program_session is TRUE, we now start that session with a -1 stdout/stderr right? that's broken.
so walters pointed out on IRC, the last hunk of the patch makes log output "inherit" from wherever the daemon itself is logging to (as configured by gdm)
Created attachment 234039 [details] [review] Log output to systemd journal if available Previously, we put stuff in /var/log/gdm for the session, and then ~/.cache/gdm/session.log for the user. Now let's use explicit sd_journal_stream_fd() calls. Some adjustments from Ray Strode.
i've made some changes there, so we have a separate stream for error versus output. Also, I forgot to drop this comment: + /* In the systemd case, our stdout will be connected + * to the journal, so we just let the child inherit + * those FDs. + */ Even though, I changed it to not do that and instead always create a new argv[0] stream.
Review of attachment 234039 [details] [review]: One minor comment, otherwise looks good. ::: daemon/gdm-session-worker.c @@ +1886,3 @@ +#endif + if (!has_systemd && !worker->priv->is_program_session) { + if (!has_systemd && home_dir != NULL && home_dir[0] != '\0') { Can drop the if (!has_systemd) here
While testing my original patch a bit more, I noticed I still had a -slave.log. That one comes from gdm/daemon/gdm-slave-proxy.c:spawn_child_setup.
One concern: For systems (like say Fedora 18, maybe 19), that ship systemd but still use syslog by default, that's where the session log will end up. Is that bad? Good? I don't know... If it's bad, then maybe we need a build-time option --enable-systemd-journal.
Created attachment 234041 [details] [review] Log output to systemd journal if available Previously, we put stuff in /var/log/gdm for the session, and then ~/.cache/gdm/session.log for the user. Now let's use explicit sd_journal_stream_fd() calls. Some adjustments from Ray Strode.
(In reply to comment #17) > One concern: > > For systems (like say Fedora 18, maybe 19), that ship systemd but still use > syslog by default, that's where the session log will end up. Is that bad? > Good? I don't know... > > If it's bad, then maybe we need a build-time option --enable-systemd-journal. That's a really good point, we probably should have that.
Created attachment 234043 [details] [review] Log output to systemd journal if available Previously, we put stuff in /var/log/gdm for the session, and then ~/.cache/gdm/session.log for the user. Now let's use explicit sd_journal_stream_fd() calls. Some adjustments from Ray Strode.
Review of attachment 234043 [details] [review]: ::: daemon/gdm-slave-proxy.c @@ +221,3 @@ } + data.identifier = argv[0]; probably should strdup this.
Review of attachment 234043 [details] [review]: A couple of minor bits, thanks a lot for picking up the torch here! ::: configure.ac @@ +964,3 @@ +fi + +if test "x$use_journald" != "xno" ; then Since you have autoconf macros inside a conditional, should use AS_IF(). See https://bugzilla.gnome.org/show_bug.cgi?id=681413 ::: daemon/gdm-slave-proxy.c @@ +141,3 @@ +#ifdef ENABLE_SYSTEMD_JOURNAL +static void +clear_close_on_exec_flag (int fd) Isn't there a place to put this utility function? gdm-common.[ch]? We've both written broken F_SETFD code before, so =) @@ +221,3 @@ } + data.identifier = argv[0]; You don't actually need to since the forked child is going to inherit a copy of memory.
Hi, > Since you have autoconf macros inside a conditional, should use AS_IF(). See > https://bugzilla.gnome.org/show_bug.cgi?id=681413 That's only if the macros compile code right? We're just doing AC_DEFINE(). Anway, that pattern is throughout the configure script, so would need to get fixed in one run rather than piecemeal. > Isn't there a place to put this utility function? gdm-common.[ch]? Yea, that's a really good idea.
Attachment 230598 [details] pushed as 59d7059 - Remove handling of SIGSEGV/SIGFPE/SIGILL etc. Attachment 230599 [details] pushed as 4952f6e - Port to g_unix_signal_add(), drop GdmSignalHandler Attachment 234043 [details] pushed as a505c58 - Log output to systemd journal if available