[bug#29732] [PATCH 1/1] services: Add dhcpd-service-type and <dhcpd-configuration>.

[Clément Lassieur]

Chris Marusich <address@hidden> writes:

> Hi Clément and Ludo,
>
> Thank you for the review!
>
> Clément Lassieur <address@hidden> 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 <address@hidden> 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 :-)