Thanks for the patch! However, I think its logic is not entirely sound.

Surely if .xsession-errors is a socket or a directory, gdm can't use it to write its log. It's also a good question what to do if it's a block or character device (especially if the fs is mounted nodev). (Arguably it could try to connect() to a socket.)

Until someone comes up with good answers, I think it'd be better to make the minimal change of _not_ renaming it if it's a symlink or a fifo, but renaming it if it's anything else. If I understood your patch correctly, it does the opposite: it leaves .xsession-errors alone UNLESS it is a regular file.

Actually, gdm also contains a functionality that it opens log as temporary file at ~/.xsession-errors.XXXXXXXX if file is non-regular. But this branch of code is not used currently as .xsession-errors is renamed before this check.

But if my patch is applied, it will be used. In case if .xsession-error is special file gdm will return to (previous?) behaviour of creating such temporary logs. This also applies to fifo, so it turned out that my patch doesn't fix with fifo entirely - it needs to check on fifo in _fd_is_normal_file().

Personally, I think that it shouldn't rename any file that normally supposed to be regular but is something special, like device/directory or socket. Anyway it's up to gdm developers define what behaviour do they want, so waiting for them.

Created attachment 186717 [details] [review]
Allow .xsession-errors to be a symlink

Incidentally I stumbled over the same problem. My patch uses g_file_test() instead of a new function, and only tests for being a symlink because I want the later code to do proper error/fallback handling if .xsession-errors happens to be a directory. I leave the current behaviour in place for fifos, as I think it would actually be safer to have them rotated away. Otherwise, if you have a lost fifo sitting around there with noone listening, you risk blocking or breaking programs which are unable to write to stderr.

The later code will truncate the file to zero anyway, so even in the case
of a symlink where we don't rotate, the file won't grow indefinitely.

(In reply to comment #3)

> to be a directory. I leave the current behaviour in place for fifos, as I think
> it would actually be safer to have them rotated away. Otherwise, if you have a
> lost fifo sitting around there with noone listening, you risk blocking or
> breaking programs which are unable to write to stderr.

In the same vein, it would be safer to disable the rm(1) command because otherwise it would allow a user to inadvertently delete some important file.

Sorry, but I vehemently disagree. There are no "lost" FIFOs. Anyone who creates a ~/.xsession-errors FIFO should be assumed to know what they're doing and be allowed to shoot themselves in the foot if that is what they want. Gdm has no business trying to be smarter than users. Madness? This is Unix! (scnr)

Gdm also can't anticipate _why_ .xesssion-errors is a FIFO. It could be a FIFO for the very reason that the user wants programs that write to stderr to block (and to continue at the specific moment when he or she attaches a listener to the FIFO).

The program listening on a FIFO may, in addition to rotation, perform log processing, timestamping or any number of other things that gdm won't (and shouldn't) do.

Moving FIFOs away that gdm knows nothing about makes it unnecessarily hard to integrate gdm with other Unix programs in ways the gdm developers didn't anticipate; so it not only violates the principle of least surprise ("wtf happened to my fifo?") but also what I perceive to be "The Unix Way".

Fair enough. I'm not that picky about which of the two patches gets applied. I did mine for a stable release update, and for these we want to change as little behaviour as possible.

Thanks for the feedback!

@Ray or Brian: Is either of the patches ok for you? I'm happy to commit them, but don't want to do so without your ack.

So the idea here is:

1) user has slow nfs home directory
2) user is getting junk performance because buggy apps are spamming ~/.xsession-errors with g_warnings and such
3) user tries to fix the problem by doing ln -sf $XDG_RUNTIME_DIR/.xsession-errors ~/.xsession-errors or something
4) to their dismay it doesn't work because the symlink just gets renamed to ~/.xsession-errors.old next time they log in.

Is that about right?

@Ray, mostly; the specific issue was not (only) with symlinks but also with FIFOs (I wanted to have automatically rotated and timestamped error logs, and a good way to accomplish that would have been to have a ~/.xsession-errors FIFO with an svlogd(8) reading from it).

There may be other cases where someone may want to have a ~/.xsession-errors symlink or FIFO.

Review of attachment 186717 [details] [review]:

Honestly, I could take-or-leave the FIFO use case.  Actually, I sort of lean toward explicitly not supporting it until we have a clear idea how it could be supported.

