[bug#49578] [PATCH] Add bolt

["Ludovic Courtès" <ludo@gnu.org>]

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