From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49090) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1f6ubr-0003Wb-N1 for guix-patches@gnu.org; Fri, 13 Apr 2018 05:03:09 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1f6ubn-0000iP-0n for guix-patches@gnu.org; Fri, 13 Apr 2018 05:03:07 -0400 Received: from debbugs.gnu.org ([208.118.235.43]:41176) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1f6ubm-0000iF-SB for guix-patches@gnu.org; Fri, 13 Apr 2018 05:03:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1f6ubm-0004cO-HF for guix-patches@gnu.org; Fri, 13 Apr 2018 05:03:02 -0400 Subject: [bug#29732] [PATCH 1/1] services: Add dhcpd-service-type and . Resent-Message-ID: References: <20171216083528.2081-1-cmmarusich@gmail.com> <20171216085242.2309-1-cmmarusich@gmail.com> <871sh6rafl.fsf@lassieur.org> <87efjjwo25.fsf@gmail.com> From: =?UTF-8?Q?Cl=C3=A9ment?= Lassieur In-reply-to: <87efjjwo25.fsf@gmail.com> Date: Fri, 13 Apr 2018 11:02:02 +0200 Message-ID: <87bmen32et.fsf@lassieur.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+kyle=kyleam.com@gnu.org Sender: "Guix-patches" To: Chris Marusich Cc: 29732@debbugs.gnu.org Chris Marusich writes: > Hi Clément and Ludo, > > Thank you for the review! > > Clément Lassieur 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 >>> + (($ 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 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 :-)