GNU bug report logs - #29732
[PATCH 0/1] Add a DHCP daemon service

Previous Next

Package: guix-patches;

Reported by: Chris Marusich <cmmarusich <at> gmail.com>

Date: Sat, 16 Dec 2017 08:36:02 UTC

Severity: normal

Tags: patch

Done: Chris Marusich <cmmarusich <at> gmail.com>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 29732 in the body.
You can then email your comments to 29732 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to guix-patches <at> gnu.org:
bug#29732; Package guix-patches. (Sat, 16 Dec 2017 08:36:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Chris Marusich <cmmarusich <at> gmail.com>:
New bug report received and forwarded. Copy sent to guix-patches <at> gnu.org. (Sat, 16 Dec 2017 08:36:02 GMT) Full text and rfc822 format available.

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

From: Chris Marusich <cmmarusich <at> gmail.com>
To: guix-patches <at> gnu.org
Cc: Chris Marusich <cmmarusich <at> gmail.com>
Subject: [PATCH 0/1] Add a DHCP daemon service
Date: Sat, 16 Dec 2017 00:35:28 -0800
Hi,

The following patch adds a DHCP daemon service.  It applies cleanly to
commit 341bddb31542d03bef35b1234e6e9466be337798.

I haven't found the time to add system tests yet, and I'm not sure how
easy it would be to do that (wouldn't such a system test require at
least two marionetted VMs?  Or is it possible to do it with just
one?).  Any thoughts about how to do that would be welcome.

In addition, I decided not to provide any kind of Guile scheme
representation for the daemon's config file, since that was the
quickest way for me to get a working DHCP daemon service.  I think it
makes sense to let a user specify a config file explicitly, like this
patch does.  I also think it would be neat to provide a way to
configure the daemon in Guile scheme, like we do for other services
such as openssh.  Any thoughts on this front would be welcome, too.

Chris Marusich (1):
  services: Add dhcpd-service-type and <dhcpd-configuration>.

 doc/guix.texi               | 17 +++++++++++
 gnu/services/networking.scm | 72 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+)

-- 
2.15.1





Information forwarded to guix-patches <at> gnu.org:
bug#29732; Package guix-patches. (Sat, 16 Dec 2017 08:54:02 GMT) Full text and rfc822 format available.

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

From: Chris Marusich <cmmarusich <at> gmail.com>
To: 29732 <at> debbugs.gnu.org
Cc: Chris Marusich <cmmarusich <at> gmail.com>
Subject: [PATCH 1/1] services: Add dhcpd-service-type and
 <dhcpd-configuration>.
Date: Sat, 16 Dec 2017 00:52:42 -0800
* doc/guix.texi (Networking Services): Document it.
* gnu/services/networking.scm (dhcpd-service-type): Add it.
(dhcpd-configuration, dhcpd-configuration?): Add it.
(dhcpd-configuration-package): Add it.
(dhcpd-configuration-config-file): Add it.
(dhcpd-configuration-ip-version): Add it.
(dhcpd-configuration-run-directory): Add it.
(dhcpd-configuration-lease-file): Add it.
(dhcpd-configuration-pid-file): Add it.
(dhcpd-configuration-interfaces): Add it.
---
 doc/guix.texi               | 17 +++++++++++
 gnu/services/networking.scm | 72 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 89 insertions(+)

diff --git a/doc/guix.texi b/doc/guix.texi
index 64f73b38a..3b62a0578 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -10357,6 +10357,23 @@ Return a service that runs @var{dhcp}, a Dynamic Host Configuration
 Protocol (DHCP) client, on all the non-loopback network interfaces.
 @end deffn
 
