GNU bug report logs - #68675
[PATCH] Support dhcpcd in dhcp-client-service-type

Previous Next

Full log

View this message in rfc822 format

From: Ludovic Courtès <ludo <at> gnu.org>
To: soeren <at> soeren-tempel.net
Cc: 68675 <at> debbugs.gnu.org
Subject: [bug#68675] [PATCH v2] services: dhcp: Support the dhcpcd implementation.
Date: Mon, 12 Feb 2024 22:41:42 +0100

soeren <at> soeren-tempel.net skribis:

> From: Sören Tempel <soeren <at> soeren-tempel.net>
>
> Prior to this commit, the isc-dhcp implementation was the only DHCP
> implementation supported by dhcp-client-shepherd-service. This is
> problematic as the ISC implementation has reached end-of-life in
> 2022(!). As a first step to migrate away from isc-dhcp, this commit
> adds support for dhcpcd to dhcp-client-shepherd-service. Currently,
> it has to be enabled explicitly via the package field of the
> dhcp-client-configuration. In the future, it is intended to become
> the default to migrate away from isc-dhcp.
>
> While at it, also remove isc-dhcp from %base-packages as it is no
> longer necessarily needed and it will be pulled in by the DHCP
> client service if required.
>
> See also: https://issues.guix.gnu.org/68619
>
> * gnu/services/networking.scm (dhcp-client-shepherd-service): Add
>   support for the dhcpcd client implementation.
> * gnu/services/networking.scm (dhcp-client-account-service): New
>   procedure.
> * gnu/services/networking.scm (dhcp-client-service-type): Add optional
>   account-service-type extensions (needed for dhcpcd).
> * gnu/system.scm (%base-packages-networking): Remove isc-dhcp from
>   %base-packages (will be pulled in by dhcp-client-shepherd-service).
>
> Signed-off-by: Sören Tempel <soeren <at> soeren-tempel.net>

Much welcome improvement!

Some comments:

> +     (let* ((package (dhcp-client-configuration-package config))
> +            (client-name (package-name package))
> +            (requirement (dhcp-client-configuration-shepherd-requirement config))
> +            (provision (dhcp-client-configuration-shepherd-provision config))
> +            (interfaces (dhcp-client-configuration-interfaces config)))

Instead of calling ‘package-name’, which would prevent the use of
something other than a <package> record (such as an <inferior-package>)
or a package with a different name (like “dhcpcd-next”), what about
checking in the gexp which of /bin/dhclient and /bin/dhcpcd exists?

>                (start #~(lambda _
> -                         (define dhclient
> -                           (string-append #$package "/sbin/dhclient"))
> +                         (use-modules (ice-9 popen)
> +                                      (ice-9 rdelim))

Instead of ‘use-modules’ within a function, which has ill-defined
semantics, add a ‘modules’ field to the ‘shepherd-service’ form.

> +                         ;; Returns the execution configuration for the DHCP client
> +                         ;; selected by the package field of dhcp-client-configuration.
> +                         ;; The configuration is a pair of pidfile and execution command
> +                         ;; where the latter is a list.
> +                         (define exec-config
> +                           (case (string->symbol #$client-name)
> +                             ((isc-dhcp)
> +                              (let ((pid-file "/var/run/dhclient.pid"))
> +                                (cons
> +                                  (cons* (string-append #$package "/sbin/dhclient")
> +                                         "-nw" "-I" "-pf" pid-file ifaces)
> +                                  pid-file)))
> +                             ((dhcpcd)
> +                              ;; For dhcpcd, the utilized pid-file depends on the
> +                              ;; command-line arguments.  If multiple interfaces are
> +                              ;; given, a different pid-file is returned.  Hence, we
> +                              ;; consult dhcpcd itself to determine the pid-file.
> +                              (let* ((cmd (string-append #$package "/sbin/dhcpcd"))
> +                                     (arg (cons* cmd "-b" ifaces)))
> +                                (cons arg
> +                                  (let* ((pipe (string-join (append arg '("-P")) " "))
> +                                         (port (open-input-pipe pipe))
> +                                         (path (read-line port)))
> +                                    (close-pipe port)
> +                                    path))))

That sounds quite complex.  Surely there must be a way to force the name
of the PID file or to determine its name without running dhcpcd in a
pipe?

> +                             (else
> +                               (error (G_ "unknown 'package' value in dhcp-client-configuration")))))

‘G_’ here is undefined, a ‘error’ doesn’t do what you perhaps think it
does (it throws to ‘misc-error’).

Instead, I would just print a message to the current error port (it’ll
be logged) and have ‘start’ return #f, meaning that the service failed
to start.
> +++ b/gnu/system.scm
> @@ -917,7 +917,7 @@ (define %base-packages-interactive
>  
>  (define %base-packages-networking
>    ;; Default set of networking packages.
> -  (list inetutils isc-dhcp
> +  (list inetutils

I would leave this change for a separate patch.

Or leave it out entirely: I find it useful to have a DHCP client at hand
just in case.  Thoughts?

Could you send updated patches?

Thanks,
Ludo’.

Previous Next

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