Subject: [bug#61869] Review of the third patch

From: "Wicki Gabriel (wicg)" <[email-masked]>
Date: Mon, 21 Aug 2023 15:12:04 +0000

Thank you very much for your review and the third patch!

I have 3 small additions to your work (see your adapted patch in the attachments):

  *   I added a commit message (although I guess you'll edit that again anyways)
  *   I extended your explanations on the `interface' option (when bind-dynamic​?` or​ bind-interfaces?​ is set...)
  *   I filled in your TODO

Please let me know (don't forget to cc me in your reply) if I can do anything else to merge this patch-set.

Thanks again, for your time, patience and expertise

gabriel
From a6a696994f7d5ef58f85b4aaa14c088428e49353 Mon Sep 17 00:00:00 2001
Message-Id: <[email-masked]>
In-Reply-To: <[email-masked]>
References: <[email-masked]>
From: Gabriel Wicki <[email-masked]>
Date: Mon, 21 Aug 2023 16:57:19 +0200
Subject: [PATCH 3/3] services: dnsmasq: Fix inconsistencies.

* doc/guix.texi: Adapt the service documentation to reflect dnsmasq's
man-pages.
* gnu/services/dns.scm (<dnsmasq-configuration>): [interface, domain,
dhcp-range, dhcp-options] Rename fields to reflect object type (i.e. plural
for lists).  [local] Remove option (this is an alias for servers).
[bind-interfaces?] New option.
---
 doc/guix.texi        | 100 ++++++++++++++++++++++++++++++++++---------
 gnu/services/dns.scm |  47 ++++++++++----------
 2 files changed, 103 insertions(+), 44 deletions(-)

diff --git a/doc/guix.texi b/doc/guix.texi
index ba8f8a52ce..1a65d33228 100644
--- a/doc/guix.texi
+++ b/doc/guix.texi
@@ -32472,42 +32472,102 @@ DNS Services
 queries which are forwarded upstream.
 
 @item @code{domain-needed?} (default: @code{#f})
-Whether to forward queries with no domain part.
+@code{#t} tells dnsmasq to never forward A or AAAA queries for plain
+names, without dots or domain parts, to upstream nameservers.
+If the name is not known from @file{/etc/hosts} or DHCP then a ``not found''
+answer is returned.
 
 @item @code{bogus-priv?} (default: @code{#f})
-Whether to fake reverse lookups for RFC1918 private address ranges.
+Whether to fake bogus reverse lookups for RFC6303 private address ranges.
+All reverse lookups for private IP ranges (e.g. 192.168.x.x) which are not
+found in @file{/etc/hosts} or the DHCP leases file are answered with
+``no such domain'' rather than being forwarded upstream.
 
 @item @code{filterwin2k?} (default: @code{#f})
-Whether to forward spurious DNS requests from Windows hosts.
+Whether to block spurious DNS requests periodically made by hosts running
+Windows, which don't get sensible answers from the public DNS.
 
 @item @code{poll?} (default: @code{#t})
-Continuously reads @file{/etc/resolv.conf} when @code{#true}, otherwise only
-does so on SIGHUP.
+When @code{#t}, continuously check @file{/etc/resolv.conf} for changes.
+Otherwise do so only on SIGHUP.
 
-@item @code{local} (default: @code{#f})
-A string representing domains where nothing will be forwarded to
-@code{"/domain/"}).
+@item @code{interfaces} (default: @code{'()})
+Listen only on the specified list of interfaces, such as @code{"wlp3"}
+or @code{"lan0"}.  The local (loopback) interface is added automatically.
+If this list is empty, listen on all available interfaces.
 
-@item @code{interface} (default: @code{#f})
-The interface(s) dnsmasq works on (like @code{"wlp3"} or @code{"lan0"}.
-Multiple names can be specified as strings separated by commas.
+When @code{bind-dynamic?} or @code{bind-interfaces?} is set on Linux, IP
+alias interface labels such as @code{"eth1:0"} will be checked rather
+than interface names.
+
+@item @code{bind-interfaces?} (default: @code{#f})
+When @code{#t} on systems which support it, dnsmasq binds the wildcard
+address, to discard requests for interfaces it is not supposed to reply
+to later.  This should only be necessary when running another nameserver
+(or another instance of dnsmasq) on the same machine.
 
 @item @code{bind-dynamic?} (default: @code{#f})
-Bind to interfaces in use - check for new interfaces.
+When @code{#t}, bind to the address of the network interfaces
+currently in use, allowing for multiple Dnsmasq instances.
+Moreover, automatically listen on any new interfaces or addresses
+that appear later on---subject to access control configuration.
+
+This mode is available only on Linux.  On other kernels, it will fall
+back to @code{bind-interfaces?} mode.
 
 @item @code{expand-hosts?} (default: @code{#f})
-Expand simple names in @file{/etc/hosts} with domain-suffix.
+Add the domain to simple names (those without a period) in
+@file{/etc/hosts} in the same way as for DHCP-derived names.  This does
+not apply to domain names in cnames, PTR records, TXT records, etc.
 
-@item @code{domain} (default: @code{#f})
-Specify the domain to be assigned in DHCP leases.
+@item @code{domains} (default: @code{'()})
+A list of strings describing DNS domains for the DHCP server.  Domains
+may be be given unconditionally (without the IP range) or for limited
+IP ranges.
+
+This has two effects; firstly it causes the DHCP server to return the
+domain to any hosts which request it, and secondly it sets the domain
+which it is legal for DHCP-configured hosts to claim.
+
+If a domain suffix is specified, then hostnames with a domain part are
+allowed, provided the domain part matches the suffix.  Hostnames
+without a domain part have the suffix added as an optional domain part.
+
+If no domain suffix is specified, then any DHCP hostname with a domain
+part (i.e., with a period) will be disallowed and logged.
 
-@item @code{dhcp-range} (default: @code{#f})
-Enable DHCP in the range given with lease duration, the format is
-@code{<START-IP>,<END-IP>,<MASK>,<LEASE-TIME>}, e.g.
-@code{192.0.2.50,192.0.2.150,255.255.255.0,1h}.
+@item @code{dhcp-ranges} (default: @code{'()})
+Whether to enable the DHCP server for the given range(s).  Each range
+is a string that commonly follows the format
+@code{<start-addr>,<end-addr>[,<prefix-length>[,<lease-time>]]}, e.g.
+@code{"192.0.2.50,192.0.2.150,255.255.255.0,1h"}.
+
+IP addresses will be given out (``leased'') from the range
+@code{<start-addr>} to @code{<end-addr>}, with an optional
+@code{<prefix-length>}.
+@comment …and from statically defined addresses given in --dhcp-host options.
+
+@code{<lease-time>} is optional.  If given, leases will be valid
+for that length of time: in seconds if no unit is given (e.g.,
+@code{45m}, @code{12h}, @code{7d}, @code{1w}), or @code{infinite}.
+
+Refer to the dnsmasq(8) man page for more options and information.
 
 @item @code{dhcp-options} (default: @code{'()})
-A list of options to be passed along.
+A list of DHCP option strings as listed in the output of
+@command{dnsmasq --help dhcp} and @command{dnsmasq --help dhcp6}.
+
+You can use both numerical options and their readable names:
+
+@lisp
+;; This friendly list…
+(list "option:router,1.2.3.4"
+      "option6:dns-server,[::],[1234::88]")
+
+;; …is equivalent to this one.
+(list "3,1.2.3.4"
+      "23,[::],[1234::88]")
+@end lisp
 
 @item @code{tftp-enable?} (default: @code{#f})
 Whether to enable the built-in TFTP server.
diff --git a/gnu/services/dns.scm b/gnu/services/dns.scm
index 17dec9ad8a..33af84e621 100644
--- a/gnu/services/dns.scm
+++ b/gnu/services/dns.scm
@@ -5,6 +5,7 @@
 ;;; Copyright © 2021 Maxime Devos <[email-masked]>
 ;;; Copyright © 2022 Remco van 't Veer <[email-masked]>
 ;;; Copyright © 2023 Gabriel Wicki <[email-masked]>
+;;; Copyright © 2023 Tobias Geerinckx-Rice <[email-masked]>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -774,19 +775,19 @@ (define-record-type* <dnsmasq-configuration>
                     (default #f))       ;boolean
   (poll?            dnsmasq-configuration-poll?
                     (default #t))       ;boolean
-  (local            dnsmasq-configuration-local
-                    (default #f))       ;string
-  (interface        dnsmasq-configuration-interface
-                    (default #f))       ;string
+  (interfaces       dnsmasq-configuration-interfaces
+                    (default '()))      ;list of string
+  (bind-interfaces? dnsmasq-configuration-bind-interfaces?
+                    (default #f))       ;boolean
   (bind-dynamic?    dnsmasq-configuration-bind-dynamic?
                     (default #f))       ;boolean
   (expand-hosts?    dnsmasq-configuration-expand-hosts?
                     (default #f))       ;boolean
-  (domain           dnsmasq-configuration-domain
-                    (default #f))       ;string
-  (dhcp-range       dnsmasq-configuration-dhcp-range
-                    (default #f))       ;string
-  (dhcp-options      dnsmasq-configuration-dhcp-options
+  (domains          dnsmasq-configuration-domains
+                    (default '()))      ;list of string
+  (dhcp-ranges      dnsmasq-configuration-dhcp-ranges
+                    (default '()))      ;list of string
+  (dhcp-options     dnsmasq-configuration-dhcp-options
                     (default '()))      ;list of string
   (tftp-enable?     dnsmasq-configuration-tftp-enable?
                     (default #f))       ;boolean
@@ -822,10 +823,10 @@ (define (dnsmasq-shepherd-service config)
      cache-size negative-cache?
      cpe-id
      domain-needed? bogus-priv? filterwin2k? poll?
-     local interface
-     bind-dynamic? expand-hosts?
-     domain
-     dhcp-range dhcp-options
+     interfaces
+     bind-interfaces? bind-dynamic? expand-hosts?
+     domains
+     dhcp-ranges dhcp-options
      tftp-enable? tftp-no-fail?
      tftp-single-port? tftp-secure?
      tftp-max tftp-mtu tftp-no-blocksize?
@@ -885,11 +886,11 @@ (define (dnsmasq-shepherd-service config)
                 #$@(if poll?
                        '()
                        '("--no-poll"))
-                #$@(if local
-                       (list (format #f "--local=~a" local))
-                       '())
-                #$@(if interface
-                       (list (format #f "--interface=~a" interface))
+                #$@(if (null? interfaces)
+		       '()
+                       (list (format #f "--interface=~{~a~^,~}" interfaces)))
+                #$@(if bind-interfaces?
+                       '("--bind-interfaces")
                        '())
                 #$@(if bind-dynamic?
                        '("--bind-dynamic")
@@ -897,12 +898,10 @@ (define (dnsmasq-shepherd-service config)
                 #$@(if expand-hosts?
                        '("--expand-hosts")
                        '())
-                #$@(if domain
-                       (list (format #f "--domain=~a" domain))
-                       '())
-                #$@(if dhcp-range
-                       (list (format #f "--dhcp-range=~a" dhcp-range))
-                       '())
+                #$@(map (cut format #f "--domain=~a" <>)
+                        domains)
+                #$@(map (cut format #f "--dhcp-range=~a" <>)
+                        dhcp-ranges)
                 #$@(map (cut format #f "--dhcp-option=~a" <>)
                         dhcp-options)
                 #$@(if tftp-enable?
-- 
2.40.1