+@deffn {Scheme Procedure} dhcpd-service-type
+This type defines a DHCP daemon.  To create a service of this type, you
+must supply a @code{<dhcpd-configuration>}.  For example:
+
+@example
+(service dhcpd-service-type
+         (dhcpd-configuration (config-file (local-file "my-dhcpd.conf"))
+                              (interfaces '("enp2s0f0"))))
+@end example
+
+Here, @file{my-dhcpd.conf} is a local file that defines a valid
+@command{dhcpd} configuration.  Any ``file-like'' object will do here.
+For example, you could use @code{plain-file} instead of
+@code{local-file} if you prefer to embed the @code{dhcpd} configuration
+file in your scheme code.
+@end deffn
+
 @defvr {Scheme Variable} static-networking-service-type
 This is the type for statically-configured network interfaces.
 @c TODO Document <static-networking> data structures.
diff --git a/gnu/services/networking.scm b/gnu/services/networking.scm
index b0c23aafc..d562b7011 100644
--- a/gnu/services/networking.scm
+++ b/gnu/services/networking.scm
@@ -55,6 +55,18 @@
             static-networking-service
             static-networking-service-type
             dhcp-client-service
+
+            dhcpd-service-type
+            dhcpd-configuration
+            dhcpd-configuration?
+            dhcpd-configuration-package
+            dhcpd-configuration-config-file
+            dhcpd-configuration-ip-version
+            dhcpd-configuration-run-directory
+            dhcpd-configuration-lease-file
+            dhcpd-configuration-pid-file
+            dhcpd-configuration-interfaces
+
             %ntp-servers
 
             ntp-configuration
@@ -338,6 +350,66 @@ to handle."
 Protocol (DHCP) client, on all the non-loopback network interfaces."
   (service dhcp-client-service-type dhcp))
 
+(define-record-type* <dhcpd-configuration> dhcpd-configuration
+  make-dhcpd-configuration
+  dhcpd-configuration?
+  (package   dhcpd-configuration-package ;<package>
+             (default isc-dhcp))
+  (config-file   dhcpd-configuration-config-file ;file-like
+                 (default #f))
+  (ip-version dhcpd-ip-version          ; either "4" or "6"
+              (default "4"))
+  (run-directory dhcpd-run-directory
+                 (default "/run/dhcpd"))
+  (lease-file dhcpd-lease-file
+              (default "/var/db/dhcpd.leases"))
+  (pid-file dhcpd-pid-file
+            (default "/run/dhcpd/dhcpd.pid"))
+  (interfaces dhcpd-interfaces ; list of strings, e.g. (list "enp0s25")
+              (default '())))
+
+(define dhcpd-shepherd-service
+  (match-lambda
+    (($ <dhcpd-configuration> package config-file ip-version _ lease-file pid-file interfaces)
+     (when (null-list? interfaces)
+       (error "Must specify at least one interface for DHCP daemon to use"))
+     (unless config-file
+       (error "Must supply a config-file"))
+     (list (shepherd-service
+            (provision '(dhcp-daemon))
+            (documentation "Run the DHCP daemon.")
+            (requirement '(networking))
+            (start #~(make-forkexec-constructor
+                      '(#$(file-append package "/sbin/dhcpd")
+                          #$(string-append "-" ip-version)
+                          "-lf" #$lease-file
+                          "-pf" #$pid-file
+                          "-cf" #$config-file
+                          #$@interfaces)
+                      #:pid-file #$pid-file))
+            (stop #~(make-kill-destructor)))))))
+
+(define dhcpd-activation
+  (match-lambda
+    (($ <dhcpd-configuration> package config-file _ run-directory lease-file _ _)
+     #~(begin
+         (unless (file-exists? #$run-directory)
+           (mkdir #$run-directory))
+         ;; According to the DHCP manual (man dhcpd.leases), the lease
+         ;; database must be present for dhcpd to start successfully.
+         (unless (file-exists? #$lease-file)
+           (with-output-to-file #$lease-file
+             (lambda _ (display ""))))
+         ;; Validate the config.
+         (zero? (system* #$(file-append package "/sbin/dhcpd") "-t" "-cf" #$config-file))))))
+
+(define dhcpd-service-type
+  (service-type
+   (name 'dhcpd)
+   (extensions
+    (list (service-extension shepherd-root-service-type dhcpd-shepherd-service)
+          (service-extension activation-service-type dhcpd-activation)))))
+
 (define %ntp-servers
   ;; Default set of NTP servers. These URLs are managed by the NTP Pool project.
   ;; Within Guix, Leo Famulari <leo <at> famulari.name> is the administrative contact
-- 
2.15.1





Information forwarded to guix-patches <at> gnu.org:
bug#29732; Package guix-patches. (Sat, 16 Dec 2017 22:06:02 GMT) Full text and rfc822 format available.

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

From: Christopher Baines <mail <at> cbaines.net>
To: Chris Marusich <cmmarusich <at> gmail.com>
Cc: 29732 <at> debbugs.gnu.org
Subject: Re: [bug#29732] [PATCH 0/1] Add a DHCP daemon service
Date: Sat, 16 Dec 2017 22:05:16 +0000
Chris Marusich writes:

> Hi,
>
> The following patch adds a DHCP daemon service.  It applies cleanly to
> commit 341bddb31542d03bef35b1234e6e9466be337798.
>
> I haven't found the time to add system tests yet, and I'm not sure how
> easy it would be to do that (wouldn't such a system test require at
> least two marionetted VMs?  Or is it possible to do it with just
> one?).  Any thoughts about how to do that would be welcome.
>
> In addition, I decided not to provide any kind of Guile scheme
> representation for the daemon's config file, since that was the
> quickest way for me to get a working DHCP daemon service.  I think it
> makes sense to let a user specify a config file explicitly, like this
> patch does.  I also think it would be neat to provide a way to
> configure the daemon in Guile scheme, like we do for other services
> such as openssh.  Any thoughts on this front would be welcome, too.
>
> Chris Marusich (1):
>   services: Add dhcpd-service-type and <dhcpd-configuration>.
>
>  doc/guix.texi               | 17 +++++++++++
>  gnu/services/networking.scm | 72 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 89 insertions(+)

I had a quick look at the patch, and it looks good to me.

A system test would be nice. I think the important thing to test is the
Guix part of the service, so if it's possible to get it to start on a
VM, I think just checking the status of the shepherd service would be a
good test.

I think not providing a Guile configuration approach is fine as well.

Thanks,

Chris




Information forwarded to guix-patches <at> gnu.org:
bug#29732; Package guix-patches. (Tue, 19 Dec 2017 07:34:01 GMT) Full text and rfc822 format available.

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

From: Chris Marusich <cmmarusich <at> gmail.com>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 29732 <at> debbugs.gnu.org
Subject: Re: [bug#29732] [PATCH 0/1] Add a DHCP daemon service
Date: Mon, 18 Dec 2017 23:33:33 -0800
[Message part 1 (text/plain, inline)]
Christopher Baines <mail <at> cbaines.net> writes:

> I had a quick look at the patch, and it looks good to me.

Thank you for taking a look!

> A system test would be nice. I think the important thing to test is the
> Guix part of the service, so if it's possible to get it to start on a
> VM, I think just checking the status of the shepherd service would be a
> good test.

OK.  I'll find time to add something by the end of January.

-- 
Chris
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#29732; Package guix-patches. (Tue, 27 Feb 2018 09:49:02 GMT) Full text and rfc822 format available.

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Chris Marusich <cmmarusich <at> gmail.com>
Cc: 29732 <at> debbugs.gnu.org, Christopher Baines <mail <at> cbaines.net>
Subject: Re: [bug#29732] [PATCH 0/1] Add a DHCP daemon service
Date: Tue, 27 Feb 2018 10:48:14 +0100
Heya Chris,

Looks like this patch is pretty much ready.  Would you have time to take
a look again?

TIA,
Ludo’.

Chris Marusich <cmmarusich <at> gmail.com> skribis:

> Christopher Baines <mail <at> cbaines.net> writes:
>
>> I had a quick look at the patch, and it looks good to me.
>
> Thank you for taking a look!
>
>> A system test would be nice. I think the important thing to test is the
>> Guix part of the service, so if it's possible to get it to start on a
>> VM, I think just checking the status of the shepherd service would be a
>> good test.
>
> OK.  I'll find time to add something by the end of January.




Information forwarded to guix-patches <at> gnu.org:
bug#29732; Package guix-patches. (Tue, 27 Feb 2018 10:32:01 GMT) Full text and rfc822 format available.

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

From: Clément Lassieur <clement <at> lassieur.org>
To: Chris Marusich <cmmarusich <at> gmail.com>
Cc: 29732 <at> debbugs.gnu.org
Subject: Re: [bug#29732] [PATCH 1/1] services: Add dhcpd-service-type and
 <dhcpd-configuration>.
Date: Tue, 27 Feb 2018 11:31:10 +0100
Hi Chris, thank you for this service!

A few comments:

Chris Marusich <cmmarusich <at> gmail.com> writes:

> * doc/guix.texi (Networking Services): Document it.
> * gnu/services/networking.scm (dhcpd-service-type): Add it.
> (dhcpd-configuration, dhcpd-configuration?): Add it.
> (dhcpd-configuration-package): Add it.
> (dhcpd-configuration-config-file): Add it.
> (dhcpd-configuration-ip-version): Add it.
> (dhcpd-configuration-run-directory): Add it.
> (dhcpd-configuration-lease-file): Add it.
> (dhcpd-configuration-pid-file): Add it.
> (dhcpd-configuration-interfaces): Add it.
> ---
>  doc/guix.texi               | 17 +++++++++++
>  gnu/services/networking.scm | 72 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 89 insertions(+)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 64f73b38a..3b62a0578 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -10357,6 +10357,23 @@ Return a service that runs @var{dhcp}, a Dynamic Host Configuration
>  Protocol (DHCP) client, on all the non-loopback network interfaces.
>  @end deffn
>  
> +@deffn {Scheme Procedure} dhcpd-service-type
> +This type defines a DHCP daemon.  To create a service of this type, you
> +must supply a @code{<dhcpd-configuration>}.  For example:
> +
> +@example
> +(service dhcpd-service-type
> +         (dhcpd-configuration (config-file (local-file "my-dhcpd.conf"))
> +                              (interfaces '("enp2s0f0"))))
> +@end example
> +
> +Here, @file{my-dhcpd.conf} is a local file that defines a valid
> +@command{dhcpd} configuration.  Any ``file-like'' object will do here.
> +For example, you could use @code{plain-file} instead of
> +@code{local-file} if you prefer to embed the @code{dhcpd} configuration
> +file in your scheme code.
> +@end deffn
> +
>  @defvr {Scheme Variable} static-networking-service-type
>  This is the type for statically-configured network interfaces.
>  @c TODO Document <static-networking> data structures.
> diff --git a/gnu/services/networking.scm b/gnu/services/networking.scm
> index b0c23aafc..d562b7011 100644
> --- a/gnu/services/networking.scm
> +++ b/gnu/services/networking.scm
> @@ -55,6 +55,18 @@
>              static-networking-service
>              static-networking-service-type
>              dhcp-client-service
> +
> +            dhcpd-service-type
> +            dhcpd-configuration
> +            dhcpd-configuration?
> +            dhcpd-configuration-package
> +            dhcpd-configuration-config-file
> +            dhcpd-configuration-ip-version
> +            dhcpd-configuration-run-directory
> +            dhcpd-configuration-lease-file
> +            dhcpd-configuration-pid-file
> +            dhcpd-configuration-interfaces
> +
>              %ntp-servers
>  
>              ntp-configuration
> @@ -338,6 +350,66 @@ to handle."
>  Protocol (DHCP) client, on all the non-loopback network interfaces."
>    (service dhcp-client-service-type dhcp))
>  
> +(define-record-type* <dhcpd-configuration> dhcpd-configuration
> +  make-dhcpd-configuration
> +  dhcpd-configuration?
> +  (package   dhcpd-configuration-package ;<package>
> +             (default isc-dhcp))
> +  (config-file   dhcpd-configuration-config-file ;file-like
> +                 (default #f))
> +  (ip-version dhcpd-ip-version          ; either "4" or "6"
> +              (default "4"))

Upstream defaults to "6".  I guess it's because they want to encourage
people to use IPv6.  Maybe we should respect their choice and default to
"6" as well?

> +  (run-directory dhcpd-run-directory
> +                 (default "/run/dhcpd"))
> +  (lease-file dhcpd-lease-file
> +              (default "/var/db/dhcpd.leases"))
> +  (pid-file dhcpd-pid-file
> +            (default "/run/dhcpd/dhcpd.pid"))

I wonder, does it make sense to run two instances of the daemon, one for
IPv4 and another for IPv6?  Would having the IP version included in the
pid file name make it possible?  (dhcpd-4.pid and dhcp-6.pid)

> +  (interfaces dhcpd-interfaces ; list of strings, e.g. (list "enp0s25")
> +              (default '())))

I don't really understand the logic of the indentation and alignment
of this whole block :-).

> +(define dhcpd-shepherd-service
> +  (match-lambda
> +    (($ <dhcpd-configuration> package config-file ip-version _ lease-file pid-file interfaces)
> +     (when (null-list? interfaces)
> +       (error "Must specify at least one interface for DHCP daemon to use"))
> +     (unless config-file
> +       (error "Must supply a config-file"))
> +     (list (shepherd-service
> +            (provision '(dhcp-daemon))
> +            (documentation "Run the DHCP daemon.")
> +            (requirement '(networking))
> +            (start #~(make-forkexec-constructor
> +                      '(#$(file-append package "/sbin/dhcpd")
> +                          #$(string-append "-" ip-version)
> +                          "-lf" #$lease-file
> +                          "-pf" #$pid-file
> +                          "-cf" #$config-file
> +                          #$@interfaces)
> +                      #:pid-file #$pid-file))
> +            (stop #~(make-kill-destructor)))))))
> +
> +(define dhcpd-activation
> +  (match-lambda
> +    (($ <dhcpd-configuration> package config-file _ run-directory lease-file _ _)
> +     #~(begin
> +         (unless (file-exists? #$run-directory)
> +           (mkdir #$run-directory))

mkdir-p I guess

> +         ;; According to the DHCP manual (man dhcpd.leases), the lease
> +         ;; database must be present for dhcpd to start successfully.
> +         (unless (file-exists? #$lease-file)
> +           (with-output-to-file #$lease-file
> +             (lambda _ (display ""))))
> +         ;; Validate the config.
> +         (zero? (system* #$(file-append package "/sbin/dhcpd") "-t" "-cf" #$config-file))))))
> +
> +(define dhcpd-service-type
> +  (service-type
> +   (name 'dhcpd)
> +   (extensions
> +    (list (service-extension shepherd-root-service-type dhcpd-shepherd-service)
> +          (service-extension activation-service-type dhcpd-activation)))))
> +
>  (define %ntp-servers
>    ;; Default set of NTP servers. These URLs are managed by the NTP Pool project.
>    ;; Within Guix, Leo Famulari <leo <at> famulari.name> is the administrative contact

Also, could you stick to 80 columns?

Thank you!
Clément




Information forwarded to guix-patches <at> gnu.org:
bug#29732; Package guix-patches. (Fri, 13 Apr 2018 07:42:01 GMT) Full text and rfc822 format available.

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

From: Chris Marusich <cmmarusich <at> gmail.com>
To: Clément Lassieur <clement <at> lassieur.org>
Cc: 29732 <at> debbugs.gnu.org
Subject: Re: [bug#29732] [PATCH 1/1] services: Add dhcpd-service-type and
 <dhcpd-configuration>.
Date: Fri, 13 Apr 2018 00:41:38 -0700
[Message part 1 (text/plain, inline)]
Hi Clément and Ludo,

Thank you for the review!

Clément Lassieur <clement <at> lassieur.org> writes:

>> +  (ip-version dhcpd-ip-version          ; either "4" or "6"
>> +              (default "4"))
>
> Upstream defaults to "6".  I guess it's because they want to encourage
> people to use IPv6.  Maybe we should respect their choice and default to
> "6" as well?

Are you sure about this?  The manual (man 8 dhcpd) says:

  COMMAND LINE OPTIONS
       -4     Run as a DHCP server. This is the default and cannot  be
              combined with -6.

       -6     Run as a DHCPv6 server. This cannot be combined with -4.

       -4o6 port
              Participate in the DHCPv4 over DHCPv6 protocol specified
              by RFC 7341.  This associates  a  DHCPv4  and  a  DHCPv6
              server  to  allow  the  v4 server to receive v4 requests
              that were encapsulated in a  v6  packet.   Communication
              between the two servers is done on a pair of UDP sockets
              bound to ::1 port and port + 1.  Both  servers  must  be
              launched using the same port argument.

I agree we should respect the default that upstream sets.  Where did you
see it indicated that upstream sets the default to 6?

In addition, I didn't even know about the -4o6 option, but thankfully my
patch would still allow someone to specify that as the ip-version, too.

> I wonder, does it make sense to run two instances of the daemon, one for
> IPv4 and another for IPv6?  Would having the IP version included in the
> pid file name make it possible?  (dhcpd-4.pid and dhcp-6.pid)

That would make it possible, yes.  However, I think that it should be up
to the user to decide whether or not to run two services.  If they want
to run two (or three) dhcpd services for the different IP protocol
versions, then they should be able to do so easily.  I might need to
adjust the "provision" part of the dhcpd-shepherd-service to allow that,
though.  I'm not sure what happens if two services try to provision the
same "provision" symbol - probably nothing good.

>> +  (interfaces dhcpd-interfaces ; list of strings, e.g. (list "enp0s25")
>> +              (default '())))
>
> I don't really understand the logic of the indentation and alignment
> of this whole block :-).

I try to follow the indentation style mentioned in the Guix manual (see:
(guix) Coding Style), but I may have let a few rough spots slip through.
If you have specific suggestions for how to improve the indentation, I
would be glad to hear them.

>> +(define dhcpd-activation
>> +  (match-lambda
>> +    (($ <dhcpd-configuration> package config-file _ run-directory lease-file _ _)
>> +     #~(begin
>> +         (unless (file-exists? #$run-directory)
>> +           (mkdir #$run-directory))
>
> mkdir-p I guess

Yeah, that's probably better.  I'll see about adjusting it.

>> +         ;; According to the DHCP manual (man dhcpd.leases), the lease
>> +         ;; database must be present for dhcpd to start successfully.
>> +         (unless (file-exists? #$lease-file)
>> +           (with-output-to-file #$lease-file
>> +             (lambda _ (display ""))))
>> +         ;; Validate the config.
>> +         (zero? (system* #$(file-append package "/sbin/dhcpd") "-t" "-cf" #$config-file))))))
>> +
>> +(define dhcpd-service-type
>> +  (service-type
>> +   (name 'dhcpd)
>> +   (extensions
>> +    (list (service-extension shepherd-root-service-type dhcpd-shepherd-service)
>> +          (service-extension activation-service-type dhcpd-activation)))))
>> +
>>  (define %ntp-servers
>>    ;; Default set of NTP servers. These URLs are managed by the NTP Pool project.
>>    ;; Within Guix, Leo Famulari <leo <at> famulari.name> is the administrative contact
>
> Also, could you stick to 80 columns?

Certainly - looks like I accidentally missed a few long lines.  I've got
a new patch almost ready to share which will improve the formatting and,
most importantly, add a couple tests!

I'll try to clean it up and post it by the end of January.  ;-)

-- 
Chris
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#29732; Package guix-patches. (Fri, 13 Apr 2018 09:03:02 GMT) Full text and rfc822 format available.

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

From: Clément Lassieur <clement <at> lassieur.org>
To: Chris Marusich <cmmarusich <at> gmail.com>
Cc: 29732 <at> debbugs.gnu.org
Subject: Re: [bug#29732] [PATCH 1/1] services: Add dhcpd-service-type and
 <dhcpd-configuration>.
Date: Fri, 13 Apr 2018 11:02:02 +0200
Chris Marusich <cmmarusich <at> gmail.com> writes:

> Hi Clément and Ludo,
>
> Thank you for the review!
>
> Clément Lassieur <clement <at> lassieur.org> writes:
>
>>> +  (ip-version dhcpd-ip-version          ; either "4" or "6"
>>> +              (default "4"))
>>
>> Upstream defaults to "6".  I guess it's because they want to encourage
>> people to use IPv6.  Maybe we should respect their choice and default to
>> "6" as well?
>
> Are you sure about this?  The manual (man 8 dhcpd) says:
>
>   COMMAND LINE OPTIONS
>        -4     Run as a DHCP server. This is the default and cannot  be
>               combined with -6.
>
>        -6     Run as a DHCPv6 server. This cannot be combined with -4.
>
>        -4o6 port
>               Participate in the DHCPv4 over DHCPv6 protocol specified
>               by RFC 7341.  This associates  a  DHCPv4  and  a  DHCPv6
>               server  to  allow  the  v4 server to receive v4 requests
>               that were encapsulated in a  v6  packet.   Communication
>               between the two servers is done on a pair of UDP sockets
>               bound to ::1 port and port + 1.  Both  servers  must  be
>               launched using the same port argument.
>
> I agree we should respect the default that upstream sets.  Where did you
> see it indicated that upstream sets the default to 6?

Indeed, you are right.  I saw it there:
https://linux.die.net/man/8/dhcpd, but it seems like it's an old version
of the documentation, which contains an error.  The error was fixed in
commit 802fdea172f0d2f59298a69efb7ccfd492feb7f0 of
https://source.isc.org/git/dhcp.git

    - Documentation cleanup
      [ISC-Bugs #23326] Updated References document, several man page updates

Which contains

--8<---------------cut here---------------start------------->8---
modified   server/dhcpd.8
@@ -28,7 +28,7 @@
 .\" Support and other services are available for ISC products - see
 .\" https://www.isc.org for more information or to learn more about ISC.
 .\"
-.\" $Id: dhcpd.8,v 1.34 2011/04/15 21:58:12 sar Exp $
+.\" $Id: dhcpd.8,v 1.35 2011/05/20 13:48:33 tomasz Exp $
 .\"
 .TH dhcpd 8
 .SH NAME
@@ -190,11 +190,11 @@ possible, and listen for DHCP broadcasts on each interface.
 .SH COMMAND LINE OPTIONS
 .TP
 .BI \-4
-Run as a DHCP server.  This cannot be combined with \fB\-6\fR.
+Run as a DHCP server. This is the default and cannot be combined with
+\fB\-6\fR.
 .TP
 .BI \-6
-Run as a DHCPv6 server.  This is the default and cannot be combined
-with \fB\-4\fR.
+Run as a DHCPv6 server. This cannot be combined with \fB\-4\fR.
 .TP
 .BI \-p \ port
 The udp port number on which 
--8<---------------cut here---------------end--------------->8---

> In addition, I didn't even know about the -4o6 option, but thankfully my
> patch would still allow someone to specify that as the ip-version, too.
>
>> I wonder, does it make sense to run two instances of the daemon, one for
>> IPv4 and another for IPv6?  Would having the IP version included in the
>> pid file name make it possible?  (dhcpd-4.pid and dhcp-6.pid)
>
> That would make it possible, yes.  However, I think that it should be up
> to the user to decide whether or not to run two services.  If they want
> to run two (or three) dhcpd services for the different IP protocol
> versions, then they should be able to do so easily.  I might need to
> adjust the "provision" part of the dhcpd-shepherd-service to allow that,
> though.  I'm not sure what happens if two services try to provision the
> same "provision" symbol - probably nothing good.
>
>>> +  (interfaces dhcpd-interfaces ; list of strings, e.g. (list "enp0s25")
>>> +              (default '())))
>>
>> I don't really understand the logic of the indentation and alignment
>> of this whole block :-).
>
> I try to follow the indentation style mentioned in the Guix manual (see:
> (guix) Coding Style), but I may have let a few rough spots slip through.
> If you have specific suggestions for how to improve the indentation, I
> would be glad to hear them.

I don't remember why I said this, sorry about it.

>>> +(define dhcpd-activation
>>> +  (match-lambda
>>> +    (($ <dhcpd-configuration> package config-file _ run-directory lease-file _ _)
>>> +     #~(begin
>>> +         (unless (file-exists? #$run-directory)
>>> +           (mkdir #$run-directory))
>>
>> mkdir-p I guess
>
> Yeah, that's probably better.  I'll see about adjusting it.
>
>>> +         ;; According to the DHCP manual (man dhcpd.leases), the lease
>>> +         ;; database must be present for dhcpd to start successfully.
>>> +         (unless (file-exists? #$lease-file)
>>> +           (with-output-to-file #$lease-file
>>> +             (lambda _ (display ""))))
>>> +         ;; Validate the config.
>>> +         (zero? (system* #$(file-append package "/sbin/dhcpd") "-t" "-cf" #$config-file))))))
>>> +
>>> +(define dhcpd-service-type
>>> +  (service-type
>>> +   (name 'dhcpd)
>>> +   (extensions
>>> +    (list (service-extension shepherd-root-service-type dhcpd-shepherd-service)
>>> +          (service-extension activation-service-type dhcpd-activation)))))
>>> +
>>>  (define %ntp-servers
>>>    ;; Default set of NTP servers. These URLs are managed by the NTP Pool project.
>>>    ;; Within Guix, Leo Famulari <leo <at> famulari.name> is the administrative contact
>>
>> Also, could you stick to 80 columns?
>
> Certainly - looks like I accidentally missed a few long lines.  I've got
> a new patch almost ready to share which will improve the formatting and,
> most importantly, add a couple tests!

Cool!  This one looks good to me anyway.

> I'll try to clean it up and post it by the end of January.  ;-)

That's very ambitious!  Take your time :-)




Information forwarded to guix-patches <at> gnu.org:
bug#29732; Package guix-patches. (Tue, 17 Apr 2018 05:52:01 GMT) Full text and rfc822 format available.

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

From: Chris Marusich <cmmarusich <at> gmail.com>
To: Clément Lassieur <clement <at> lassieur.org>, ludo <at> gnu.org
 (Ludovic Courtès)
Cc: 29732 <at> debbugs.gnu.org
Subject: Re: [bug#29732] [PATCH 1/1] services: Add dhcpd-service-type and
 <dhcpd-configuration>.
Date: Mon, 16 Apr 2018 22:51:12 -0700
[Message part 1 (text/plain, inline)]
Hi Clément and Ludo!

Here's a new patch.  It mainly tidies up some formatting, makes it
possible to run more than one version of the DHCP daemon at the same
time (e.g., for IPv4, IPv6, and IPv4 over IPv6), and adds a system test
to verify that the DHCPv4 daemon can start up without error.

I tried adding a test case for DHCPv6, but I ran into complications.
Specifically, I'm not sure how to give an IPv6 address and subnet to the
eth0 interface within the test VM using the static-networking-service.
The DHCPv6 daemon requires the interface to have an IPv6 subnet
configured, so it refuses to run on that interface.

For now, it's nice enough, I think, that we have a DHCPv4 system test!
Please let me know what you think.

-- 
Chris
[0001-services-Add-dhcpd-service-type-and-dhcpd-configurat.patch (text/x-patch, attachment)]
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#29732; Package guix-patches. (Wed, 18 Apr 2018 20:29:01 GMT) Full text and rfc822 format available.

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

From: ludo <at> gnu.org (Ludovic Courtès)
To: Chris Marusich <cmmarusich <at> gmail.com>
Cc: 29732 <at> debbugs.gnu.org,
 Clément Lassieur <clement <at> lassieur.org>
Subject: Re: [bug#29732] [PATCH 1/1] services: Add dhcpd-service-type and
 <dhcpd-configuration>.
Date: Wed, 18 Apr 2018 22:28:33 +0200
Hello Chris,

Chris Marusich <cmmarusich <at> gmail.com> skribis:

> Here's a new patch.  It mainly tidies up some formatting, makes it
> possible to run more than one version of the DHCP daemon at the same
> time (e.g., for IPv4, IPv6, and IPv4 over IPv6), and adds a system test
> to verify that the DHCPv4 daemon can start up without error.
>
> I tried adding a test case for DHCPv6, but I ran into complications.
> Specifically, I'm not sure how to give an IPv6 address and subnet to the
> eth0 interface within the test VM using the static-networking-service.
> The DHCPv6 daemon requires the interface to have an IPv6 subnet
> configured, so it refuses to run on that interface.

OK.

> For now, it's nice enough, I think, that we have a DHCPv4 system test!
> Please let me know what you think.

I agree, nice job!

> From 5dd2e6853f1a332e55f3a4ba69b9baf199458fcb Mon Sep 17 00:00:00 2001
> From: Chris Marusich <cmmarusich <at> gmail.com>
> Date: Sat, 16 Dec 2017 00:52:42 -0800
> Subject: [PATCH] services: Add dhcpd-service-type and <dhcpd-configuration>.
>
> * doc/guix.texi (Networking Services): Document it.
> * gnu/services/networking.scm (dhcpd-service-type): Add it.
> (dhcpd-configuration, dhcpd-configuration?): Add it.
> (dhcpd-configuration-package): Add it.
> (dhcpd-configuration-config-file): Add it.
> (dhcpd-configuration-version): Add it.
> (dhcpd-configuration-run-directory): Add it.
> (dhcpd-configuration-lease-file): Add it.
> (dhcpd-configuration-pid-file): Add it.
> (dhcpd-configuration-interfaces): Add it.
> ---
>  doc/guix.texi               | 17 +++++++
>  gnu/services/networking.scm | 80 ++++++++++++++++++++++++++++++
>  gnu/tests/networking.scm    | 97 ++++++++++++++++++++++++++++++++++++-

Please mention the gnu/tests/networking.scm changes in the log.

Otherwise LGTM, thank you!

Ludo’.




Reply sent to Chris Marusich <cmmarusich <at> gmail.com>:
You have taken responsibility. (Sun, 22 Apr 2018 07:45:01 GMT) Full text and rfc822 format available.

Notification sent to Chris Marusich <cmmarusich <at> gmail.com>:
bug acknowledged by developer. (Sun, 22 Apr 2018 07:45:02 GMT) Full text and rfc822 format available.

Message #37 received at 29732-done <at> debbugs.gnu.org (full text, mbox):

From: Chris Marusich <cmmarusich <at> gmail.com>
To: ludo <at> gnu.org (Ludovic Courtès)
Cc: 29732-done <at> debbugs.gnu.org,
 Clément Lassieur <clement <at> lassieur.org>
Subject: Re: [bug#29732] [PATCH 1/1] services: Add dhcpd-service-type and
 <dhcpd-configuration>.
Date: Sun, 22 Apr 2018 00:44:20 -0700
[Message part 1 (text/plain, inline)]
ludo <at> gnu.org (Ludovic Courtès) writes:

>> For now, it's nice enough, I think, that we have a DHCPv4 system test!
>> Please let me know what you think.
>
> I agree, nice job!

Thank you!  I hope this service proves useful to somebody besides just
me!

> Please mention the gnu/tests/networking.scm changes in the log.

Good catch; I've fixed the ChangeLog entry.  I've also added
documentation for <dhcpd-configuration> in the manual, since users
deserve good documentation.  I've committed all this as
f1104d900978900addad731dfec4e0e6e5765fbe.

Thank you both for your helpful review!

-- 
Chris
[signature.asc (application/pgp-signature, inline)]

bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Sun, 20 May 2018 11:24:03 GMT) Full text and rfc822 format available.

This bug report was last modified 5 years and 314 days ago.

Previous Next


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