Package: guix-patches;
Reported by: Alexey Abramov <levenson <at> mmer.org>
Date: Sun, 23 Mar 2025 10:27:04 UTC
Severity: normal
Tags: patch
Done: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Bug is archived. No further changes may be made.
Message #56 received at 77204 <at> debbugs.gnu.org (full text, mbox):
From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com> To: Alexey Abramov <levenson <at> mmer.org> Cc: Ludovic Courtès <ludo <at> gnu.org>, 77204 <at> debbugs.gnu.org Subject: Re: [bug#77204] [PATCH v3 3/3] services: dnsmasq: Add stats and reload shepherd actions. Date: Tue, 22 Apr 2025 15:20:10 +0900
Hi again,
Alexey Abramov <levenson <at> mmer.org> 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
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.