Subject: [bug#77204] [PATCH v3 3/3] services: dnsmasq: Add stats and reload shepherd actions.

From: Maxim Cournoyer <[email-masked]>
Date: Tue, 22 Apr 2025 15:20:10 +0900

Hi again,

Alexey Abramov <[email-masked]> writes:

> * gnu/services/dns.scm (dnsmasq-service-reload-action): New function to
>   implement SIGHUP handling for reloading configurations.
> * gnu/services/dns.scm (dnsmasq-service-stats-action): New function to
>   implement SIGUSR1 handling for dumping statistics.
> * gnu/services/dns.scm (dnsmasq-shepherd-service): Use the new actions.
> * doc/guix.texi: Document the new actions with examples.
> * gnu/tests/networking.scm (%test-dnsmasq): Add tests to verify the
>   functionality of the new actions.

Looks nice! Same comment as earlier: file names should appear once.

> ---
>  doc/guix.texi            | 10 ++++
>  gnu/services/dns.scm     | 22 +++++++++
>  gnu/tests/networking.scm | 98 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 130 insertions(+)
>
> diff --git a/doc/guix.texi b/doc/guix.texi
> index 1bbd1824a87..816a9ed57d0 100644
> --- a/doc/guix.texi
> +++ b/doc/guix.texi
> @@ -35060,6 +35060,16 @@ DNS Services
>  @end lisp
>  @end defvar

The newly added dnsmasq-serice-type should be nested into the existing
'defvar' for it.

> +@code{dnsmasq-service-type} also provides few helpful actions which are

Then, I'd just write: Two actions are provided:

> +@code{reload} and @code{stats}.  For example:
> +
> +@example
> +herd stats dnsmasq
> +@end example
> +
> +Will ask @command{dnsmasq} service to dump its statistics to the system log, which
> +is usually @file{/var/log/messages}.

s/is usually//  (is it configurable to be elsewhere -- I don't think
so?)

