Message ID | 5aff02159575834de675684dfde71d2ec66f4b10.1706123111.git.soeren@soeren-tempel.net |
---|---|
State | New |
Headers | show |
Series | [bug#68675,v2] services: dhcp: Support the dhcpcd implementation. | expand |
soeren@soeren-tempel.net skribis: > From: Sören Tempel <soeren@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@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’.
Hi Ludovic, Thanks a lot for the feedback, really truly helpful! More comments below. Ludovic Courtès <ludo@gnu.org> wrote: > 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? I think I haven't fully grasped G-Expressions yet. I do understand how this would work inside the shepherd start gexp. However, I am not sure how I would check if /bin/dhclient or /bin/dhcpcd exist in the package within the dhcp-client-account-service since, at least as far as I understand, the dhcp-client-account-service is itself not a gexp. I think one could also argue that it might be preferable to check for the package name as, just because a package provides /bin/dhclient, doesn't necessarily mean that it is command-line compatible with the ISC implementation. However, if you have any hints on updating the account-service I would be willing to look into it. > 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? Unfortunately, I don't think there is any way to force the PID file name. Furthermore, the pidfile path depends on the amount interfaces specified. If there is only one interface, a single dhcpcd instance is spawned and the pidfile refers to that. Otherwise, a management process is started and the pidfile refers to the management process [1]. I think the best solution here would be to spawn dhcpcd as child process of GNU shepherd and have shepherd supervise it that way. I have a service doing that too, but its annoying to integrate with the existing dhclient service [2]. Maybe that's something we can migrate to in the long run. > Or leave it out entirely: I find it useful to have a DHCP client at hand > just in case. Thoughts? I removed it, I agree that this change is best discussed separately. > Could you send updated patches? Apart from the two comments I discussed in greater detail above, I hope I implemented all of your feedback as requested in the v3 revision of this patchset. Please let me know if I missed anything. Greetings Sören [1]: https://github.com/NetworkConfiguration/dhcpcd/blob/v10.0.6/src/dhcpcd.c#L2144 [2]: https://github.com/nmeum/guix-channel/blob/b1b80697a9d35ca015ce56ccf3031f91f2bf554f/src/nmeum/services/networking.scm#L130-L132
Hi Sören, Apologies for the late reply. Sören Tempel <soeren@soeren-tempel.net> skribis: > Ludovic Courtès <ludo@gnu.org> wrote: >> 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? > > I think I haven't fully grasped G-Expressions yet. I do understand how > this would work inside the shepherd start gexp. However, I am not sure > how I would check if /bin/dhclient or /bin/dhcpcd exist in the package > within the dhcp-client-account-service since, at least as far as > I understand, the dhcp-client-account-service is itself not a gexp. I meant, within the ‘start’ method, something like: (start #~(… ;; Check whether we’re dealing with ISC’s dhclient or dhcpcd. (let ((type (if (file-exists? (string-append #$package "/bin/dhclient")) 'isc-dhcp 'dhcpcd))) (fork+exec-command …)))) Does that make sense? >> 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? > > Unfortunately, I don't think there is any way to force the PID file > name. Furthermore, the pidfile path depends on the amount interfaces > specified. If there is only one interface, a single dhcpcd instance is > spawned and the pidfile refers to that. Otherwise, a management process > is started and the pidfile refers to the management process [1]. > > I think the best solution here would be to spawn dhcpcd as child process > of GNU shepherd and have shepherd supervise it that way. I have a service > doing that too, but its annoying to integrate with the existing dhclient > service [2]. Maybe that's something we can migrate to in the long run. What do others do? I guess it’s not possible to have a systemd .service file given those PID file semantics, is it? Thanks, Ludo’.
diff --git a/gnu/services/networking.scm b/gnu/services/networking.scm index 495d049728..3621e2bda2 100644 --- a/gnu/services/networking.scm +++ b/gnu/services/networking.scm @@ -316,25 +316,21 @@ (define-record-type* <dhcp-client-configuration> (define dhcp-client-shepherd-service (match-lambda ((? dhcp-client-configuration? config) - (let ((package (dhcp-client-configuration-package config)) - (requirement (dhcp-client-configuration-shepherd-requirement config)) - (provision (dhcp-client-configuration-shepherd-provision config)) - (interfaces (dhcp-client-configuration-interfaces config)) - (pid-file "/var/run/dhclient.pid")) + (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))) (list (shepherd-service (documentation "Set up networking via DHCP.") (requirement `(user-processes udev ,@requirement)) (provision provision) - ;; XXX: Running with '-nw' ("no wait") avoids blocking for a minute when - ;; networking is unavailable, but also means that the interface is not up - ;; yet when 'start' completes. To wait for the interface to be ready, one - ;; should instead monitor udev events. (start #~(lambda _ - (define dhclient - (string-append #$package "/sbin/dhclient")) + (use-modules (ice-9 popen) + (ice-9 rdelim)) - ;; When invoked without any arguments, 'dhclient' discovers all + ;; When invoked without any arguments, the client discovers all ;; non-loopback interfaces *that are up*. However, the relevant ;; interfaces are typically down at this point. Thus we perform ;; our own interface discovery here. @@ -355,17 +351,40 @@ (define dhcp-client-shepherd-service (_ #~'#$interfaces)))) - (false-if-exception (delete-file #$pid-file)) - (let ((pid (fork+exec-command - ;; By default dhclient uses a - ;; pre-standardization implementation of - ;; DDNS, which is incompatable with - ;; non-ISC DHCP servers; thus, pass '-I'. - ;; <https://kb.isc.org/docs/aa-01091>. - (cons* dhclient "-nw" "-I" - "-pf" #$pid-file ifaces)))) - (and (zero? (cdr (waitpid pid))) - (read-pid-file #$pid-file))))) + ;; 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)))) + (else + (error (G_ "unknown 'package' value in dhcp-client-configuration"))))) + + (let ((pid-file (cdr exec-config)) + (exec-cmd (car exec-config))) + (false-if-exception (delete-file pid-file)) + (let ((pid (fork+exec-command exec-cmd))) + (and (zero? (cdr (waitpid pid))) + (read-pid-file pid-file)))))) (stop #~(make-kill-destructor)))))) (package (warning (G_ "'dhcp-client' service now expects a \ @@ -377,10 +396,27 @@ (define dhcp-client-shepherd-service (dhcp-client-configuration (package package)))))) +(define (dhcp-client-account-service config) + (let ((package (dhcp-client-configuration-package config))) + ;; Contrary to other DHCP clients (e.g. dhclient), dhcpcd supports + ;; privilege separation. Hence, we need to create an account here. + (if (string=? "dhcpcd" (package-name package)) + (list (user-group (name "dhcpcd") (system? #t)) + (user-account + (name "dhcpcd") + (group "dhcpcd") + (system? #t) + (comment "dhcpcd daemon user") + (home-directory "/var/empty") + (shell "/run/current-system/profile/sbin/nologin"))) + '()))) + (define dhcp-client-service-type (service-type (name 'dhcp-client) (extensions - (list (service-extension shepherd-root-service-type + (list (service-extension account-service-type + dhcp-client-account-service) + (service-extension shepherd-root-service-type dhcp-client-shepherd-service))) (default-value (dhcp-client-configuration)) (description "Run @command{dhcp}, a Dynamic Host Configuration diff --git a/gnu/system.scm b/gnu/system.scm index 3cd64a5c9f..a7676ec90e 100644 --- a/gnu/system.scm +++ 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 iproute wget ;; wireless-tools is deprecated in favor of iw, but it's still what