Another thought is maybe we should move ~/.xsession-errors to the local disk by default and make it always be a symlink, so users don't run into this problem in the first place.

Also, the whole thing should probably be moved to g_file_query_info, g_file_move, etc.

Neither of those things need to happen now, though. Martin, feel free to commit.

(So if it isn't clear I wrote comment 9 before reading comment 8)

Andras, when do you start svlogd during the login process?  Can you give a few more details about your setup?

svlogd is started independently of the login process (in this specific case, by runit(8) by way of runsv(8), during boot).

TBH, I don't see what there is to "support" about a FIFO - just leave it alone (refrain from renaming it), assuming the user knew why he/she put it there. Surely that's not difficult?

So a more robust approach to your setup might be to drop a file in /etc/X11/xinit/xinitrc.d (which is sourced by the Xsession script before execing your session) that redirects stderr and stdout to your service.

Anyway, it sounds like this isn't just a hypothetical "what if" use case, you already do it, and given we're all on the fence about it, supporting it doesn't seem to wrong.

The problem with fiddling with /etc/X11 is that that affects all users, and I wanted this for only one specific user (but I also wanted to log messages generated by /etc/X11/xinit/*, so doing the redirection from ~/.xsession oslt wouldn't have been workable).

Of course, a sufficiently convoluted /etc/X11/xinit* script can figure out if the current user's error logs should go into a FIFO or not, but I think this is really something we should be able to leave up to the users themselves; giving them the option of creating their own FIFO is by far the most straightforward option.

I'm glad you agree supporting it isn't wrong. :)

Review of attachment 186717 [details] [review]:

::: daemon/gdm-session-worker.c
@@ +1630,3 @@
 
+        if (g_access (dir, R_OK | W_OK | X_OK) == 0 && g_access (filename, R_OK | W_OK) == 0 &&
+            !g_file_test (filename, G_FILE_TEST_IS_SYMLINK)) {

Martin, before you commit can you change this part to call a new function is_loggable_file (filename) instead of the g_access and g_file_test?  the function would be like Alexey's is_normal_file, but would check also allow fifos and would check the readability/writability so we don't have to call access() separately.

Comment on attachment 186717 [details] [review]
Allow .xsession-errors to be a symlink

@Ray: sounds good, avoids stat()ing three times. I initially did that to minimize the distro patch, but for the upstream commit I'll do what you suggested.

Ah, I think I'll need a separate stat() and g_access() after all, as from a mere stat it's not really trivial to see whether you can actually read/write the file (you'd need to compare owners, conditionally check the user/group/other permissions, group memberships, etc.). But it still looks cleaner with a separate function.

Why would you care if you can read it? (Just curious.)

Also, please don't reimplement vfs access controls in userspace - that's the kernel's job, and you can't guess (in the general case) what it will allow or not allow (e.g. file permissions may look good, but the fs may be readonly; or some MAC scheme may grant you write access even if the permissions seemingly don't).

That's a very good point.

We shouldn't be calling access() at all here, although we should potentially be logging why the rename fails when it fails.

I found another gotcha: If ~/.xsession-errors is a symlink to e. g. /dev/null, then 

  if (fd < 0 || !_fd_is_normal_file (fd)) {

would always cause a new .xsession-errors.XXXXXX to be created and used even if ~/.xsession-errors was perfectly writable. I fixed that case as well and committed

http://git.gnome.org/browse/gdm/commit/?id=178459c33a64170d9ebe83ebbaeabeb05574f169

which now works for FIFOs and symlinks, and a symlink to /dev/null also works.

Sorry, Ray, I only read your last comment after committing. Right now it opens the file as O_RDWR, but as it's only dup2()ed to stdout/stderr, I agree that O_WRONLY and consequently g_access (filename, W_OK) should be sufficient. (That's independent from the original reported bug, of course). I see no reason to not just do O_WRONLY.

Likewise, the g_access (dir, R_OK | W_OK | X_OK) (where dir == $HOME) is quite superfluous indeed. If your home dir isn't writable, then the rename will fail, but we'll have bigger problems at that point and we can't do any sensible per-user logging anyway, so we migh tjust as well ignore rename() failures.

Want me to clean that up as well?

let's just close this out.

We can do further clean ups later.