Hi Adolf, Thanks. I'd read that link before but missed changing the Subject line when I created the email. As you see, I've resubmitted it. Regards, Nick On 24/03/2024 15:38, Adolf Belka wrote: > > Hi Nick, > > You need to send the patch, as you have it below to the ipfire > development list. > > With PATCH in the subject line it will be recognised by the IPFire > development mailing list as a patch and will also be picked up in the > IPFire Patchwork system. > > https://patchwork.ipfire.org/project/ipfire/list/ > > As this email has a line that contains Subject: [PATCH] it has already > been picked up by Patchwork. You can check via the above link./ > > Then the patch will be reviewed on this mailing list. If accepted, it > will be merged by one of the devs and if not accepted you will get a > response as a reply to the patch so a history will be kept of the whole > patch review/discussion etc. > > There is a wiki page on patch submission for more details. > > https://www.ipfire.org/docs/devel/submit-patches > > Regards, > > Adolf. > > > On 24/03/2024 16:25, Nick Howitt wrote: >> Hi all, >> >> Please bear with me as I am new to IPFire and not at all used to this >> style of development, so any guidance would be appreciated. >> >> There is a bug, https://bugzilla.ipfire.org/show_bug.cgi?id=13254, >> where unbound-dhcp-leases-bridge restarts unbound a lot when it is >> totally unnecessary. It appears to be because when a lease is renewed, >> the script gets triggered, created a new /etc/unbound/dhcp-leases.conf >> then restarts Unbound to read the file. Generally this seems to be >> unnecessary as, with a renewed lease, the old and new >> /etc/unbound/dhcp-leases.conf files are the same. With 5 leases in >> that file (one for a machine not active) I am getting 3-4 restarts an >> hour of Unbound when I have a min/max lease time of 60/120min. >> >> Looking at the code, it is fairly easy to fix. The current code >> creates a temp file then copies it into place then restarts unbound. >> All I am doing is doing a file comparison before the copy and skipping >> the restart if the files are the same. >> >> There were a couple of gotchas because setting the file attributes and >> copying the file were done inside the "with" block for generating the >> temporary file. This meant a file comparison always returned False as >> the temp file was still open and so never the same is the old file. >> >> I moved those two statements outside the "with". This forced me to >> change the fchmod to chmod. >> >> It could be argued that the file copy should not be done if the files >> are not different, but it would take an extra "if" and you'd have to >> remember to delete the temp file. If required, I can make that change. >> >> Also, one small thing I noticed once is that the old and new >> dhcp-leases.conf files could occasionally contain the same leases but >> in a different order. I have been unable to reproduce, but to sidestep >> it, instead of stepping through the leases variable directly, I sort >> it and step through that. It should make the resulting file completely >> deterministic and make the file comparison more effective. >> >> My patch is: >> >> From 73873d4944944a2f02317a73074a6894726f36f7 Mon Sep 17 00:00:00 2001 >> From: Nick Howitt >> Date: Sun, 24 Mar 2024 15:17:19 +0000 >> Subject: [PATCH] Stop unbound-dhcp-leases-bridge from restarting >> unbound every >>  time it write dhcp-leases.conf as it is very often unchanged and does >> not >>  require a restart. >> >> --- >>  config/unbound/unbound-dhcp-leases-bridge | 28 +++++++++++++++-------- >>  1 file changed, 19 insertions(+), 9 deletions(-) >> >> diff --git a/config/unbound/unbound-dhcp-leases-bridge >> b/config/unbound/unbound-dhcp-leases-bridge >> index e9f022aff..d22772066 100644 >> --- a/config/unbound/unbound-dhcp-leases-bridge >> +++ b/config/unbound/unbound-dhcp-leases-bridge >> @@ -22,6 +22,7 @@ >>  import argparse >>  import datetime >>  import daemon >> +import filecmp >>  import functools >>  import ipaddress >>  import logging >> @@ -516,26 +517,35 @@ class UnboundConfigWriter(object): >> >>      def update_dhcp_leases(self, leases): >>          # Write out all leases >> -        self.write_dhcp_leases(leases) >> +        if self.write_dhcp_leases(leases): >> >> -        log.debug("Reloading Unbound...") >> +            log.debug("Reloading Unbound...") >> >> -        # Reload the configuration without dropping the cache >> -        self._control("reload_keep_cache") >> +            # Reload the configuration without dropping the cache >> +            self._control("reload_keep_cache") >> + >> +        else: >> + >> +            log.debug("Not reloading Unbound. Leases not changed.") >> >>      def write_dhcp_leases(self, leases): >>          log.debug("Writing DHCP leases...") >> >>          with tempfile.NamedTemporaryFile(mode="w", delete=False) as f: >> -            for l in leases: >> +            for l in sorted(leases): >>                  for rr in l.rrset: >>                      f.write("local-data: \"%s\"\n" % " ".join(rr)) >> >> -            # Make file readable for everyone >> -            os.fchmod(f.fileno(), >> stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat.S_IROTH) >> +        filecmp.clear_cache() >> +        RequireUnboundReload = not filecmp.cmp(f.name, self.path, >> shallow=False) >> + >> +        # Make file readable for everyone >> +        os.chmod(f.name, >> stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat.S_IROTH) >> + >> +        # Move the file to its destination >> +        os.rename(f.name, self.path) >> >> -            # Move the file to its destination >> -            os.rename(f.name, self.path) >> +        return RequireUnboundReload >> >>      def _control(self, *args): >>          command = ["unbound-control"] >