> +
>  @deftp {Data Type} dnsmasq-configuration
>  Data type representing the configuration of dnsmasq.
>  
> diff --git a/gnu/services/dns.scm b/gnu/services/dns.scm
> index 210fef4ece2..f96d6dbb158 100644
> --- a/gnu/services/dns.scm
> +++ b/gnu/services/dns.scm
> @@ -873,6 +873,8 @@ (define (dnsmasq-shepherd-service config)
>       (provision (or provision shepherd-provision))
>       (requirement shepherd-requirement)
>       (documentation "Run the dnsmasq DNS server.")
> +     (actions (list (dnsmasq-service-reload-action config)
> +                    (dnsmasq-service-stats-action config)))
>       (start #~(make-forkexec-constructor
>                 (list
>                  #$(file-append package "/sbin/dnsmasq")
> @@ -964,6 +966,26 @@ (define (dnsmasq-activation config)
>        ;; create directory to store dnsmasq lease file
>        (mkdir-p "/var/lib/misc")))
>  
> +(define (dnsmasq-service-reload-action config)
> +  (match-record config <dnsmasq-configuration> ()
> +    (shepherd-action
> +     (name 'reload)
> +     (documentation "Send a SIGHUP signal to re-load /etc/hosts and /etc/ethers and any
> +file given by --dhcp-hostsfile, --dhcp-hostsdir, --dhcp-optsfile, --dhcp-optsdir,
> +--addn-hosts or --hostsdir.  SIGHUP does NOT re-read the configuration file.")

s/re-load/reload/
s/re-read/reload/

Please mind maximum 80 columns coding style; I believe this is also to
be formatted as Texinfo so you could use @file{/etc/hosts} and
@file{/etc/ethers} for example, and @option for each listed options.
Instead of emphasising with full caps, you can use @emph{not}.

> +     (procedure #~(lambda (running)
> +                    (let ((pid (process-id running)))
> +                      (kill pid SIGHUP)))))))
> +
> +(define (dnsmasq-service-stats-action config)
> +  (match-record config <dnsmasq-configuration> ()
> +    (shepherd-action
> +     (name 'stats)
> +     (documentation "Send a SIGUSR1 to write statistics to the system log.")
> +     (procedure #~(lambda (running)
> +                    (let ((pid (process-id running)))
> +                      (kill pid SIGUSR1)))))))
> +
>  (define dnsmasq-service-type
>    (service-type
>     (name 'dnsmasq)
> diff --git a/gnu/tests/networking.scm b/gnu/tests/networking.scm
> index 7d54ebba50e..fdc515ceb04 100644
> --- a/gnu/tests/networking.scm
> +++ b/gnu/tests/networking.scm
> @@ -27,6 +27,7 @@ (define-module (gnu tests networking)
>    #:use-module (gnu system vm)
>    #:use-module (gnu services)
>    #:use-module (gnu services base)
> +  #:use-module (gnu services dns)
>    #:use-module (gnu services networking)
>    #:use-module (guix gexp)
>    #:use-module (guix store)
> @@ -46,6 +47,7 @@ (define-module (gnu tests networking)
>              %test-openvswitch
>              %test-dhcpd
>              %test-dhcpcd
> +            %test-dnsmasq
>              %test-tor
>              %test-iptables
>              %test-ipfs))
> @@ -675,6 +677,102 @@ (define %test-dhcpd
>     (description "Test a running DHCP daemon configuration.")
>     (value (run-dhcpd-test))))
>  
> +
> +
> +;;;
> +;;; dnsmasq tests
> +;;;
> +
> +
> +(define dnsmasq-os-configuration
> +  (dnsmasq-configuration))
> +
> +(define %dnsmasq-os
> +  (simple-operating-system
> +   (service dhcp-client-service-type)
> +   (service dnsmasq-service-type
> +            (dnsmasq-configuration
> +             (extra-options
> +              (list "--log-facility=/tmp/dnsmasq.log"))))))
> +
> +
> +(define (run-dnsmasq-test)
> +  (define os
> +    (marionette-operating-system %dnsmasq-os
> +                                 #:imported-modules '((gnu services herd))))
> +
> +  (define test
> +    (with-imported-modules '((gnu build marionette))
> +      #~(begin
> +          (use-modules (gnu build marionette)
> +                       (srfi srfi-64))
> +
> +          (define marionette
> +            (make-marionette (list #$(virtual-machine os))))
> +
> +          (test-runner-current (system-test-runner #$output))
> +          (test-begin "dnsmasq")
> +
> +          (test-assert "dnsmasq is alive"
> +            (marionette-eval
> +             '(begin
> +                (use-modules (gnu services herd))
> +                (wait-for-service 'dnsmasq))
> +             marionette))
> +
> +          (test-assert "pid file exists"
> +            (wait-for-file
> +             '#$(dnsmasq-configuration-pid-file dnsmasq-os-configuration)
> +             marionette))
> +
> +          (test-assert "send SIGHUP"
> +            (positive?
> +             (marionette-eval
> +              '(begin
> +                 (use-modules (ice-9 rdelim))
> +                 (system* "sync")
> +                 (let* ((port (open-input-file "/tmp/dnsmasq.log")))
> +                   (seek port 0 SEEK_END)
> +                   (system* "herd" "reload" "dnsmasq")
> +                   (system* "sync")
> +                   (let ((line (read-line port)))
> +                     (close-port port)
> +                     (string-contains line "read /etc/hosts"))))
> +              marionette)))

Interesting!

> +          (test-assert "send SIGUSR1"
> +            (positive?
> +             (marionette-eval
> +              '(begin
> +                 (use-modules (ice-9 rdelim))
> +                 (system* "sync")
> +                 (let* ((port (open-input-file "/tmp/dnsmasq.log")))
> +                   (seek port 0 SEEK_END)
> +                   (system* "herd" "stats" "dnsmasq")
> +                   (system* "sync")
> +                   (let ((line (read-line port)))
> +                     (close-port port)
> +                     (string-contains-ci line "time"))))
> +              marionette)))
> +
> +          (test-assert "dnsmasq is alive"
> +            (marionette-eval
> +             '(begin
> +                (use-modules (gnu services herd))
> +                (wait-for-service 'dnsmasq))
> +             marionette))
> +
> +          (test-end))))
> +
> +  (gexp->derivation "dnsmasq-test" test))

That looks like a useful series.  Could you please send a v4,
integrating my requested changes?

-- 
Thanks,
Maxim