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

[Tomas Volf]

Hello Fabio,

first, let me thank you for the review, and apologize for somewhat late
response, sadly I have been busy.

On 2024-04-17 10:30:12 +0100, Fabio Natali wrote:
> 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.

Great! :)

>
> > `(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?

I think the current way is fine, since this is Guix specific code, so it does
not have to be extremely portable.  But that is just my opinion.  Would be nice
to know if it works on Hurd.

>
> > 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?

While POSIX does not seem to specify the permissions of the created file, the
Guile's manual is pretty clear regarding it:

     POSIX doesn’t specify the permissions mode of the file.  On GNU and
     most systems it’s ‘#o600’; an application can use ‘chmod’ to relax
     that if desired.

In my understanding that makes this usage safe.

>
> 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?

Yes, the clean up was the main motivator.  The script could *try* to clean up,
but even then it would leave garbage in the $HOME in situations like power
failure and kernel crashes.  So using /tmp seems like simple yet reliable
solution.

>
> > 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.

Have a nice day,
Tomas

--
There are only two hard things in Computer Science:
cache invalidation, naming things and off-by-one errors.

signature.asc
Description: PGP signature