After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 722889 - Redirect Xorg.log to the systemd journal
Redirect Xorg.log to the systemd journal
Product: gdm
Classification: Core
Component: general
Other Linux
: Normal normal
: ---
Assigned To: GDM maintainers
GDM maintainers
Depends on:
Reported: 2014-01-24 05:15 UTC by Peter Hutterer
Modified: 2014-01-28 14:46 UTC
See Also:
GNOME target: ---
GNOME version: ---

0001-gdm-let-the-xserver-log-to-the-journal.patch (5.29 KB, patch)
2014-01-24 05:15 UTC, Peter Hutterer
committed Details | Review

Description Peter Hutterer 2014-01-24 05:15:40 UTC
Created attachment 267101 [details] [review]

If systemd is running, don't re-route the xserver's stdout/stderr to a log     file. Instead hook it up to a journal stream and let the xserver log to the     journal.

Patch is attached, it makes gdm start the server with
      Xorg <other options> -logfile /dev/null -verbose 3

(where 3 is the default Xorg log verbosity).

Comments on the patch:
* on error, it still uses the log file, and things are unchanged for non-systemd setups
* needed an extra variable to store the program name. I could get that by parsing argv from server_child_setup but that seems more effort than just strdup argv[0]
* we could add similar options to the server itself but we won't get much benefit from it. we need to printf in the signal handler so even in the server we'd have to open a stream and write() to it, so we don't get all the other benefits that sd_journal_print() gives us anyway.
Comment 1 zbyszek 2014-01-24 12:54:05 UTC
This is a big improvement in usability. And it is a relatively painless way to achieve journal integration. But it would be even nicer to hook up Xorg with journal logging directly, without going through the stream API. One difference is that Xorg has it's own notion of log priorities and they are necessarilly lost along with other metadata like line numbers. Is it possible to change X?
Comment 2 Peter Hutterer 2014-01-24 20:56:50 UTC
the biggest problem at the moment is that X has a number of printf statements in the signal handler and thus needs a sigsafe printing function. In the server we have our own little pnprintf implementation for that and just write(2) to the fd.

unless there's a sigsafe sd journal function we can't use it as-is yet.

of course one option would be to just use the normal sd functions for non-signal logging and open up an extra fd for the sigsafe logging. that would only lose us the file information on some run-time messages and bug notifications.
Comment 3 zbyszek 2014-01-24 21:53:51 UTC
sd_journal_sendv is signal safe, I think. All it does is
open() (sometimes), some on-stack memory manipulation, and sendmsg(). There's also a fallback path when the message is too large, but it shouldn't apply here.
So pnprintf could be modified to call sd_journal_sendv.

By itself calling sd_journal_sendv from pnprintf is not an advantage over simply writing to the journal fd. But LogVMessageVerbSigSafe could be modified to pass structured data. It could also be turned into a macro like sd_journal_print, and append FUNCTION, LINE, and FILE.
Comment 4 Peter Hutterer 2014-01-24 22:06:09 UTC
I'm happy to use whatever is available, but I'd want _guarantees_ that it is signal safe. Not just "the current implementation is" but documented as signal safe and that it'll stay that way. It takes a while to get things released in an xserver, I'd prefer that by the time we release the implementation hasn't changed to something incompatible.
Comment 5 zbyszek 2014-01-24 22:08:29 UTC
OK. I'll look over the functions again, and submit a documentation patch for sd_journal_send(3) for review to systemd-devel to make it official.
Comment 6 Hans de Goede 2014-01-25 10:56:52 UTC
Note being signal safe means that:
1) It cannot take any locks, because a signal may interrupt the main thread while holding the lock, and then we've a dead lock
2) It must first assemble the entire message in a buffer and then write(2) it in a single call to avoid interleaving parts of messages

It would be acceptable if we need to first call some open() function in a non signal path before hand, to guarantee things to be signal safe.
Comment 7 Ray Strode [halfline] 2014-01-27 21:05:56 UTC
I'm going to go ahead and push the patch in comment 0 even if the X server handles things on its own going forward.  it won't hurt, and it helps in the mean time.

I am going to make minor changes to the patch to encode the display name in the identifier instead of just using argv[0]
Comment 8 Thomas Andersen 2014-01-28 09:28:36 UTC
It seems to me that the #ifdef ENABLE_SYSTEMD_JOURNAL in gdm_server_setup_journal_fds ends one line too early. Shouldn't we return false if it is not defined?
Comment 9 Ray Strode [halfline] 2014-01-28 14:46:01 UTC
(In reply to comment #8)
> It seems to me that the #ifdef ENABLE_SYSTEMD_JOURNAL in
> gdm_server_setup_journal_fds ends one line too early. Shouldn't we return false
> if it is not defined?

yes, thanks.