GNU bug report logs - #75934
[PATCH] services: networking: Add dhcpcd service.

Previous Next

Full log

Message #8 received at 75934 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: soeren <at> soeren-tempel.net
Cc: Quelln <at> protonmail.com, 873216071 <at> qq.com, 75934 <at> debbugs.gnu.org
Subject: Re: [bug#75934] [PATCH] services: networking: Add dhcpcd service.
Date: Wed, 05 Mar 2025 19:29:00 +0100

Hi,

soeren <at> soeren-tempel.net skribis:

> From: Sören Tempel <soeren <at> soeren-tempel.net>
>
> 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’.

Previous Next

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