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

Previous Next

Full log

View this message in rfc822 format

From: Fabio Natali <me <at> fabionatali.com>
To: 68289 <at> debbugs.gnu.org, ~@wolfsden.cz
Subject: [bug#68289] [PATCH] services: xorg: Add xorg-start-command-xinit procedure.
Date: Tue, 16 Apr 2024 19:29:09 +0100

Hi Tomas,

Thanks for patch 68289 re `xorg-start-command-xinit'. I think it'd be
great to have a command like that in Guix.

In a clumsy attempt to review the patch, I've compared it with the code
for `startx' that I found here⁰. My comments, including some general
observations that might help other reviewers, follow.

tl;dr:

- I hope someone more Xorg savvy than me can have a look.
- Other than a couple of questions (below), things look alright to me.
- I haven't tested the patch on my system yet, but I plan to do it soon.

Thanks, have a great day, Fabio.

⁰ https://gitlab.freedesktop.org/xorg/app/xinit/-/blob/master/startx.cpp


`(determine-unused-display n)' maps closely to this code block:

,----
| XCOMM Automatically determine an unused $DISPLAY
| d=0
| while true ; do
|     [ -e "/tmp/.X$d-lock" -o -S "/tmp/.X11-unix/X$d" ] || break
|     d=$(($d + 1))
| done
| defaultdisplay=":$d"
| unset d
`----

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

,----
| #ifdef __linux__
|     XCOMM When starting the defaultserver start X on the current tty to avoid
|     XCOMM the startx session being seen as inactive:
|     XCOMM "https://bugzilla.redhat.com/show_bug.cgi?id=806491"
|     tty=$(tty)
|     if expr "$tty" : '/dev/tty[0-9][0-9]*$' > /dev/null; then
|         tty_num=$(echo "$tty" | grep -oE '[0-9]+$')
|         vtarg="vt$tty_num -keeptty"
|     fi
| #endif
`----

`(enable-xauth server-auth-file display)' maps closely to:

,----
|     XCOMM create a file with auth information for the server. ':0' is a dummy.
|     xserverauthfile=$HOME/.serverauth.$$
|     trap "rm -f '$xserverauthfile'" HUP INT QUIT ILL TRAP KILL BUS TERM
|     xauth -q -f "$xserverauthfile" << EOF
| add :$dummy . $mcookie
| EOF
| #if defined(__APPLE__) || defined(__CYGWIN__)
|     xserverauthfilequoted=$(echo ${xserverauthfile} | sed "s/'/'\\\\''/g")
|     serverargs=${serverargs}" -auth '"${xserverauthfilequoted}"'"
| #else
|     serverargs=${serverargs}" -auth "${xserverauthfile}
| #endif
|
|     XCOMM now add the same credentials to the client authority file
|     XCOMM if '$displayname' already exists do not overwrite it as another
|     XCOMM server may need it. Add them to the '$xserverauthfile' instead.
|     for displayname in $authdisplay $hostname$authdisplay; do
|         authcookie=`XAUTH list "$displayname" @@
|         | sed -n "s/.*$displayname[[:space:]*].*[[:space:]*]//p"` 2>/dev/null;
|         if [ "z${authcookie}" = "z" ] ; then
|             XAUTH -q << EOF
| add $displayname . $mcookie
| EOF
`----

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?

Here's the two relevant bits:

,----
| (server-auth-port (mkstemp "/tmp/serverauth.XXXXXX"))
| (server-auth-file (port-filename server-auth-port))
`----

,----
|     xserverauthfile=$HOME/.serverauth.$$
|     trap "rm -f '$xserverauthfile'" HUP INT QUIT ILL TRAP KILL BUS TERM
`----

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?


-- 
Fabio Natali
https://fabionatali.com

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.