[bug#68289] [PATCH] services: xorg: Add xorg-start-command-xinit procedure.

[Fabio Natali]

Hi, a quick follow-up on a couple of points.

On 2024-04-16, 19:29 +0100, Fabio Natali <me@fabionatali.com> wrote:
> - I haven't tested the patch on my system yet, but I plan to do it
> soon.

I've tested the patch and it works as expected on my system.

> `(determine-vty)' is similar to the block below, but `startx' relies
> on the `tty' command from Coreutils. Do you think there might be any
> advantage in using it in `(determine-vty)'? A slight simplification
> perhaps?

Looking into this more closely, the `tty' command wouldn't be a
simplification. It might be a bit more consistent with other parts of
the patch and it'd abstract away the hardcoded `/proc/self/fd/0', but
probably not worth the change?

> The patch saves the server's auth file in `/tmp' whereas `startx' uses
> the home directory. I wonder if this might make any difference in
> terms of security. Related, how can we be sure that `(mkstemp
> "/tmp/serverauth.XXXXXX")' will be setting the right file permissions?

I see the reason why we want to use `/tmp', as otherwise the number of
stale `serverauth.XXXXXX' files would grow indefinitely. Using `/tmp',
at least we know they'll be garbage collected at every reboot. Any way
to emulate `startx' and use some sort of `trap' to remove the file on
exit?

> Finally, on a purely cosmetic side, any reason to have `(define X
> (xorg-wrapper config))' outside the G-expression, while the other
> definitions are inside?

Oh yes, the `(define X ...)' has to be outside the G-expression, of
course.

The security aspect (in relation to the server auth file, its
permissions and location) is the only remaining point where I'd like an
extra pair of eyes. The rest of the patch LGTM.

There's a couple of microscopic formatting issues (e.g. an occurrence of
tty where I'd write TTY instead), I'll list them all in a follow-up.

Thanks, best wishes, Fabio.