[PATCH 0/1] Add a DHCP daemon service

  • Done
  • quality assurance status badge
Details
4 participants
  • Clément Lassieur
  • Chris Marusich
  • Ludovic Courtès
  • Christopher Baines
Owner
unassigned
Submitted by
Chris Marusich
Severity
normal
C
C
Chris Marusich wrote on 16 Dec 2017 09:35
(address . guix-patches@gnu.org)(name . Chris Marusich)(address . cmmarusich@gmail.com)
20171216083528.2081-1-cmmarusich@gmail.com
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
C
C
Chris Marusich wrote on 16 Dec 2017 09:52
[PATCH 1/1] services: Add dhcpd-service-type and <dhcpd-configuration>.
(address . 29732@debbugs.gnu.org)(name . Chris Marusich)(address . cmmarusich@gmail.com)
20171216085242.2309-1-cmmarusich@gmail.com
* 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(+)

Toggle diff (120 lines)
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@famulari.name> is the administrative contact
--
2.15.1
C
C
Christopher Baines wrote on 16 Dec 2017 23:05
Re: [bug#29732] [PATCH 0/1] Add a DHCP daemon service
(name . Chris Marusich)(address . cmmarusich@gmail.com)(address . 29732@debbugs.gnu.org)
87d13epbvn.fsf@cbaines.net
Chris Marusich writes:

Toggle quote (25 lines)
> 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
C
C
Chris Marusich wrote on 19 Dec 2017 08:33
(name . Christopher Baines)(address . mail@cbaines.net)(address . 29732@debbugs.gnu.org)
87k1xji33m.fsf@gmail.com
Christopher Baines <mail@cbaines.net> writes:

Toggle quote (2 lines)
> I had a quick look at the patch, and it looks good to me.

Thank you for taking a look!

Toggle quote (5 lines)
> 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
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEy/WXVcvn5+/vGD+x3UCaFdgiRp0FAlo4wM0ACgkQ3UCaFdgi
Rp1xDQ//d7wDvA8f93rhvnuD0AOZdXE5a9KL4E7i7+toeGY7exuxoZe+7bSO1VaC
J2zdUk2XZ4coMmRcepBWtdjZrh6waM74Mh86mYYtE+puJTL+0flfnm41yHvXiH3p
x3KJrLBSTk/irxW8qhMEPUpvFgWz20N3I+6WyUDCO1OPgbuaNUAun+DaBja8Emk7
/jStQxOyNTysUz8FuiuyL8WnoCXG9o9akV2Y/6gH0DlZSagx5y+/XN9UFRDx+dHy
Gs/NXhXMrw8eTQPkpH6VLtj+jB+fBfyqn9J1dkFNcGmEb5MmQFrVkf13jW2Wvwdh
CSyeT6vAwTE2/8IpAIKCrZRJFo8/28jMgtqkENT//bXPD6r/hoRwCRXkckFP2hPj
WaX6c+GFZz1vJ64MOW83jwsHR01AF+HrzYpBQ+v4KyxXxW87ZlalWpQx/9Hcbozl
m0z/RUNC2dl8v0htTKqaPSqbnQmB9Abci1btd0xMjwPRKSDl/7cw+rx/W3Y2QHoP
PfO5nqPmTQEEktdqc5lo/+3vf9SAZhi8e7bUpOsYTzBEg97jZPOtQyWAVFcyI+l8
S58hsXIzcRm37khTlW1SIOWzm5wWM1wxBID5JCo0wpfYOqoCoXH4hzRufCURudVr
7FY8LZ/soQJ4Nl/F79m1xTRVMhqdFgt7pxBZF+r4VnmbVUxXbx0=
=wkmn
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 27 Feb 2018 10:48
(name . Chris Marusich)(address . cmmarusich@gmail.com)
87h8q24vc1.fsf@gnu.org
Heya Chris,

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

TIA,
Ludo’.

Chris Marusich <cmmarusich@gmail.com> skribis:

Toggle quote (12 lines)
> Christopher Baines <mail@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.
C
C
Clément Lassieur wrote on 27 Feb 2018 11:31
Re: [bug#29732] [PATCH 1/1] services: Add dhcpd-service-type and <dhcpd-configuration>.
(name . Chris Marusich)(address . cmmarusich@gmail.com)(address . 29732@debbugs.gnu.org)
871sh6rafl.fsf@lassieur.org
Hi Chris, thank you for this service!

A few comments:

Chris Marusich <cmmarusich@gmail.com> writes:

Toggle quote (80 lines)
> * 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?

Toggle quote (7 lines)
> + (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)

Toggle quote (3 lines)
> + (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 :-).

Toggle quote (28 lines)
> +(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

Toggle quote (19 lines)
> + ;; 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@famulari.name> is the administrative contact

Also, could you stick to 80 columns?

Thank you!
Clément
C
C
Chris Marusich wrote on 13 Apr 2018 09:41
(name . Clément Lassieur)(address . clement@lassieur.org)(address . 29732@debbugs.gnu.org)
87efjjwo25.fsf@gmail.com
Hi Clément and Ludo,

Thank you for the review!

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

Toggle quote (7 lines)
>> + (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.

Toggle quote (4 lines)
> 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.

Toggle quote (6 lines)
>> + (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.

Toggle quote (9 lines)
>> +(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.

Toggle quote (21 lines)
>> + ;; 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@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
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEy/WXVcvn5+/vGD+x3UCaFdgiRp0FAlrQXzIACgkQ3UCaFdgi
Rp0ZxxAA0F5WDEhS5F50Mp0osoX+kiuwpD7ou7a0pIgDAGmEEM/fP/GJBVkBLoaj
msqiXu5pUfyiuUvMvsDUR14ferGD9bJU0TgyF/lXGYgy3zPOfE2H0fRN0tDWKAho
sF/yGvpSscW/TMg/qaqjshuazGAypB4kqDqvJTRucqNwWffFvY9i7iYNi0QGPI5/
4X33sS+/hZZSrezSJNwAyqLU4U7msG5pJWyjrgF4MsH6wGB6p2fefBpwHbYAXhtX
fsoThTQLfXhIlyNcXfChXOPa5EIJ1iJ15mBI5+TVBAM7LHSZtyvIBe/JgjeAzFbE
JNd5v3/49Oa1iXDRIWb5f9mElHG3jdZshWweRROGdSUZA58rvUMxpJ7CxQKt09jh
kU6LViDnsrIkRxxzCuKJCYXn2vLhiyfUR5DLEC0RWTJ36t/apgo2Hk/Hx/b4pVt3
Y90zs4WPWGA0+8jHWzRXihk4uCUFpnTTiyTxeVUWC5jYz9s4Pc/6bHL/j0r+4mM0
E8dR8ROqbDmcZsWz0Bw618ZVXi6z0b54jjJdir+tmCyyAtt+6g2GkKDY2Md/RDAj
Af/wPJZQ5s4rNHhu6jpw0676ZLDSbk6TJN9LW1YzC+e4ISEK0Qf1zxHqNegYtTem
fcXDcBY+fNEJJqHyUxiO28JcqVNTVtqUozA2FpSlpjGos0mAkAI=
=wDsf
-----END PGP SIGNATURE-----

C
C
Clément Lassieur wrote on 13 Apr 2018 11:02
(name . Chris Marusich)(address . cmmarusich@gmail.com)(address . 29732@debbugs.gnu.org)
87bmen32et.fsf@lassieur.org
Chris Marusich <cmmarusich@gmail.com> writes:

Toggle quote (33 lines)
> Hi Clément and Ludo,
>
> Thank you for the review!
>
> Clément Lassieur <clement@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

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

Which contains

Toggle snippet (27 lines)
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

Toggle quote (26 lines)
> 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.

Toggle quote (36 lines)
>>> +(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@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.

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

That's very ambitious! Take your time :-)
C
C
Chris Marusich wrote on 17 Apr 2018 07:51
(address . 29732@debbugs.gnu.org)
87po2yfkj3.fsf_-_@garuda.local.i-did-not-set--mail-host-address--so-tickle-me
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
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEy/WXVcvn5+/vGD+x3UCaFdgiRp0FAlrVi1AACgkQ3UCaFdgi
Rp23EA/+M0b8WFRg7CYP2my10Q0+e9hYbC8ZDoa8pevx9p464u+/Jj4BN/L8l8q3
DGxHHvwVc2h1C2sBpQHmDqOvaO46h3r+lARCCDq9tz1Jc47XEIvnTyJSR5X2PnAi
GX/2x5G0AbFXWxWDiNxmnM9zG1ISG1refhLtjKNctKwFkCxCZrco59Io1y7rp0Mv
E9cJoVDXxL9bqTe0nTYeWRwFsVX621GoBQtiQCtBo2lDR+nlrA+/VhJqbYhjElMj
rr5zF9W1fRYF+WIkEnfZJBx4PGTmCX/hbRZZyXS2Ohthtqp0j4FDlGw5oaQLGmJN
voKvH4rHrxgCRTnW5Q5VKjEnHOEioCNlGofUhJYtAyp516blvqVzBo25nVtJkZ63
qwsDOkLHF3LkFNqROti/L+JvEV5+d/pgVaMpKg+Dsyzybirhv0bXgJ0kvwZRIW2Z
VfNbRhEp++DiY8+QEM5OQDQlnLpa2sH/NRHYh5ZKvrQjVZH68GrUS/Oj6YnrTYQb
XyFxBGWcYDKivIaoB4Hxn5OldhBkwDWZpFHB2IwFMh7K3wwMeXj1ZQIzEREZqljV
G45Ms1S3bHP9q779kkqQrdLCtfufoXzQed+oJxn4K8TgSUZzmRYPXo6nPSyJHmsP
9Z+qntL/qcRsRRBZnI2uUooWDC36uIlS8Vfjtcxjigl+9fO4txI=
=iVW4
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 18 Apr 2018 22:28
(name . Chris Marusich)(address . cmmarusich@gmail.com)
87604ow972.fsf@gnu.org
Hello Chris,

Chris Marusich <cmmarusich@gmail.com> skribis:

Toggle quote (11 lines)
> 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.

Toggle quote (3 lines)
> 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!

Toggle quote (20 lines)
> From 5dd2e6853f1a332e55f3a4ba69b9baf199458fcb Mon Sep 17 00:00:00 2001
> From: Chris Marusich <cmmarusich@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’.
C
C
Chris Marusich wrote on 22 Apr 2018 09:44
(name . Ludovic Courtès)(address . ludo@gnu.org)
87lgdfy9bf.fsf@garuda.local.i-did-not-set--mail-host-address--so-tickle-me
ludo@gnu.org (Ludovic Courtès) writes:

Toggle quote (5 lines)
>> 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!

Toggle quote (2 lines)
> 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
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEy/WXVcvn5+/vGD+x3UCaFdgiRp0FAlrcPVQACgkQ3UCaFdgi
Rp3pFw/8DvbRmZq51s+sexYsMc9UqwicJpqSamccGfIejfILdR0skt8Ph6RawMBx
Kn0TkYDQful8eb8niEbqr2DUv9PLueIjw1VWLwA6y6KoKNcZbCr1wjQIirt8v8dd
IUM2xs6xN5zpR+cDdTeW+D4JgdWLVq0hImTXk2iXVaq02ys5+PJd0H3CWeXAaw42
YquFxdOGMPnhbCR1WQWcQZPgy1jQNIyAtvI7V5K/Ljwxrik5uOgMVMPVkBqIpCIl
C5Q5l75tiH4mtrR/qpZdroUbiDWgbEy4rMNDXw+2FOs5HpKbFGqx06/brKeXt/Nt
jFKdH2/pRCYi4nOA/MutPN4JQQelkVtN2bgH3lTDD6WDC6h+KImvJRajG0Lyo2mG
imRtWOifcjr+g5cAr/nodWxDgMtvzfYhFsBQ4wXd/aqajEWK9/6gzXbLBx7nnRbS
DfsHecuDS/NRg2i3ORWvbeqMboW53YN2kqirHr/TGxJzsZzb6skj2o/RfwI/DdTQ
ryhCVGowWCa1dw62DcgZ5YlqghjJdSVyHJeJCvSXNLSERaZ9v9iK5YYHw0+i7i92
64lfiYFiYB+/OaEJtYgN6Ekn1rtC1R7RYEMcAtQvqLA/1tpSwl4DZk9k2GRUkQwz
HBZJTafu9MmBrwplqotTn+MGPTtHqEKyJCISBY7W1bH2kcDox+k=
=Orpn
-----END PGP SIGNATURE-----

Closed
?