GNOME Bugzilla – Bug 722889
Redirect Xorg.log to the systemd journal
Last modified: 2014-01-28 14:46:01 UTC
Created attachment 267101 [details] [review] 0001-gdm-let-the-xserver-log-to-the-journal.patch 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.
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?
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.
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.
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.
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.
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.
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]
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?
(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.