diff mbox series

[bug#49578,v4] gnu: Add bolt.

Message ID rp8SpAlHZIINbm9ZNO_4lI_mK0OlLOQM9EdTQt8Zzz_9c5mpXm_xvGC8gpNjRMDew1MOpECQp6y5MpJWgEk7buLEMmBQleiH1NycK7uKAys=@protonmail.com
State New
Headers show
Series [bug#49578,v4] gnu: Add bolt. | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git-branch success View Git branch
cbaines/applying patch success View Laminar job
cbaines/issue success View issue

Commit Message

phodina Aug. 14, 2022, 10 p.m. UTC 
Hi Sarah,

here's updated patch with your tips.

David, the way I test new packages is either by running:
[1]
guix shell --check --pure -D guix

or 
[2]
guix build -L <load_path>


[1] https://guix.gnu.org/manual/en/html_node/Invoking-guix-environment.html
[2] https://guix.gnu.org/manual/en/html_node/Common-Build-Options.html

----
Petr

Comments

Ludovic Courtès Dec. 14, 2022, 11:22 a.m. UTC | #1 
Hi,

Thanks a lot, Sarah, for the review work.  Somehow it eventually fell
through the cracks but it’s never too late.  :-)

phodina <phodina@protonmail.com> skribis:

> From 538eeedf7ee64f98b17507ea11d38512525ef29f Mon Sep 17 00:00:00 2001
> From: Petr Hodina <phodina@protonmail.com>
> Date: Sun, 18 Jul 2021 12:11:55 +0200
> Subject: [PATCH v4 1/2] gnu: Add bolt.
>
> * gnu/packages/linux.scm (bolt): New variable.

LGTM!  I went ahead and applied it.

> From e054c89f9964686670e7716c820ca9ebb9f41543 Mon Sep 17 00:00:00 2001
> From: Petr Hodina <phodina@protonmail.com>
> Date: Sat, 18 Sep 2021 13:11:18 +0200
> Subject: [PATCH v4 2/2] services: Add a service for bolt.
>
> * gnu/services/linux.scm (bolt-service-type)
> (bolt-shepherd-service, bolt-dbus-service)
> (bolt-configuration, bolt-configuration?): New procedures.

[...]

> +(define-record-type* <bolt-configuration>
> +  bolt-configuration make-bolt-configuration bolt-configuration?
> +  (package bolt-configuration-package ; package
> +           (default bolt)))
> +
> +(define bolt-shepherd-service
> +  (match-lambda
> +    (($ <bolt-configuration> package)

Just call ‘bolt-configuration-package’ instead of using ‘match’ here.

> +     (with-imported-modules (source-module-closure
> +                             '((gnu build shepherd)))
> +       (shepherd-service
> +        (documentation "Thunderbolt daemon")
> +        (provision '(thunderbolt))
> +        (requirement '(networking))
> +        (modules '((gnu build shepherd)))
> +        (start #~(make-forkexec-constructor/container
> +                  (list #$(file-append package "/libexec/boltd"))
> +		  ))

Please move the parens to the previous line.  :-)

> +        (stop #~(make-kill-destructor)))))))
> +
> +(define %bolt-activation
> +  #~(begin
> +      (use-modules (guix build utils))
> +      (mkdir-p "/var/lib/boltd")))
> +
> +(define (bolt-dbus-service config)
> +  (list (wrapped-dbus-service (bolt-configuration-bolt config)
> +			      "libexec/boltd"
> +			      `(("BOLT_CONF_FILE_NAME"
> +				 '("share/dbus-1/interfaces/org.freedesktop.bolt.xml"))))))
> +
> +(define %bolt-accounts
> + (list (user-group (name "boltd") (system? #t))
> +       (user-account
> +	 (name "boltd")
> +	 (group "boltd")
> +	 (system? #t)
> +	 (comment "Boltd daemon user")
> +	 (home-directory "/var/empty")
> +	 (shell "/run/current-system/profile/sbin/nologin"))))

This is creating accounts, but ‘boltd’ is started as root.  Is that
expected?  Does ‘boltd’ setuids by itself?  If so, please add a comment
above the ‘make-forkexec-constructor’ call.

> +(define bolt-udev-rule
> +  (match-lambda
> +    (($ <bolt-configuration> package)

Same comment as above.

> +(define bolt-service-type
> +  (service-type
> +   (name 'boltd)
> +   (description
> +    "Thunderbolt daemon")

Please write full sentences here, giving enough context to make ‘guix
system search’ useful.

> +   (extensions
> +    (list (service-extension udev-service-type
> +			     (compose list bolt-udev-rule))
> +	  (service-extension activation-service-type
> +			     (const %bolt-activation))
> +	  (service-extension dbus-root-service-type
> +	  (compose list bolt-configuration-package))
> +	;		     bolt-dbus-service)

Maybe remove this line?

One last thing: please document it in ‘doc/guix.texi’.  In particular,
make sure to add a paragraph that gives a bit of context andq a
configuration example.

Could you send an updated patch?

Thanks,
Ludo’.
Ludovic Courtès Jan. 5, 2023, 9:28 p.m. UTC | #2 
Hi,

Just noticed an issue:

phodina <phodina@protonmail.com> skribis:

> +    (($ <bolt-configuration> package)
> +     (with-imported-modules (source-module-closure
> +                             '((gnu build shepherd)))
> +       (shepherd-service
> +        (documentation "Thunderbolt daemon")
> +        (provision '(thunderbolt))
> +        (requirement '(networking))
> +        (modules '((gnu build shepherd)))
> +        (start #~(make-forkexec-constructor/container
> +                  (list #$(file-append package "/libexec/boltd"))

‘make-forkexec-constructor/container’ is deprecated in favor of the
facilities in (guix least-authority).

However, running boltd in a container might prevent it from accessing
useful files or devices under /dev, /sys, or whatever.  I recommend
checking whether boltd can actually run in a container; you can strace
it to get an idea of the files it needs to access.  It needs to run as
root anyway so perhaps running it in a container is not that important.

Ludo’.
diff mbox series

Patch

From e054c89f9964686670e7716c820ca9ebb9f41543 Mon Sep 17 00:00:00 2001
From: Petr Hodina <phodina@protonmail.com>
Date: Sat, 18 Sep 2021 13:11:18 +0200
Subject: [PATCH v4 2/2] services: Add a service for bolt.

* gnu/services/linux.scm (bolt-service-type)
(bolt-shepherd-service, bolt-dbus-service)
(bolt-configuration, bolt-configuration?): New procedures.

diff --git a/gnu/services/linux.scm b/gnu/services/linux.scm
index 60e2093e1d..37dcd14f97 100644
--- a/gnu/services/linux.scm
+++ b/gnu/services/linux.scm
@@ -5,6 +5,7 @@ 
 ;;; Copyright © 2021 raid5atemyhomework <raid5atemyhomework@protonmail.com>
 ;;; Copyright © 2021 B. Wilson <elaexuotee@wilsonb.com>
 ;;; Copyright © 2022 Josselin Poiret <dev@jpoiret.xyz>
+;;; Copyright © 2021-2022 Petr Hodina <phodina@protonmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -29,15 +30,21 @@  (define-module (gnu services linux)
   #:use-module (guix i18n)
   #:use-module (guix ui)
   #:use-module (gnu services)
+  #:use-module (gnu services dbus)
   #:use-module (gnu services base)
   #:use-module (gnu services shepherd)
+  #:use-module (gnu system shadow)
   #:use-module (gnu packages linux)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-26)
   #:use-module (srfi srfi-34)
   #:use-module (srfi srfi-35)
   #:use-module (ice-9 match)
-  #:export (earlyoom-configuration
+  #:export (bolt-configuration
+            bolt-configuration?
+            bolt-service-type
+
+            earlyoom-configuration
             earlyoom-configuration?
             earlyoom-configuration-earlyoom
             earlyoom-configuration-minimum-available-memory
@@ -65,6 +72,76 @@  (define-module (gnu services linux)
             zram-device-configuration-priority
             zram-device-service-type))
 
+
+;;;
+;;; Thunderbolt daemon.
+;;;
+
+(define-record-type* <bolt-configuration>
+  bolt-configuration make-bolt-configuration bolt-configuration?
+  (package bolt-configuration-package ; package
+           (default bolt)))
+
+(define bolt-shepherd-service
+  (match-lambda
+    (($ <bolt-configuration> package)
+     (with-imported-modules (source-module-closure
+                             '((gnu build shepherd)))
+       (shepherd-service
+        (documentation "Thunderbolt daemon")
+        (provision '(thunderbolt))
+        (requirement '(networking))
+        (modules '((gnu build shepherd)))
+        (start #~(make-forkexec-constructor/container
+                  (list #$(file-append package "/libexec/boltd"))
+		  ))
+        (stop #~(make-kill-destructor)))))))
+
+(define %bolt-activation
+  #~(begin
+      (use-modules (guix build utils))
+      (mkdir-p "/var/lib/boltd")))
+
+(define (bolt-dbus-service config)
+  (list (wrapped-dbus-service (bolt-configuration-bolt config)
+			      "libexec/boltd"
+			      `(("BOLT_CONF_FILE_NAME"
+				 '("share/dbus-1/interfaces/org.freedesktop.bolt.xml"))))))
+
+(define %bolt-accounts
+ (list (user-group (name "boltd") (system? #t))
+       (user-account
+	 (name "boltd")
+	 (group "boltd")
+	 (system? #t)
+	 (comment "Boltd daemon user")
+	 (home-directory "/var/empty")
+	 (shell "/run/current-system/profile/sbin/nologin"))))
+
+(define bolt-udev-rule
+  (match-lambda
+    (($ <bolt-configuration> package)
+  (file->udev-rule "90-bolt.rules" (file-append package "/lib/udev/rules.d/90-bolt.rules")))))
+
+(define bolt-service-type
+  (service-type
+   (name 'boltd)
+   (description
+    "Thunderbolt daemon")
+   (extensions
+    (list (service-extension udev-service-type
+			     (compose list bolt-udev-rule))
+	  (service-extension activation-service-type
+			     (const %bolt-activation))
+	  (service-extension dbus-root-service-type
+	  (compose list bolt-configuration-package))
+	;		     bolt-dbus-service)
+	  (service-extension account-service-type
+			     (const %bolt-accounts))
+          (service-extension shepherd-root-service-type
+                             (compose list bolt-shepherd-service))))
+   (default-value (bolt-configuration))))
+
 
 ;;;
 ;;; Early OOM daemon.
-- 
2.37.0