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

  • Open
  • quality assurance status badge
Details
2 participants
  • Fabio Natali
  • Tomas Volf
Owner
unassigned
Submitted by
Tomas Volf
Severity
normal
T
T
Tomas Volf wrote on 6 Jan 16:07 +0100
(address . guix-patches@gnu.org)(name . Tomas Volf)(address . ~@wolfsden.cz)
4fdf0d9993bb3375797ca807d894f66920bd81d2.1704553618.git.~@wolfsden.cz
When user does not use any desktop environment, the typical sequence is to log
in and then type `startx' into the tty to get a window manager running. Most
distributions do provide startx by default, but Guix has only
xorg-start-command, that is not suitable for this type of task.

This commit adds second procedure, xorg-start-command-xinit, that correctly
picks virtual terminal to use, sets up XAUTHORITY and starts xinit with
correct arguments. That should make running Guix without any desktop
environment more approachable.

* gnu/services/xorg.scm (xorg-start-command-xinit): New procedure.
(define-module): Export it.
* doc/guix.texi (X Window): Document it.

Change-Id: I17cb16093d16a5c6550b1766754700d4fe014ae9
---
doc/guix.texi | 18 ++++++++++
gnu/services/xorg.scm | 82 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 100 insertions(+)

Toggle diff (140 lines)
diff --git a/doc/guix.texi b/doc/guix.texi
index a648a106b3..72c5527270 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -23177,6 +23177,24 @@ X Window
Usually the X server is started by a login manager.
@end deffn
+@deffn {Procedure} xorg-start-command-xinit [config]
+Return a @code{startx} script in which the modules, fonts,
+etc. specified in @var{config}, are available. The result should be
+used in place of @code{startx}. Compared to the
+@code{xorg-start-command} it calls xinit, therefore it works well when
+executed from tty. If you are using a desktop environment, you are
+unlikely to have a need for this procedure.
+
+The resulting file should be invoked by user from the tty after login,
+common name for the program would be @code{startx}. Convenience link
+can be created by (for example) this home service:
+
+@lisp
+(simple-service 'home-files home-files-service-type
+ `(("bin/startx" ,(xorg-start-command-xinit))))
+@end lisp
+@end deffn
+
@defvar screen-locker-service-type
Type for a service that adds a package for a screen locker or screen
diff --git a/gnu/services/xorg.scm b/gnu/services/xorg.scm
index 1ee15ea90c..2f5aa3b4f3 100644
--- a/gnu/services/xorg.scm
+++ b/gnu/services/xorg.scm
@@ -53,6 +53,7 @@ (define-module (gnu services xorg)
#:use-module (gnu packages gnome)
#:use-module (gnu packages admin)
#:use-module (gnu packages bash)
+ #:use-module (gnu packages linux)
#:use-module (gnu system shadow)
#:use-module (guix build-system glib-or-gtk)
#:use-module (guix build-system trivial)
@@ -84,6 +85,7 @@ (define-module (gnu services xorg)
xorg-wrapper
xorg-start-command
+ xorg-start-command-xinit
xinitrc
xorg-server-service-type
@@ -414,6 +416,86 @@ (define* (xorg-start-command #:optional (config (xorg-configuration)))
(program-file "startx" exp))
+(define* (xorg-start-command-xinit #:optional (config (xorg-configuration)))
+ "Return a @code{startx} script in which the modules, fonts, etc. specified in
+@var{config}, are available. The result should be used in place of
+@code{startx}. Compared to the @code{xorg-start-command} it calls xinit,
+therefore it works well when executed from tty."
+ (define X
+ (xorg-wrapper config))
+
+ (define exp
+ ;; Small wrapper providing subset of functionality of typical startx script
+ ;; from distributions like alpine.
+ #~(begin
+ (use-modules (ice-9 popen)
+ (ice-9 textual-ports))
+
+ (define (checked-system* . args)
+ (if (= 0 (status:exit-val (apply system* args)))
+ #t
+ (error "command failed")))
+
+ (define (capture-stdout . prog+args)
+ (let* ((port (apply open-pipe* OPEN_READ prog+args))
+ (data (get-string-all port)))
+ (if (= 0 (status:exit-val (close-pipe port)))
+ (string-trim-right data #\newline)
+ (error "command failed"))))
+
+ (define (determine-unused-display n)
+ (let ((lock-file (format #f "/tmp/.X~a-lock" n))
+ (sock-file (format #f "/tmp/.X11-unix/X~a" n)))
+ (if (or (file-exists? lock-file)
+ (false-if-exception
+ (eq? 'socket (stat:type (stat sock-file)))))
+ (determine-unused-display (+ n 1))
+ (format #f ":~a" n))))
+ (define (determine-vty)
+ (let ((fd0 (readlink "/proc/self/fd/0"))
+ (pref "/dev/tty"))
+ (if (string-prefix? pref fd0)
+ (string-append "vt" (substring fd0 (string-length pref)))
+ (error (format #f "Cannot determine VT from: ~a" fd0)))))
+
+ (define (enable-xauth server-auth-file display)
+ ;; Configure and enable X authority
+ (or (getenv "XAUTHORITY")
+ (setenv "XAUTHORITY" (string-append (getenv "HOME") "/.Xauthority")))
+
+ (let* ((bin/xauth (string-append #$xauth "/bin/xauth"))
+ (bin/mcookie (string-append #$util-linux "/bin/mcookie"))
+
+ (mcookie (capture-stdout bin/mcookie)))
+ (checked-system* bin/xauth "-qf" server-auth-file
+ "add" display "." mcookie)
+ (checked-system* bin/xauth "-q"
+ "add" display "." mcookie)))
+
+ (let* ((xinit (string-append #$xinit "/bin/xinit"))
+ (display (determine-unused-display 0))
+ (vty (determine-vty))
+ (server-auth-port (mkstemp "/tmp/serverauth.XXXXXX"))
+ (server-auth-file (port-filename server-auth-port)))
+ (close-port server-auth-port)
+ (enable-xauth server-auth-file display)
+ (apply execl
+ xinit
+ xinit
+ "--"
+ #$X
+ display
+ vty
+ "-keeptty"
+ "-auth" server-auth-file
+ ;; These are set by xorg-start-command, so do the same to keep
+ ;; it consistent.
+ "-logverbose" "-verbose" "-terminate"
+ #$@(xorg-configuration-server-arguments config)
+ (cdr (command-line))))))
+
+ (program-file "startx" exp))
+
(define* (xinitrc #:key fallback-session)
"Return a system-wide xinitrc script that starts the specified X session,
which should be passed to this script as the first argument. If not, the

base-commit: e994bc0abf39db228fa61f1aaf24840c19c47647
--
2.41.0
F
F
Fabio Natali wrote 6 days ago
87o7a9upoq.fsf@fabionatali.com
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.



`(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:
| 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
F
F
Fabio Natali wrote 6 days ago
87r0f4l4kb.fsf@fabionatali.com
Hi, a quick follow-up on a couple of points.

On 2024-04-16, 19:29 +0100, Fabio Natali <me@fabionatali.com> wrote:
Toggle quote (3 lines)
> - 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.

Toggle quote (5 lines)
> `(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?

Toggle quote (5 lines)
> 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?

Toggle quote (4 lines)
> 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.
F
F
Fabio Natali wrote 4 days ago
87jzkufr4y.fsf@fabionatali.com
On 2024-04-17, 10:30 +0100, Fabio Natali <me@fabionatali.com> wrote:
Toggle quote (2 lines)
> Hi, a quick follow-up on a couple of points.

Also, I suppose one could use Guix's 'invoke' instead of a custom
'checked-system*'?


Cheers, F.
T
T
Tomas Volf wrote 4 days ago
(name . Fabio Natali)(address . me@fabionatali.com)(address . 68289@debbugs.gnu.org)
ZiGMJGQfxqSvXyjF@ws
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:
Toggle quote (8 lines)
> 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! :)

Toggle quote (11 lines)
>
> > `(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.

Toggle quote (6 lines)
>
> > 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.

Toggle quote (7 lines)
>
> 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.

Toggle quote (17 lines)
>
> > 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.
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEt4NJs4wUfTYpiGikL7/ufbZ/wakFAmYhjCQACgkQL7/ufbZ/
wancOBAAhw1uq2DKbubetW4Pamy1U/lc+2JgOsUZtNaKMgor7/LTTZEJWAj7Foik
N8I3FvXpyUX0o0uGKxlAMLHSGRSI1CAERzbAFfdWox8anh+/LgxXd4R+L2nOf84v
MJf/h0FqoDc16Bl7U7yfKVob6JS8WiR+p6NM38YwaVMAbdtsKMZwmyIAAQ2UDdti
sEHgBlReq591XFX8sRmvA6Sbl+UNMoc2i6QH1FAn7jc4hyyjxfcIiGETt9PLsCMz
K4yhGXwCmhbpPcVozW6qZW272vmY1q7aiqpCpHymQeZMAhO9I0XVnAFcmksPlf7j
JcKLuSgHg+GJeXMF9iucs3V53K9WcTydMBAKqaYqTAvRhcudTNAl3BdGl4/L2DyM
Mok6Dms4JTalVBk88Dd2yvGBae+INeBj3ErXPRnlZccStLCScvCOtkC7BZ/dQTRL
zGUfUgTMRxXLGPrWVVaO/yAU9IkHRLti9ww6HhOfm5MQRK1DzE+kWt4cAUDgDGVY
hW2Sggg9UkK9oCb3w1+Xf387mvrF3WcJqtqSrSmEDhyCklIMOQ+GpRzZuHDsLE33
pVatUu+tS2ae2oGnayDxi/+54LGVSN/i5AF8R7m6ZTFFqTpD5NjhQRVb20R1pZ17
jUwrjzDtaE3JKJHH6djbZnGRUKjlQVBAd5rPvb99d2xRkaevmIU=
=XkJ6
-----END PGP SIGNATURE-----


T
T
Tomas Volf wrote 4 days ago
(name . Fabio Natali)(address . me@fabionatali.com)(address . 68289@debbugs.gnu.org)
ZiGN9OhfIYS2I_EG@ws
On 2024-04-18 19:43:41 +0100, Fabio Natali wrote:
Toggle quote (10 lines)
> On 2024-04-17, 10:30 +0100, Fabio Natali <me@fabionatali.com> wrote:
> > Hi, a quick follow-up on a couple of points.
>
> Also, I suppose one could use Guix's 'invoke' instead of a custom
> 'checked-system*'?
>
> https://issues.guix.gnu.org/issue/68289/#0-lineno88
>
> Cheers, F.

Yes, that would be an option. I do not remember why I wrote it like this, it is
possible I just did not know about `invoke' at that time (the code is over a
year old). However I am not sure whether it is fine to depend on code in the
(guix build utils) module for non-build purposes. Assuming it is fine, I have
no problem sending a v2.

Cheers,
T.

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

iQIzBAEBCgAdFiEEt4NJs4wUfTYpiGikL7/ufbZ/wakFAmYhjfQACgkQL7/ufbZ/
wameDw/+JTVUtT2IvGpB7d69FShKy9nZhqn/gWQMaisBoUlZVzQmdoEICDWGept/
ZJncP4QInV0TVywiegQehWhkz+EAHaHYWzwBpU9CSm1s7v6/En73TlOC9RHxMK4A
wOf755gwiFGZkuXEm8ywYMj3BKIBza41pHgf51THZ0SjTjRqEexxwYhmSDE9gcqo
9oib1kGNREo27Dlu9P4JF6XQ3NcOFWoO5+u+8BoqR5CbOLs7JjYZmdGvfSxWkmhi
1/Go8xiTpiawHtP4bYrkM3fGBcGiaqk1Y9YHonEN07ZjcXiEfEIYlb7t2dAKyb+v
YLiRt0WnwZ5GQLKe8ZNKVk4ZUDATT+tMWwG0A57/Vo9ryQBjo82oc11zUa/SbgWy
xbZngrBU6LuJ4D+ucAiLZ8MafmzTEDePujj/YKfJSyEQiDTgLXGTCAvsJ/96VNYQ
LIbHD81Wq6Oh0tMT+SYpbc0orsl+CF/YnMyJ6atOGiWEIqyAXYP/AmjddIIBn4bh
PcOwkcy0JnTG+bxS5B2IR1tks4TVtheiDtuqYMZIlFUXgYOJyk3YyXbc1qxoVJQr
UsG76TZg0ZMAXV+G6Q36qP5vtddAHNwGXsBroB2WY8SKUnNmRqBlaD62/rTnBNAG
POOIBN3d6UJ4P6I3hzGGkKSnG2JfDNva7JO9r0kFRoG9TclmURU=
=0giq
-----END PGP SIGNATURE-----


F
F
Fabio Natali wrote 4 days ago
(name . Tomas Volf)(address . ~@wolfsden.cz)(address . 68289@debbugs.gnu.org)
87zftpedyt.fsf@fabionatali.com
On 2024-04-18, 23:09 +0200, Tomas Volf <~@wolfsden.cz> wrote:
Toggle quote (3 lines)
> first, let me thank you for the review, and apologize for somewhat
> late response, sadly I have been busy.

Contrarily, thank you for getting back to my points. ?

It all sounds good. I'll try and bring this up on IRC or during one of
the patch review sessions organised by Futurile - in case there's a
committer who's willing to merge it.

Have a nice day.

Best, Fabio.


--
Fabio Natali
?