Subject: [bug#75934] [PATCH] services: networking: Add dhcpcd service.

From: Ludovic Courtès <[email-masked]>
Date: Wed, 05 Mar 2025 19:29:00 +0100

Hi,

[email-masked] skribis:

> From: Sören Tempel <[email-masked]>
>
> This is intended as an alternative to dhcp-client-service-type as
> isc-dhcp has reached its end-of-life in 2022 (three years ago!),
> see #68619 for more details.  Long-term, this services is therefore
> intended to replace dhcp-client-service-type.
>
> * gnu/services/networking.scm (dhcpcd-service-type): New service.
> (dhcpcd-shepherd-service): New procedure.
> (dhcpcd-account-service): New variable.
> (dhcpcd-config-file): New procedure.
> (dhcpcd-configuration): New record type.
> (dhcpcd-serialize-list-of-strings, dhcpcd-serialize-boolean)
> (dhcpcd-serialize-string): New procedures.
> * gnu/tests/networking.scm (run-dhcpcd-test): New procedure.
> (%dhcpcd-os, %test-dhcpcd): New variables.
> * doc/guix.texi (Networking Services): Document it.
> ---
> Previously, an integration into the dhcp-client-service-type was
> attempted.  However, the discussion there established that a new
> entirely separate service would be a better fit.
>
> See https://issues.guix.gnu.org/68675 for the prior discussion.

Nice, thanks for working on it.

Overall LGTM.  Some rather minor suggestions below:

> +@cindex DHCPCD, networking service
> +
> +@defvar dhcpcd-service-type
> +This is a service which runs @var{dhcpcd}, an alternative Dynamic
> +Host Configuration Protocol (DHCP) client.
> +@end defvar

What about something like this, to provide more context:

  This the type for a service running @command{dhcpcd}, a @acronym{DHCP,
  Dynamic Host Configuration Protocol} client that can be used as a
  replacement for the historical ISC client supported by
  @code{dhcp-client-service-type}.

  Its value must be a @code{dhcpcd-configuration} record, as described
  below.

> +(define (dhcpcd-serialize-string field-name value)
> +  (let ((field (object->string field-name)))
> +    (if (string=? field "extra-content")
> +      #~(string-append #$value "\n")
> +      #~(format #f "~a ~a~%" #$field #$value))))

Please indent ‘if’ expressions like this:

  (if condition
      then
      else)

> +(define-configuration dhcpcd-configuration
> +  (interfaces
> +    (list '())
> +    "List of interfaces to start a DHCP client for."

“List of networking interfaces---e.g., @code{\"eth0\"}---to start …”

Perhaps also explain what happens when it’s the empty list.

> +  (command-args

s/command-args/command-arguments/

> +    (list '("-q" "-q"))

Twice?

> +  (hostname
> +    (maybe-string "")
> +    "Hostname to send via DHCP, defaults to the current system hostname.")

Rather ‘host-name’ and “Host name”.  :-)

> +  (duid
> +    (maybe-string "")
> +    "Use and generate a DHCP Unique Identifier.")

This isn’t clear to me; make sure to describe what the value represents.

> +  (persistent
> +    (boolean #t)
> +    "Do not de-configure on shutdown.")

Rather ‘persistent?’ (question mark), and perhaps something like:

  “When true, preserve configuration on disk when shutting down and
  reuse it when restarting.”

> +  ;; Common options not set in the default configuration file.
> +  (nooption
> +    (list-of-strings '())
> +    "List of options to remove from the message before it's processed.")
> +  (nohook
> +    (list-of-strings '())
> +    "List of hook script which should not be invoked.")

‘excluded-options’, ‘excluded-hooks’ maybe?

> +  (static
> +    (list-of-strings '())
> +    "Configure a static value (e.g. ip_address).")

‘static-values’?  An example would be welcome.

> +  (vendorclassid
> +    maybe-string
> +    "Set the DHCP Vendor Class.")
> +  (clientid
> +    maybe-string
> +    "Use the interface hardware address or the given string as a Client ID.")

‘vendor-class-id’, ‘client-id’, but again with examples probably.

> +            (actions (list (shepherd-configuration-action config-file)))
> +            (start
> +              #~(lambda _
> +                  (fork+exec-command
> +                    (list (string-append #$dhcpcd "/sbin/dhcpcd")
> +                          #$@command-args "-B" "-f" #$config-file #$@ifaces))))

Rather:

  (start #~(make-forkexec-constructor (list …)))

The system test is pretty nice!

Could you send an updated version?

Thanks,
Ludo’.