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

[Chris Marusich]

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?

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 <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!

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

-- 
Chris

signature.asc
Description: PGP signature