Stop unbound-dhcp-leases-bridge from continually restarting unbound

  Hi all,

**** This is a repost of an email incorrectly submitted. ****

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 <nick@howitts.co.uk>
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(-)


      def _control(self, *args):
          command = ["unbound-control"]
  

If there is a risk that the /etc/unbound/dhcp-leases.conf does not 
exist, for example on a clean install (I don't know), the filecmp.cmp 
line needs to be wrapped in a try...except to stop it blowing up:

try:
	RequireUnboundReload = not filecmp.cmp(f.name, self.path, shallow=False)
except:
	RequireUnboundReload = True

This will force the file to be to be generated, even if it is only 
copying in a blank temp file, and unbound will restart. It will only 
ever happen once.


On 24/03/2024 15:49, Nick Howitt wrote:
> 
> Hi all,
> 
> **** This is a repost of an email incorrectly submitted. ****
> 
> 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 <nick@howitts.co.uk>
> 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"]
  

Hello,

I don’t think that the risk is very high, but I would account for this problem.

With my proposed changes from my previous email this should not be a large problem at all. When opening /etc/unbound/dhcp-leases.conf with mode “r+” you can catch the Exception and skip the comparison and open the file again with “w”.

> On 25 Mar 2024, at 10:10, Nick Howitt <nick@howitts.co.uk> wrote:
> 
> If there is a risk that the /etc/unbound/dhcp-leases.conf does not exist, for example on a clean install (I don't know), the filecmp.cmp line needs to be wrapped in a try...except to stop it blowing up:
> 
> try:
> RequireUnboundReload = not filecmp.cmp(f.name, self.path, shallow=False)
> except:
> RequireUnboundReload = True

I generally don’t like blanco exception catches. You will never know about the daemons that you are hiding.

> This will force the file to be to be generated, even if it is only copying in a blank temp file, and unbound will restart. It will only ever happen once.
> 
> 
> On 24/03/2024 15:49, Nick Howitt wrote:
>> Hi all,
>> **** This is a repost of an email incorrectly submitted. ****
>> 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 <nick@howitts.co.uk>
>> 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"]
  

On 25/03/2024 11:31, Michael Tremer wrote:
> 
> Hello,
> 
> I don’t think that the risk is very high, but I would account for this problem.
> 
> With my proposed changes from my previous email this should not be a large problem at all. When opening /etc/unbound/dhcp-leases.conf with mode “r+” you can catch the Exception and skip the comparison and open the file again with “w”.
> 
>> On 25 Mar 2024, at 10:10, Nick Howitt <nick@howitts.co.uk> wrote:
>>
>> If there is a risk that the /etc/unbound/dhcp-leases.conf does not exist, for example on a clean install (I don't know), the filecmp.cmp line needs to be wrapped in a try...except to stop it blowing up:
>>
>> try:
>> RequireUnboundReload = not filecmp.cmp(f.name, self.path, shallow=False)
>> except:
>> RequireUnboundReload = True
> 
> I generally don’t like blanco exception catches. You will never know about the daemons that you are hiding.
> 
>> This will force the file to be to be generated, even if it is only copying in a blank temp file, and unbound will restart. It will only ever happen once.
>>
>>
>> On 24/03/2024 15:49, Nick Howitt wrote:
>>> Hi all,
>>> **** This is a repost of an email incorrectly submitted. ****
>>> 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 <nick@howitts.co.uk>
>>> 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"]
> 
Resubmitting the patch.

Following my previous reply, I would like to stick with filecmp as I am 
using it with "shallow=False" to avoid doing the comparison just with 
stat() calls. Note that I also flush the filecmp cache before using the 
command so it is running with the latest file data. Talking about file 
descriptors, atomic file replacement etc., scares me.

I have modified the earlier try/except I proposed to catch only 
FileNotFoundError, and I have changed CamelCase to snake_case.

---
  config/unbound/unbound-dhcp-leases-bridge | 31 ++++++++++++++++-------
  1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/config/unbound/unbound-dhcp-leases-bridge 
b/config/unbound/unbound-dhcp-leases-bridge
index e9f022aff..a9ec8e1e2 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,38 @@ 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()
+		try:
+			require_unbound_reload = not filecmp.cmp(f.name, self.path, 
shallow=False)
+		except FileNotFoundError:
+			require_unbound_reload = True
+
+		# 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 require_unbound_reload

  	def _control(self, *args):
  		command = ["unbound-control"]
  

> On 25 Mar 2024, at 13:11, Nick Howitt <nick@howitts.co.uk> wrote:
> 
> 
> 
> On 25/03/2024 11:31, Michael Tremer wrote:
>> Hello,
>> I don’t think that the risk is very high, but I would account for this problem.
>> With my proposed changes from my previous email this should not be a large problem at all. When opening /etc/unbound/dhcp-leases.conf with mode “r+” you can catch the Exception and skip the comparison and open the file again with “w”.
>>> On 25 Mar 2024, at 10:10, Nick Howitt <nick@howitts.co.uk> wrote:
>>> 
>>> If there is a risk that the /etc/unbound/dhcp-leases.conf does not exist, for example on a clean install (I don't know), the filecmp.cmp line needs to be wrapped in a try...except to stop it blowing up:
>>> 
>>> try:
>>> RequireUnboundReload = not filecmp.cmp(f.name, self.path, shallow=False)
>>> except:
>>> RequireUnboundReload = True
>> I generally don’t like blanco exception catches. You will never know about the daemons that you are hiding.
>>> This will force the file to be to be generated, even if it is only copying in a blank temp file, and unbound will restart. It will only ever happen once.
>>> 
>>> 
>>> On 24/03/2024 15:49, Nick Howitt wrote:
>>>> Hi all,
>>>> **** This is a repost of an email incorrectly submitted. ****
>>>> 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 <nick@howitts.co.uk>
>>>> 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"]
> Resubmitting the patch.
> 
> Following my previous reply, I would like to stick with filecmp as I am using it with "shallow=False" to avoid doing the comparison just with stat() calls. Note that I also flush the filecmp cache before using the command so it is running with the latest file data. Talking about file descriptors, atomic file replacement etc., scares me.

File descriptors are great. They avoid race-conditions and all sorts of things.

I don’t think that module is the best choice really if you have to hack around and flush the cache. Why would it even cache things in the first place? I don’t see where the cache helps.

> I have modified the earlier try/except I proposed to catch only FileNotFoundError, and I have changed CamelCase to snake_case.
> 
> ---
> config/unbound/unbound-dhcp-leases-bridge | 31 ++++++++++++++++-------
> 1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge
> index e9f022aff..a9ec8e1e2 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,38 @@ 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()
> + try:
> + require_unbound_reload = not filecmp.cmp(f.name, self.path, shallow=False)
> + except FileNotFoundError:
> + require_unbound_reload = True

If you want to go this route, you will have to flush the temporary file first. Otherwise you might have some data left in the buffer that has not been written (because the file is still open). You will then compare incomplete output. This would not become a problem if you used the file descriptors.

Catching FileNotFound would work for me.

> +
> + # 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 require_unbound_reload
> 
> def _control(self, *args):
> command = ["unbound-control"]
> -- 
> 2.43.0
> 
> 
> Nick
  

On 26/03/2024 11:10, Michael Tremer wrote:
>
>
>> On 25 Mar 2024, at 13:11, Nick Howitt <nick@howitts.co.uk> wrote:
>>
>>
>>
>> On 25/03/2024 11:31, Michael Tremer wrote:
>>> Hello,
>>> I don’t think that the risk is very high, but I would account for this problem.
>>> With my proposed changes from my previous email this should not be a large problem at all. When opening /etc/unbound/dhcp-leases.conf with mode “r+” you can catch the Exception and skip the comparison and open the file again with “w”.
>>>> On 25 Mar 2024, at 10:10, Nick Howitt <nick@howitts.co.uk> wrote:
>>>>
>>>> If there is a risk that the /etc/unbound/dhcp-leases.conf does not exist, for example on a clean install (I don't know), the filecmp.cmp line needs to be wrapped in a try...except to stop it blowing up:
>>>>
>>>> try:
>>>> RequireUnboundReload = not filecmp.cmp(f.name, self.path, shallow=False)
>>>> except:
>>>> RequireUnboundReload = True
>>> I generally don’t like blanco exception catches. You will never know about the daemons that you are hiding.
>>>> This will force the file to be to be generated, even if it is only copying in a blank temp file, and unbound will restart. It will only ever happen once.
>>>>
>>>>
>>>> On 24/03/2024 15:49, Nick Howitt wrote:
>>>>> Hi all,
>>>>> **** This is a repost of an email incorrectly submitted. ****
>>>>> 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 <nick@howitts.co.uk>
>>>>> 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"]
>> Resubmitting the patch.
>>
>> Following my previous reply, I would like to stick with filecmp as I am using it with "shallow=False" to avoid doing the comparison just with stat() calls. Note that I also flush the filecmp cache before using the command so it is running with the latest file data. Talking about file descriptors, atomic file replacement etc., scares me.
> File descriptors are great. They avoid race-conditions and all sorts of things.
But I have never used them so I don't intend to go that way. Too much 
for me to get my brain round and a limited ability to test.
>
> I don’t think that module is the best choice really if you have to hack around and flush the cache. Why would it even cache things in the first place? I don’t see where the cache helps.
 From the filecmp.clear_cache documentation:

    Clear the filecmp cache. This may be useful if a file is compared so
    quickly after it is modified that it is within the mtime resolution
    of the underlying filesystem.

Now this makes me believe that the clear_cache is only relevant when 
using shallow=True, which I'm not and I don't know how to check the 
underlying code. It is used to make sure the stat() info is updated, but 
we are not using the stat() info. I guess we could as easily add a one 
second sleep instead as that is way longer than the mtime resolution.

The module is chosen because all the references I read about comparing 
files preferred this one-liner for comparing files, although other 
methods were given and there were lots of discussions about large files 
(GB+) because if you chomp them it may be much quicker to say they are 
not the same when you find the first block that does not match. Having 
said this, I don't know how filecmp works, but we only have a small 
file. At the same time, I am not a regular programmer so simple 
one-liners are good for me.
>> I have modified the earlier try/except I proposed to catch only FileNotFoundError, and I have changed CamelCase to snake_case.
>>
>> ---
>> config/unbound/unbound-dhcp-leases-bridge | 31 ++++++++++++++++-------
>> 1 file changed, 22 insertions(+), 9 deletions(-)
>>
>> diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge
>> index e9f022aff..a9ec8e1e2 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,38 @@ 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()
>> + try:
>> + require_unbound_reload = not filecmp.cmp(f.name, self.path, shallow=False)
>> + except FileNotFoundError:
>> + require_unbound_reload = True
> If you want to go this route, you will have to flush the temporary file first. Otherwise you might have some data left in the buffer that has not been written (because the file is still open). You will then compare incomplete output. This would not become a problem if you used the file descriptors.
The temp file is closed at the point of doing the filecmp, and the 
filecmp cache has been flushed. How do I flush the temporary file 
further. Reading up, is it just having the last statement in the with 
block as f.flush() with the same indent as "for l in sorted(leases):"
>
> Catching FileNotFound would work for me.
Good
>
>> +
>> + # 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 require_unbound_reload
>>
>> def _control(self, *args):
>> command = ["unbound-control"]
>> -- 
>> 2.43.0
>>
>>
>> Nick
  

Hello,

> On 27 Mar 2024, at 15:28, Nick Howitt <nick@howitts.co.uk> wrote:
> 
> 
> 
> On 26/03/2024 11:10, Michael Tremer wrote:
>> 
>> 
>>> On 25 Mar 2024, at 13:11, Nick Howitt <nick@howitts.co.uk> wrote:
>>> 
>>> 
>>> 
>>> On 25/03/2024 11:31, Michael Tremer wrote:
>>>> Hello,
>>>> I don’t think that the risk is very high, but I would account for this problem.
>>>> With my proposed changes from my previous email this should not be a large problem at all. When opening /etc/unbound/dhcp-leases.conf with mode “r+” you can catch the Exception and skip the comparison and open the file again with “w”.
>>>>> On 25 Mar 2024, at 10:10, Nick Howitt <nick@howitts.co.uk> wrote:
>>>>> 
>>>>> If there is a risk that the /etc/unbound/dhcp-leases.conf does not exist, for example on a clean install (I don't know), the filecmp.cmp line needs to be wrapped in a try...except to stop it blowing up:
>>>>> 
>>>>> try:
>>>>> RequireUnboundReload = not filecmp.cmp(f.name, self.path, shallow=False)
>>>>> except:
>>>>> RequireUnboundReload = True
>>>> I generally don’t like blanco exception catches. You will never know about the daemons that you are hiding.
>>>>> This will force the file to be to be generated, even if it is only copying in a blank temp file, and unbound will restart. It will only ever happen once.
>>>>> 
>>>>> 
>>>>> On 24/03/2024 15:49, Nick Howitt wrote:
>>>>>> Hi all,
>>>>>> **** This is a repost of an email incorrectly submitted. ****
>>>>>> 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 <nick@howitts.co.uk>
>>>>>> 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"]
>>> Resubmitting the patch.
>>> 
>>> Following my previous reply, I would like to stick with filecmp as I am using it with "shallow=False" to avoid doing the comparison just with stat() calls. Note that I also flush the filecmp cache before using the command so it is running with the latest file data. Talking about file descriptors, atomic file replacement etc., scares me.
>> File descriptors are great. They avoid race-conditions and all sorts of things.
> But I have never used them so I don't intend to go that way. Too much for me to get my brain round and a limited ability to test.

Well, I pretty much gave you the entire solution… There was nothing left to do apart from copying and pasting the snippets.

>> I don’t think that module is the best choice really if you have to hack around and flush the cache. Why would it even cache things in the first place? I don’t see where the cache helps.
> From the filecmp.clear_cache documentation:
> 
>   Clear the filecmp cache. This may be useful if a file is compared so
>   quickly after it is modified that it is within the mtime resolution
>   of the underlying filesystem.
> 
> Now this makes me believe that the clear_cache is only relevant when using shallow=True, which I'm not and I don't know how to check the underlying code. It is used to make sure the stat() info is updated, but we are not using the stat() info. I guess we could as easily add a one second sleep instead as that is way longer than the mtime resolution.

The function is here and it is rather simple: https://github.com/python/cpython/blob/3.12/Lib/filecmp.py#L30

It will always stat() both files and compares three items in case shallow is False: The type of the file, the size and mtime. If the file is not a regular file, the function will exist rather early. So in our case, that is all work the function is doing for thing. It boils down to something very similar to what I suggested to use to compare the files.

On top of that, there is a cache which will always be used but will never return a match for us because the temporary file at least will always have another mtime.

You could clear the cache after every call, but I would rather not have our code messy and care about implementation details of some Python module.

> The module is chosen because all the references I read about comparing files preferred this one-liner for comparing files, although other methods were given and there were lots of discussions about large files (GB+) because if you chomp them it may be much quicker to say they are not the same when you find the first block that does not match. Having said this, I don't know how filecmp works, but we only have a small file. At the same time, I am not a regular programmer so simple one-liners are good for me.

You would always want to read both files block for block and compare the blocks. There is no way to implement this significantly faster.

>>> I have modified the earlier try/except I proposed to catch only FileNotFoundError, and I have changed CamelCase to snake_case.
>>> 
>>> ---
>>> config/unbound/unbound-dhcp-leases-bridge | 31 ++++++++++++++++-------
>>> 1 file changed, 22 insertions(+), 9 deletions(-)
>>> 
>>> diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge
>>> index e9f022aff..a9ec8e1e2 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,38 @@ 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()
>>> + try:
>>> + require_unbound_reload = not filecmp.cmp(f.name, self.path, shallow=False)
>>> + except FileNotFoundError:
>>> + require_unbound_reload = True
>> If you want to go this route, you will have to flush the temporary file first. Otherwise you might have some data left in the buffer that has not been written (because the file is still open). You will then compare incomplete output. This would not become a problem if you used the file descriptors.
> The temp file is closed at the point of doing the filecmp, and the filecmp cache has been flushed. How do I flush the temporary file further. Reading up, is it just having the last statement in the with block as f.flush() with the same indent as "for l in sorted(leases):"

If you have left the with block you will already have flushed the buffers and closed the file. It looks like I misread the indentation in the patch then.

>> 
>> Catching FileNotFound would work for me.
> Good
>> 
>>> +
>>> + # 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 require_unbound_reload
>>> 
>>> def _control(self, *args):
>>> command = ["unbound-control"]
>>> -- 
>>> 2.43.0
>>> 
>>> 
>>> Nick
  

Next version.

On 28/03/2024 10:41, Michael Tremer wrote:
> 
> Hello,
> 
>> On 27 Mar 2024, at 15:28, Nick Howitt <nick@howitts.co.uk> wrote:
>>
>>
>>
>> On 26/03/2024 11:10, Michael Tremer wrote:
>>>
>>>
>>>> On 25 Mar 2024, at 13:11, Nick Howitt <nick@howitts.co.uk> wrote:
>>>>
>>>>
>>>>
>>>> On 25/03/2024 11:31, Michael Tremer wrote:
>>>>> Hello,
>>>>> I don’t think that the risk is very high, but I would account for this problem.
>>>>> With my proposed changes from my previous email this should not be a large problem at all. When opening /etc/unbound/dhcp-leases.conf with mode “r+” you can catch the Exception and skip the comparison and open the file again with “w”.
>>>>>> On 25 Mar 2024, at 10:10, Nick Howitt <nick@howitts.co.uk> wrote:
>>>>>>
>>>>>> If there is a risk that the /etc/unbound/dhcp-leases.conf does not exist, for example on a clean install (I don't know), the filecmp.cmp line needs to be wrapped in a try...except to stop it blowing up:
>>>>>>
>>>>>> try:
>>>>>> RequireUnboundReload = not filecmp.cmp(f.name, self.path, shallow=False)
>>>>>> except:
>>>>>> RequireUnboundReload = True
>>>>> I generally don’t like blanco exception catches. You will never know about the daemons that you are hiding.
>>>>>> This will force the file to be to be generated, even if it is only copying in a blank temp file, and unbound will restart. It will only ever happen once.
>>>>>>
>>>>>>
>>>>>> On 24/03/2024 15:49, Nick Howitt wrote:
>>>>>>> Hi all,
>>>>>>> **** This is a repost of an email incorrectly submitted. ****
>>>>>>> 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 <nick@howitts.co.uk>
>>>>>>> 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"]
>>>> Resubmitting the patch.
>>>>
>>>> Following my previous reply, I would like to stick with filecmp as I am using it with "shallow=False" to avoid doing the comparison just with stat() calls. Note that I also flush the filecmp cache before using the command so it is running with the latest file data. Talking about file descriptors, atomic file replacement etc., scares me.
>>> File descriptors are great. They avoid race-conditions and all sorts of things.
>> But I have never used them so I don't intend to go that way. Too much for me to get my brain round and a limited ability to test.
> 
> Well, I pretty much gave you the entire solution… There was nothing left to do apart from copying and pasting the snippets.
> 
Ok I've tried to use your version but struggled to get it going. If I 
used it as was and called compare(file1_name, file2_name) it failed as 
"compare" was not defined.
I could call it using self.compare but then it failed with "TypeError: 
UnboundConfigWriter.compare() takes 2 positional arguments but 3 were 
given". I changed the function to compare(self, f1, f2).
Then it failed with "AttributeError: 'str' object has no attribute 
'seek'", so I changed it again to accept file names then open the files 
before doing the seeks.
I really hope it is OK as I am out of my depth when defining functions 
with "self" as an argument. Please can you check it? If it is not OK, 
can I revert to filecmp?

I have made one futher change, the sorted(leases) line was not working 
and not sorting the object ever, and I saw lines changing places in the 
resulting leases file. I have changed it to "sorted(leases, key=lambda 
x: x.ipaddr)". It definitely sorts this time.

>>> I don’t think that module is the best choice really if you have to hack around and flush the cache. Why would it even cache things in the first place? I don’t see where the cache helps.
>>  From the filecmp.clear_cache documentation:
>>
>>    Clear the filecmp cache. This may be useful if a file is compared so
>>    quickly after it is modified that it is within the mtime resolution
>>    of the underlying filesystem.
>>
>> Now this makes me believe that the clear_cache is only relevant when using shallow=True, which I'm not and I don't know how to check the underlying code. It is used to make sure the stat() info is updated, but we are not using the stat() info. I guess we could as easily add a one second sleep instead as that is way longer than the mtime resolution.
> 
> The function is here and it is rather simple: https://github.com/python/cpython/blob/3.12/Lib/filecmp.py#L30
> 
> It will always stat() both files and compares three items in case shallow is False: The type of the file, the size and mtime. If the file is not a regular file, the function will exist rather early. So in our case, that is all work the function is doing for thing. It boils down to something very similar to what I suggested to use to compare the files.
> 
> On top of that, there is a cache which will always be used but will never return a match for us because the temporary file at least will always have another mtime.
> 
> You could clear the cache after every call, but I would rather not have our code messy and care about implementation details of some Python module.
> 
>> The module is chosen because all the references I read about comparing files preferred this one-liner for comparing files, although other methods were given and there were lots of discussions about large files (GB+) because if you chomp them it may be much quicker to say they are not the same when you find the first block that does not match. Having said this, I don't know how filecmp works, but we only have a small file. At the same time, I am not a regular programmer so simple one-liners are good for me.
> 
> You would always want to read both files block for block and compare the blocks. There is no way to implement this significantly faster.
> 
>>>> I have modified the earlier try/except I proposed to catch only FileNotFoundError, and I have changed CamelCase to snake_case.
>>>>
>>>> ---
>>>> config/unbound/unbound-dhcp-leases-bridge | 31 ++++++++++++++++-------
>>>> 1 file changed, 22 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge
>>>> index e9f022aff..a9ec8e1e2 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,38 @@ 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()
>>>> + try:
>>>> + require_unbound_reload = not filecmp.cmp(f.name, self.path, shallow=False)
>>>> + except FileNotFoundError:
>>>> + require_unbound_reload = True
>>> If you want to go this route, you will have to flush the temporary file first. Otherwise you might have some data left in the buffer that has not been written (because the file is still open). You will then compare incomplete output. This would not become a problem if you used the file descriptors.
>> The temp file is closed at the point of doing the filecmp, and the filecmp cache has been flushed. How do I flush the temporary file further. Reading up, is it just having the last statement in the with block as f.flush() with the same indent as "for l in sorted(leases):"
> 
> If you have left the with block you will already have flushed the buffers and closed the file. It looks like I misread the indentation in the patch then.
> 
>>>
>>> Catching FileNotFound would work for me.
>> Good
>>>
>>>> +
>>>> + # 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 require_unbound_reload
>>>>
>>>> def _control(self, *args):
>>>> command = ["unbound-control"]
>>>> -- 
>>>> 2.43.0
>>>>
>>>>
>>>> Nick
> 
> 
---
  config/unbound/unbound-dhcp-leases-bridge | 50 +++++++++++++++++++----
  1 file changed, 41 insertions(+), 9 deletions(-)

diff --git a/config/unbound/unbound-dhcp-leases-bridge 
b/config/unbound/unbound-dhcp-leases-bridge
index e9f022aff..80d9523ea 100644
--- a/config/unbound/unbound-dhcp-leases-bridge
+++ b/config/unbound/unbound-dhcp-leases-bridge
@@ -516,26 +516,37 @@ 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, key=lambda x: x.ipaddr):
  				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)
+		try:
+			require_unbound_reload = not self.compare(f.name, self.path)
+		except FileNotFoundError:
+			require_unbound_reload = True
+
+		# 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 require_unbound_reload

  	def _control(self, *args):
  		command = ["unbound-control"]
@@ -551,6 +562,27 @@ class UnboundConfigWriter(object):

  			raise e

+	def compare(self, if1, if2):
+		buffersize = 16 * 1024
+
+		f1 = open(if1, "r")
+		f2 = open(if2, "r")
+
+		# Reset file handles
+		f1.seek(0)
+		f2.seek(0)
+
+		while True:
+			b1 = f1.read(buffersize)
+			b2 = f2.read(buffersize)
+
+			if not b1 == b2:
+				return False
+
+			elif not b1 or not b2:
+				break
+
+		return True

  if __name__ == "__main__":
  	parser = argparse.ArgumentParser(description="Bridge for DHCP Leases 
and Unbound DNS")
  

Bump

On 28/03/2024 17:25, Nick Howitt wrote:
> Next version.
>
> On 28/03/2024 10:41, Michael Tremer wrote:
>>
>> Hello,
>>
>>> On 27 Mar 2024, at 15:28, Nick Howitt <nick@howitts.co.uk> wrote:
>>>
>>>
>>>
>>> On 26/03/2024 11:10, Michael Tremer wrote:
>>>>
>>>>
>>>>> On 25 Mar 2024, at 13:11, Nick Howitt <nick@howitts.co.uk> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 25/03/2024 11:31, Michael Tremer wrote:
>>>>>> Hello,
>>>>>> I don’t think that the risk is very high, but I would account for 
>>>>>> this problem.
>>>>>> With my proposed changes from my previous email this should not 
>>>>>> be a large problem at all. When opening 
>>>>>> /etc/unbound/dhcp-leases.conf with mode “r+” you can catch the 
>>>>>> Exception and skip the comparison and open the file again with “w”.
>>>>>>> On 25 Mar 2024, at 10:10, Nick Howitt <nick@howitts.co.uk> wrote:
>>>>>>>
>>>>>>> If there is a risk that the /etc/unbound/dhcp-leases.conf does 
>>>>>>> not exist, for example on a clean install (I don't know), the 
>>>>>>> filecmp.cmp line needs to be wrapped in a try...except to stop 
>>>>>>> it blowing up:
>>>>>>>
>>>>>>> try:
>>>>>>> RequireUnboundReload = not filecmp.cmp(f.name, self.path, 
>>>>>>> shallow=False)
>>>>>>> except:
>>>>>>> RequireUnboundReload = True
>>>>>> I generally don’t like blanco exception catches. You will never 
>>>>>> know about the daemons that you are hiding.
>>>>>>> This will force the file to be to be generated, even if it is 
>>>>>>> only copying in a blank temp file, and unbound will restart. It 
>>>>>>> will only ever happen once.
>>>>>>>
>>>>>>>
>>>>>>> On 24/03/2024 15:49, Nick Howitt wrote:
>>>>>>>> Hi all,
>>>>>>>> **** This is a repost of an email incorrectly submitted. ****
>>>>>>>> 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 <nick@howitts.co.uk>
>>>>>>>> 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"]
>>>>> Resubmitting the patch.
>>>>>
>>>>> Following my previous reply, I would like to stick with filecmp as 
>>>>> I am using it with "shallow=False" to avoid doing the comparison 
>>>>> just with stat() calls. Note that I also flush the filecmp cache 
>>>>> before using the command so it is running with the latest file 
>>>>> data. Talking about file descriptors, atomic file replacement 
>>>>> etc., scares me.
>>>> File descriptors are great. They avoid race-conditions and all 
>>>> sorts of things.
>>> But I have never used them so I don't intend to go that way. Too 
>>> much for me to get my brain round and a limited ability to test.
>>
>> Well, I pretty much gave you the entire solution… There was nothing 
>> left to do apart from copying and pasting the snippets.
>>
> Ok I've tried to use your version but struggled to get it going. If I 
> used it as was and called compare(file1_name, file2_name) it failed as 
> "compare" was not defined.
> I could call it using self.compare but then it failed with "TypeError: 
> UnboundConfigWriter.compare() takes 2 positional arguments but 3 were 
> given". I changed the function to compare(self, f1, f2).
> Then it failed with "AttributeError: 'str' object has no attribute 
> 'seek'", so I changed it again to accept file names then open the 
> files before doing the seeks.
> I really hope it is OK as I am out of my depth when defining functions 
> with "self" as an argument. Please can you check it? If it is not OK, 
> can I revert to filecmp?
>
> I have made one futher change, the sorted(leases) line was not working 
> and not sorting the object ever, and I saw lines changing places in 
> the resulting leases file. I have changed it to "sorted(leases, 
> key=lambda x: x.ipaddr)". It definitely sorts this time.
>
>>>> I don’t think that module is the best choice really if you have to 
>>>> hack around and flush the cache. Why would it even cache things in 
>>>> the first place? I don’t see where the cache helps.
>>>  From the filecmp.clear_cache documentation:
>>>
>>>    Clear the filecmp cache. This may be useful if a file is compared so
>>>    quickly after it is modified that it is within the mtime resolution
>>>    of the underlying filesystem.
>>>
>>> Now this makes me believe that the clear_cache is only relevant when 
>>> using shallow=True, which I'm not and I don't know how to check the 
>>> underlying code. It is used to make sure the stat() info is updated, 
>>> but we are not using the stat() info. I guess we could as easily add 
>>> a one second sleep instead as that is way longer than the mtime 
>>> resolution.
>>
>> The function is here and it is rather simple: 
>> https://github.com/python/cpython/blob/3.12/Lib/filecmp.py#L30
>>
>> It will always stat() both files and compares three items in case 
>> shallow is False: The type of the file, the size and mtime. If the 
>> file is not a regular file, the function will exist rather early. So 
>> in our case, that is all work the function is doing for thing. It 
>> boils down to something very similar to what I suggested to use to 
>> compare the files.
>>
>> On top of that, there is a cache which will always be used but will 
>> never return a match for us because the temporary file at least will 
>> always have another mtime.
>>
>> You could clear the cache after every call, but I would rather not 
>> have our code messy and care about implementation details of some 
>> Python module.
>>
>>> The module is chosen because all the references I read about 
>>> comparing files preferred this one-liner for comparing files, 
>>> although other methods were given and there were lots of discussions 
>>> about large files (GB+) because if you chomp them it may be much 
>>> quicker to say they are not the same when you find the first block 
>>> that does not match. Having said this, I don't know how filecmp 
>>> works, but we only have a small file. At the same time, I am not a 
>>> regular programmer so simple one-liners are good for me.
>>
>> You would always want to read both files block for block and compare 
>> the blocks. There is no way to implement this significantly faster.
>>
>>>>> I have modified the earlier try/except I proposed to catch only 
>>>>> FileNotFoundError, and I have changed CamelCase to snake_case.
>>>>>
>>>>> ---
>>>>> config/unbound/unbound-dhcp-leases-bridge | 31 
>>>>> ++++++++++++++++-------
>>>>> 1 file changed, 22 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/config/unbound/unbound-dhcp-leases-bridge 
>>>>> b/config/unbound/unbound-dhcp-leases-bridge
>>>>> index e9f022aff..a9ec8e1e2 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,38 @@ 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()
>>>>> + try:
>>>>> + require_unbound_reload = not filecmp.cmp(f.name, self.path, 
>>>>> shallow=False)
>>>>> + except FileNotFoundError:
>>>>> + require_unbound_reload = True
>>>> If you want to go this route, you will have to flush the temporary 
>>>> file first. Otherwise you might have some data left in the buffer 
>>>> that has not been written (because the file is still open). You 
>>>> will then compare incomplete output. This would not become a 
>>>> problem if you used the file descriptors.
>>> The temp file is closed at the point of doing the filecmp, and the 
>>> filecmp cache has been flushed. How do I flush the temporary file 
>>> further. Reading up, is it just having the last statement in the 
>>> with block as f.flush() with the same indent as "for l in 
>>> sorted(leases):"
>>
>> If you have left the with block you will already have flushed the 
>> buffers and closed the file. It looks like I misread the indentation 
>> in the patch then.
>>
>>>>
>>>> Catching FileNotFound would work for me.
>>> Good
>>>>
>>>>> +
>>>>> + # 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 require_unbound_reload
>>>>>
>>>>> def _control(self, *args):
>>>>> command = ["unbound-control"]
>>>>> -- 
>>>>> 2.43.0
>>>>>
>>>>>
>>>>> Nick
>>
>>
> ---
>  config/unbound/unbound-dhcp-leases-bridge | 50 +++++++++++++++++++----
>  1 file changed, 41 insertions(+), 9 deletions(-)
>
> diff --git a/config/unbound/unbound-dhcp-leases-bridge 
> b/config/unbound/unbound-dhcp-leases-bridge
> index e9f022aff..80d9523ea 100644
> --- a/config/unbound/unbound-dhcp-leases-bridge
> +++ b/config/unbound/unbound-dhcp-leases-bridge
> @@ -516,26 +516,37 @@ 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, key=lambda x: x.ipaddr):
>                  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)
> +        try:
> +            require_unbound_reload = not self.compare(f.name, self.path)
> +        except FileNotFoundError:
> +            require_unbound_reload = True
> +
> +        # 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 require_unbound_reload
>
>      def _control(self, *args):
>          command = ["unbound-control"]
> @@ -551,6 +562,27 @@ class UnboundConfigWriter(object):
>
>              raise e
>
> +    def compare(self, if1, if2):
> +        buffersize = 16 * 1024
> +
> +        f1 = open(if1, "r")
> +        f2 = open(if2, "r")
> +
> +        # Reset file handles
> +        f1.seek(0)
> +        f2.seek(0)
> +
> +        while True:
> +            b1 = f1.read(buffersize)
> +            b2 = f2.read(buffersize)
> +
> +            if not b1 == b2:
> +                return False
> +
> +            elif not b1 or not b2:
> +                break
> +
> +        return True
>
>  if __name__ == "__main__":
>      parser = argparse.ArgumentParser(description="Bridge for DHCP 
> Leases and Unbound DNS")
  

Please can I bump this again to request a review of my patch submitted 
on 28th March? Thanks, Nick.

On 10/04/2024 12:04, Nick Howitt wrote:
> Bump
>
> On 28/03/2024 17:25, Nick Howitt wrote:
>> Next version.
>>
>> On 28/03/2024 10:41, Michael Tremer wrote:
>>>
>>> Hello,
>>>
>>>> On 27 Mar 2024, at 15:28, Nick Howitt <nick@howitts.co.uk> wrote:
>>>>
>>>>
>>>>
>>>> On 26/03/2024 11:10, Michael Tremer wrote:
>>>>>
>>>>>
>>>>>> On 25 Mar 2024, at 13:11, Nick Howitt <nick@howitts.co.uk> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 25/03/2024 11:31, Michael Tremer wrote:
>>>>>>> Hello,
>>>>>>> I don’t think that the risk is very high, but I would account 
>>>>>>> for this problem.
>>>>>>> With my proposed changes from my previous email this should not 
>>>>>>> be a large problem at all. When opening 
>>>>>>> /etc/unbound/dhcp-leases.conf with mode “r+” you can catch the 
>>>>>>> Exception and skip the comparison and open the file again with “w”.
>>>>>>>> On 25 Mar 2024, at 10:10, Nick Howitt <nick@howitts.co.uk> wrote:
>>>>>>>>
>>>>>>>> If there is a risk that the /etc/unbound/dhcp-leases.conf does 
>>>>>>>> not exist, for example on a clean install (I don't know), the 
>>>>>>>> filecmp.cmp line needs to be wrapped in a try...except to stop 
>>>>>>>> it blowing up:
>>>>>>>>
>>>>>>>> try:
>>>>>>>> RequireUnboundReload = not filecmp.cmp(f.name, self.path, 
>>>>>>>> shallow=False)
>>>>>>>> except:
>>>>>>>> RequireUnboundReload = True
>>>>>>> I generally don’t like blanco exception catches. You will never 
>>>>>>> know about the daemons that you are hiding.
>>>>>>>> This will force the file to be to be generated, even if it is 
>>>>>>>> only copying in a blank temp file, and unbound will restart. It 
>>>>>>>> will only ever happen once.
>>>>>>>>
>>>>>>>>
>>>>>>>> On 24/03/2024 15:49, Nick Howitt wrote:
>>>>>>>>> Hi all,
>>>>>>>>> **** This is a repost of an email incorrectly submitted. ****
>>>>>>>>> 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 <nick@howitts.co.uk>
>>>>>>>>> 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"]
>>>>>> Resubmitting the patch.
>>>>>>
>>>>>> Following my previous reply, I would like to stick with filecmp 
>>>>>> as I am using it with "shallow=False" to avoid doing the 
>>>>>> comparison just with stat() calls. Note that I also flush the 
>>>>>> filecmp cache before using the command so it is running with the 
>>>>>> latest file data. Talking about file descriptors, atomic file 
>>>>>> replacement etc., scares me.
>>>>> File descriptors are great. They avoid race-conditions and all 
>>>>> sorts of things.
>>>> But I have never used them so I don't intend to go that way. Too 
>>>> much for me to get my brain round and a limited ability to test.
>>>
>>> Well, I pretty much gave you the entire solution… There was nothing 
>>> left to do apart from copying and pasting the snippets.
>>>
>> Ok I've tried to use your version but struggled to get it going. If I 
>> used it as was and called compare(file1_name, file2_name) it failed 
>> as "compare" was not defined.
>> I could call it using self.compare but then it failed with 
>> "TypeError: UnboundConfigWriter.compare() takes 2 positional 
>> arguments but 3 were given". I changed the function to compare(self, 
>> f1, f2).
>> Then it failed with "AttributeError: 'str' object has no attribute 
>> 'seek'", so I changed it again to accept file names then open the 
>> files before doing the seeks.
>> I really hope it is OK as I am out of my depth when defining 
>> functions with "self" as an argument. Please can you check it? If it 
>> is not OK, can I revert to filecmp?
>>
>> I have made one futher change, the sorted(leases) line was not 
>> working and not sorting the object ever, and I saw lines changing 
>> places in the resulting leases file. I have changed it to 
>> "sorted(leases, key=lambda x: x.ipaddr)". It definitely sorts this time.
>>
>>>>> I don’t think that module is the best choice really if you have to 
>>>>> hack around and flush the cache. Why would it even cache things in 
>>>>> the first place? I don’t see where the cache helps.
>>>>  From the filecmp.clear_cache documentation:
>>>>
>>>>    Clear the filecmp cache. This may be useful if a file is 
>>>> compared so
>>>>    quickly after it is modified that it is within the mtime resolution
>>>>    of the underlying filesystem.
>>>>
>>>> Now this makes me believe that the clear_cache is only relevant 
>>>> when using shallow=True, which I'm not and I don't know how to 
>>>> check the underlying code. It is used to make sure the stat() info 
>>>> is updated, but we are not using the stat() info. I guess we could 
>>>> as easily add a one second sleep instead as that is way longer than 
>>>> the mtime resolution.
>>>
>>> The function is here and it is rather simple: 
>>> https://github.com/python/cpython/blob/3.12/Lib/filecmp.py#L30
>>>
>>> It will always stat() both files and compares three items in case 
>>> shallow is False: The type of the file, the size and mtime. If the 
>>> file is not a regular file, the function will exist rather early. So 
>>> in our case, that is all work the function is doing for thing. It 
>>> boils down to something very similar to what I suggested to use to 
>>> compare the files.
>>>
>>> On top of that, there is a cache which will always be used but will 
>>> never return a match for us because the temporary file at least will 
>>> always have another mtime.
>>>
>>> You could clear the cache after every call, but I would rather not 
>>> have our code messy and care about implementation details of some 
>>> Python module.
>>>
>>>> The module is chosen because all the references I read about 
>>>> comparing files preferred this one-liner for comparing files, 
>>>> although other methods were given and there were lots of 
>>>> discussions about large files (GB+) because if you chomp them it 
>>>> may be much quicker to say they are not the same when you find the 
>>>> first block that does not match. Having said this, I don't know how 
>>>> filecmp works, but we only have a small file. At the same time, I 
>>>> am not a regular programmer so simple one-liners are good for me.
>>>
>>> You would always want to read both files block for block and compare 
>>> the blocks. There is no way to implement this significantly faster.
>>>
>>>>>> I have modified the earlier try/except I proposed to catch only 
>>>>>> FileNotFoundError, and I have changed CamelCase to snake_case.
>>>>>>
>>>>>> ---
>>>>>> config/unbound/unbound-dhcp-leases-bridge | 31 
>>>>>> ++++++++++++++++-------
>>>>>> 1 file changed, 22 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> diff --git a/config/unbound/unbound-dhcp-leases-bridge 
>>>>>> b/config/unbound/unbound-dhcp-leases-bridge
>>>>>> index e9f022aff..a9ec8e1e2 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,38 @@ 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()
>>>>>> + try:
>>>>>> + require_unbound_reload = not filecmp.cmp(f.name, self.path, 
>>>>>> shallow=False)
>>>>>> + except FileNotFoundError:
>>>>>> + require_unbound_reload = True
>>>>> If you want to go this route, you will have to flush the temporary 
>>>>> file first. Otherwise you might have some data left in the buffer 
>>>>> that has not been written (because the file is still open). You 
>>>>> will then compare incomplete output. This would not become a 
>>>>> problem if you used the file descriptors.
>>>> The temp file is closed at the point of doing the filecmp, and the 
>>>> filecmp cache has been flushed. How do I flush the temporary file 
>>>> further. Reading up, is it just having the last statement in the 
>>>> with block as f.flush() with the same indent as "for l in 
>>>> sorted(leases):"
>>>
>>> If you have left the with block you will already have flushed the 
>>> buffers and closed the file. It looks like I misread the indentation 
>>> in the patch then.
>>>
>>>>>
>>>>> Catching FileNotFound would work for me.
>>>> Good
>>>>>
>>>>>> +
>>>>>> + # 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 require_unbound_reload
>>>>>>
>>>>>> def _control(self, *args):
>>>>>> command = ["unbound-control"]
>>>>>> -- 
>>>>>> 2.43.0
>>>>>>
>>>>>>
>>>>>> Nick
>>>
>>>
>> ---
>>  config/unbound/unbound-dhcp-leases-bridge | 50 +++++++++++++++++++----
>>  1 file changed, 41 insertions(+), 9 deletions(-)
>>
>> diff --git a/config/unbound/unbound-dhcp-leases-bridge 
>> b/config/unbound/unbound-dhcp-leases-bridge
>> index e9f022aff..80d9523ea 100644
>> --- a/config/unbound/unbound-dhcp-leases-bridge
>> +++ b/config/unbound/unbound-dhcp-leases-bridge
>> @@ -516,26 +516,37 @@ 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, key=lambda x: x.ipaddr):
>>                  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)
>> +        try:
>> +            require_unbound_reload = not self.compare(f.name, 
>> self.path)
>> +        except FileNotFoundError:
>> +            require_unbound_reload = True
>> +
>> +        # 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 require_unbound_reload
>>
>>      def _control(self, *args):
>>          command = ["unbound-control"]
>> @@ -551,6 +562,27 @@ class UnboundConfigWriter(object):
>>
>>              raise e
>>
>> +    def compare(self, if1, if2):
>> +        buffersize = 16 * 1024
>> +
>> +        f1 = open(if1, "r")
>> +        f2 = open(if2, "r")
>> +
>> +        # Reset file handles
>> +        f1.seek(0)
>> +        f2.seek(0)
>> +
>> +        while True:
>> +            b1 = f1.read(buffersize)
>> +            b2 = f2.read(buffersize)
>> +
>> +            if not b1 == b2:
>> +                return False
>> +
>> +            elif not b1 or not b2:
>> +                break
>> +
>> +        return True
>>
>>  if __name__ == "__main__":
>>      parser = argparse.ArgumentParser(description="Bridge for DHCP 
>> Leases and Unbound DNS")
>
  

Hello Nick,

> On 25 Apr 2024, at 13:43, Nick Howitt <nick@howitts.co.uk> wrote:
> 
> Please can I bump this again to request a review of my patch submitted on 28th March? Thanks, Nick.

It is a little bit difficult to test your proposed changes if testers have to copy and paste them out of an email.

I have just submitted a patch set to this list that breaks down all the changes that you proposed and I cleaned up the code so that we can use the filecmp function without any extra steps. We can simply jump out of the write function and return False to prevent Unbound being reloaded.

We will also avoid replacing the file all the time as that causes lots of unnecessary disk I/O. I am not saying it makes a significant difference, but flash has a limited write count that we are still wasting.

Please review and you if you are happy, please add the appropriate Git tag.

Best,
-Michael

> On 10/04/2024 12:04, Nick Howitt wrote:
>> Bump
>> 
>> On 28/03/2024 17:25, Nick Howitt wrote:
>>> Next version. 
>>> 
>>> On 28/03/2024 10:41, Michael Tremer wrote:
>>>> 
>>>> Hello, 
>>>> 
>>>>> On 27 Mar 2024, at 15:28, Nick Howitt <nick@howitts.co.uk> wrote: 
>>>>> 
>>>>> 
>>>>> 
>>>>> On 26/03/2024 11:10, Michael Tremer wrote:
>>>>>> 
>>>>>> 
>>>>>>> On 25 Mar 2024, at 13:11, Nick Howitt <nick@howitts.co.uk> wrote: 
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> On 25/03/2024 11:31, Michael Tremer wrote:
>>>>>>>> Hello, 
>>>>>>>> I don’t think that the risk is very high, but I would account for this problem. 
>>>>>>>> With my proposed changes from my previous email this should not be a large problem at all. When opening /etc/unbound/dhcp-leases.conf with mode “r+” you can catch the Exception and skip the comparison and open the file again with “w”.
>>>>>>>>> On 25 Mar 2024, at 10:10, Nick Howitt <nick@howitts.co.uk> wrote: 
>>>>>>>>> 
>>>>>>>>> If there is a risk that the /etc/unbound/dhcp-leases.conf does not exist, for example on a clean install (I don't know), the filecmp.cmp line needs to be wrapped in a try...except to stop it blowing up: 
>>>>>>>>> 
>>>>>>>>> try: 
>>>>>>>>> RequireUnboundReload = not filecmp.cmp(f.name, self.path, shallow=False) 
>>>>>>>>> except: 
>>>>>>>>> RequireUnboundReload = True
>>>>>>>> I generally don’t like blanco exception catches. You will never know about the daemons that you are hiding.
>>>>>>>>> This will force the file to be to be generated, even if it is only copying in a blank temp file, and unbound will restart. It will only ever happen once. 
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> On 24/03/2024 15:49, Nick Howitt wrote:
>>>>>>>>>> Hi all, 
>>>>>>>>>> **** This is a repost of an email incorrectly submitted. **** 
>>>>>>>>>> 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 <nick@howitts.co.uk> 
>>>>>>>>>> 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"]
>>>>>>> Resubmitting the patch. 
>>>>>>> 
>>>>>>> Following my previous reply, I would like to stick with filecmp as I am using it with "shallow=False" to avoid doing the comparison just with stat() calls. Note that I also flush the filecmp cache before using the command so it is running with the latest file data. Talking about file descriptors, atomic file replacement etc., scares me.
>>>>>> File descriptors are great. They avoid race-conditions and all sorts of things.
>>>>> But I have never used them so I don't intend to go that way. Too much for me to get my brain round and a limited ability to test.
>>>> 
>>>> Well, I pretty much gave you the entire solution… There was nothing left to do apart from copying and pasting the snippets. 
>>>> 
>>> Ok I've tried to use your version but struggled to get it going. If I used it as was and called compare(file1_name, file2_name) it failed as "compare" was not defined. 
>>> I could call it using self.compare but then it failed with "TypeError: UnboundConfigWriter.compare() takes 2 positional arguments but 3 were given". I changed the function to compare(self, f1, f2). 
>>> Then it failed with "AttributeError: 'str' object has no attribute 'seek'", so I changed it again to accept file names then open the files before doing the seeks. 
>>> I really hope it is OK as I am out of my depth when defining functions with "self" as an argument. Please can you check it? If it is not OK, can I revert to filecmp? 
>>> 
>>> I have made one futher change, the sorted(leases) line was not working and not sorting the object ever, and I saw lines changing places in the resulting leases file. I have changed it to "sorted(leases, key=lambda x: x.ipaddr)". It definitely sorts this time. 
>>> 
>>>>>> I don’t think that module is the best choice really if you have to hack around and flush the cache. Why would it even cache things in the first place? I don’t see where the cache helps.
>>>>>  From the filecmp.clear_cache documentation: 
>>>>> 
>>>>>    Clear the filecmp cache. This may be useful if a file is compared so 
>>>>>    quickly after it is modified that it is within the mtime resolution 
>>>>>    of the underlying filesystem. 
>>>>> 
>>>>> Now this makes me believe that the clear_cache is only relevant when using shallow=True, which I'm not and I don't know how to check the underlying code. It is used to make sure the stat() info is updated, but we are not using the stat() info. I guess we could as easily add a one second sleep instead as that is way longer than the mtime resolution.
>>>> 
>>>> The function is here and it is rather simple: https://github.com/python/cpython/blob/3.12/Lib/filecmp.py#L30 
>>>> 
>>>> It will always stat() both files and compares three items in case shallow is False: The type of the file, the size and mtime. If the file is not a regular file, the function will exist rather early. So in our case, that is all work the function is doing for thing. It boils down to something very similar to what I suggested to use to compare the files. 
>>>> 
>>>> On top of that, there is a cache which will always be used but will never return a match for us because the temporary file at least will always have another mtime. 
>>>> 
>>>> You could clear the cache after every call, but I would rather not have our code messy and care about implementation details of some Python module. 
>>>> 
>>>>> The module is chosen because all the references I read about comparing files preferred this one-liner for comparing files, although other methods were given and there were lots of discussions about large files (GB+) because if you chomp them it may be much quicker to say they are not the same when you find the first block that does not match. Having said this, I don't know how filecmp works, but we only have a small file. At the same time, I am not a regular programmer so simple one-liners are good for me.
>>>> 
>>>> You would always want to read both files block for block and compare the blocks. There is no way to implement this significantly faster. 
>>>> 
>>>>>>> I have modified the earlier try/except I proposed to catch only FileNotFoundError, and I have changed CamelCase to snake_case. 
>>>>>>> 
>>>>>>> --- 
>>>>>>> config/unbound/unbound-dhcp-leases-bridge | 31 ++++++++++++++++------- 
>>>>>>> 1 file changed, 22 insertions(+), 9 deletions(-) 
>>>>>>> 
>>>>>>> diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge 
>>>>>>> index e9f022aff..a9ec8e1e2 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,38 @@ 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() 
>>>>>>> + try: 
>>>>>>> + require_unbound_reload = not filecmp.cmp(f.name, self.path, shallow=False) 
>>>>>>> + except FileNotFoundError: 
>>>>>>> + require_unbound_reload = True
>>>>>> If you want to go this route, you will have to flush the temporary file first. Otherwise you might have some data left in the buffer that has not been written (because the file is still open). You will then compare incomplete output. This would not become a problem if you used the file descriptors.
>>>>> The temp file is closed at the point of doing the filecmp, and the filecmp cache has been flushed. How do I flush the temporary file further. Reading up, is it just having the last statement in the with block as f.flush() with the same indent as "for l in sorted(leases):"
>>>> 
>>>> If you have left the with block you will already have flushed the buffers and closed the file. It looks like I misread the indentation in the patch then. 
>>>> 
>>>>>> 
>>>>>> Catching FileNotFound would work for me.
>>>>> Good
>>>>>> 
>>>>>>> + 
>>>>>>> + # 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 require_unbound_reload 
>>>>>>> 
>>>>>>> def _control(self, *args): 
>>>>>>> command = ["unbound-control"] 
>>>>>>> -- 
>>>>>>> 2.43.0 
>>>>>>> 
>>>>>>> 
>>>>>>> Nick
>>>> 
>>>> 
>>> --- 
>>>  config/unbound/unbound-dhcp-leases-bridge | 50 +++++++++++++++++++---- 
>>>  1 file changed, 41 insertions(+), 9 deletions(-) 
>>> 
>>> diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge 
>>> index e9f022aff..80d9523ea 100644 
>>> --- a/config/unbound/unbound-dhcp-leases-bridge 
>>> +++ b/config/unbound/unbound-dhcp-leases-bridge 
>>> @@ -516,26 +516,37 @@ 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, key=lambda x: x.ipaddr): 
>>>                  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) 
>>> +        try: 
>>> +            require_unbound_reload = not self.compare(f.name, self.path) 
>>> +        except FileNotFoundError: 
>>> +            require_unbound_reload = True 
>>> + 
>>> +        # 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 require_unbound_reload 
>>> 
>>>      def _control(self, *args): 
>>>          command = ["unbound-control"] 
>>> @@ -551,6 +562,27 @@ class UnboundConfigWriter(object): 
>>> 
>>>              raise e 
>>> 
>>> +    def compare(self, if1, if2): 
>>> +        buffersize = 16 * 1024 
>>> + 
>>> +        f1 = open(if1, "r") 
>>> +        f2 = open(if2, "r") 
>>> + 
>>> +        # Reset file handles 
>>> +        f1.seek(0) 
>>> +        f2.seek(0) 
>>> + 
>>> +        while True: 
>>> +            b1 = f1.read(buffersize) 
>>> +            b2 = f2.read(buffersize) 
>>> + 
>>> +            if not b1 == b2: 
>>> +                return False 
>>> + 
>>> +            elif not b1 or not b2: 
>>> +                break 
>>> + 
>>> +        return True 
>>> 
>>>  if __name__ == "__main__": 
>>>      parser = argparse.ArgumentParser(description="Bridge for DHCP Leases and Unbound DNS")
>> 
>
  

On 26/04/2024 16:12, Michael Tremer wrote:
> Hello Nick,
>
>> On 25 Apr 2024, at 13:43, Nick Howitt<nick@howitts.co.uk>  wrote:
>>
>> Please can I bump this again to request a review of my patch submitted on 28th March? Thanks, Nick.
> It is a little bit difficult to test your proposed changes if testers have to copy and paste them out of an email.
Confused. I was told I had to submit the diff in an email with [PATCH] 
in the title and not to attach a file. Please can you tell me how I 
should be operating?
>
> I have just submitted a patch set to this list that breaks down all the changes that you proposed and I cleaned up the code so that we can use the filecmp function without any extra steps. We can simply jump out of the write function and return False to prevent Unbound being reloaded.
I thought you didn't want me to use filecmp and pushed me to use your 
file compare routine.
>
> We will also avoid replacing the file all the time as that causes lots of unnecessary disk I/O. I am not saying it makes a significant difference, but flash has a limited write count that we are still wasting.
OK, but my changes were no worse than before, either doing a delete or 
rename whereas before it always did a rename. The "unnecessary I/O" was 
already there.
>
> Please review and you if you are happy, please add the appropriate Git tag.
Sorry but I don't know what you mean by "adding the appropriate Git 
tag". I am only using Git locally and have no repo access.

How can I pull your changes? Otherwise I'll need to patch my set up 
manually.

I also have another problem. I have temporarily had to remove my IPF 
gateway for a business reason and don't know when I'll be able to add it 
back in. I'll have to see if I can insert it in my LAN somehow. At the 
same time it is also running Jon's mods which I'll have to undo.

With respect to Jon's mods, I've asked a couple of times on bugzilla, 
but is there any point in his routine maintaining 
/etc/unbound/dhcp-leases.conf. As far as I see the file is only read 
once when unbound starts, so it only has to be valid when unbound starts 
and not at any other point in time. To this effect he can remove the 
file maintenance and I attached a modified copy 
unbound-dhcp-leases-bridge to his repo which generates a new 
dhcp-leases.conf and exits, removing all the daemonising bit and the 
inode watcher. This file can be run as part of the unbound init routine 
just before the line that actually starts or reloads unbound. It also 
has the advantage of cutting down a load of disk I/O, which is important 
as you've just pointed out.

Also on his patches, I had a significantly faster way of determining the 
domain name (in bash) and he and I were looking for your feedback on if 
we'd be allowed to use it.

Regards,

Nick
>
> Best,
> -Michael
>
>> On 10/04/2024 12:04, Nick Howitt wrote:
>>> Bump
>>>
>>> On 28/03/2024 17:25, Nick Howitt wrote:
>>>> Next version.
>>>>
>>>> On 28/03/2024 10:41, Michael Tremer wrote:
>>>>> Hello,
>>>>>
>>>>>> On 27 Mar 2024, at 15:28, Nick Howitt<nick@howitts.co.uk>  wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 26/03/2024 11:10, Michael Tremer wrote:
>>>>>>>
>>>>>>>> On 25 Mar 2024, at 13:11, Nick Howitt<nick@howitts.co.uk>  wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 25/03/2024 11:31, Michael Tremer wrote:
>>>>>>>>> Hello,
>>>>>>>>> I don’t think that the risk is very high, but I would account for this problem.
>>>>>>>>> With my proposed changes from my previous email this should not be a large problem at all. When opening /etc/unbound/dhcp-leases.conf with mode “r+” you can catch the Exception and skip the comparison and open the file again with “w”.
>>>>>>>>>> On 25 Mar 2024, at 10:10, Nick Howitt<nick@howitts.co.uk>  wrote:
>>>>>>>>>>
>>>>>>>>>> If there is a risk that the /etc/unbound/dhcp-leases.conf does not exist, for example on a clean install (I don't know), the filecmp.cmp line needs to be wrapped in a try...except to stop it blowing up:
>>>>>>>>>>
>>>>>>>>>> try:
>>>>>>>>>> RequireUnboundReload = not filecmp.cmp(f.name, self.path, shallow=False)
>>>>>>>>>> except:
>>>>>>>>>> RequireUnboundReload = True
>>>>>>>>> I generally don’t like blanco exception catches. You will never know about the daemons that you are hiding.
>>>>>>>>>> This will force the file to be to be generated, even if it is only copying in a blank temp file, and unbound will restart. It will only ever happen once.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 24/03/2024 15:49, Nick Howitt wrote:
>>>>>>>>>>> Hi all,
>>>>>>>>>>> **** This is a repost of an email incorrectly submitted. ****
>>>>>>>>>>> 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<nick@howitts.co.uk>  
>>>>>>>>>>> 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"]
>>>>>>>> Resubmitting the patch.
>>>>>>>>
>>>>>>>> Following my previous reply, I would like to stick with filecmp as I am using it with "shallow=False" to avoid doing the comparison just with stat() calls. Note that I also flush the filecmp cache before using the command so it is running with the latest file data. Talking about file descriptors, atomic file replacement etc., scares me.
>>>>>>> File descriptors are great. They avoid race-conditions and all sorts of things.
>>>>>> But I have never used them so I don't intend to go that way. Too much for me to get my brain round and a limited ability to test.
>>>>> Well, I pretty much gave you the entire solution… There was nothing left to do apart from copying and pasting the snippets.
>>>>>
>>>> Ok I've tried to use your version but struggled to get it going. If I used it as was and called compare(file1_name, file2_name) it failed as "compare" was not defined.
>>>> I could call it using self.compare but then it failed with "TypeError: UnboundConfigWriter.compare() takes 2 positional arguments but 3 were given". I changed the function to compare(self, f1, f2).
>>>> Then it failed with "AttributeError: 'str' object has no attribute 'seek'", so I changed it again to accept file names then open the files before doing the seeks.
>>>> I really hope it is OK as I am out of my depth when defining functions with "self" as an argument. Please can you check it? If it is not OK, can I revert to filecmp?
>>>>
>>>> I have made one futher change, the sorted(leases) line was not working and not sorting the object ever, and I saw lines changing places in the resulting leases file. I have changed it to "sorted(leases, key=lambda x: x.ipaddr)". It definitely sorts this time.
>>>>
>>>>>>> I don’t think that module is the best choice really if you have to hack around and flush the cache. Why would it even cache things in the first place? I don’t see where the cache helps.
>>>>>>   From the filecmp.clear_cache documentation:
>>>>>>
>>>>>>     Clear the filecmp cache. This may be useful if a file is compared so
>>>>>>     quickly after it is modified that it is within the mtime resolution
>>>>>>     of the underlying filesystem.
>>>>>>
>>>>>> Now this makes me believe that the clear_cache is only relevant when using shallow=True, which I'm not and I don't know how to check the underlying code. It is used to make sure the stat() info is updated, but we are not using the stat() info. I guess we could as easily add a one second sleep instead as that is way longer than the mtime resolution.
>>>>> The function is here and it is rather simple:https://github.com/python/cpython/blob/3.12/Lib/filecmp.py#L30  
>>>>>
>>>>> It will always stat() both files and compares three items in case shallow is False: The type of the file, the size and mtime. If the file is not a regular file, the function will exist rather early. So in our case, that is all work the function is doing for thing. It boils down to something very similar to what I suggested to use to compare the files.
>>>>>
>>>>> On top of that, there is a cache which will always be used but will never return a match for us because the temporary file at least will always have another mtime.
>>>>>
>>>>> You could clear the cache after every call, but I would rather not have our code messy and care about implementation details of some Python module.
>>>>>
>>>>>> The module is chosen because all the references I read about comparing files preferred this one-liner for comparing files, although other methods were given and there were lots of discussions about large files (GB+) because if you chomp them it may be much quicker to say they are not the same when you find the first block that does not match. Having said this, I don't know how filecmp works, but we only have a small file. At the same time, I am not a regular programmer so simple one-liners are good for me.
>>>>> You would always want to read both files block for block and compare the blocks. There is no way to implement this significantly faster.
>>>>>
>>>>>>>> I have modified the earlier try/except I proposed to catch only FileNotFoundError, and I have changed CamelCase to snake_case.
>>>>>>>>
>>>>>>>> ---
>>>>>>>> config/unbound/unbound-dhcp-leases-bridge | 31 ++++++++++++++++-------
>>>>>>>> 1 file changed, 22 insertions(+), 9 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge
>>>>>>>> index e9f022aff..a9ec8e1e2 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,38 @@ 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()
>>>>>>>> + try:
>>>>>>>> + require_unbound_reload = not filecmp.cmp(f.name, self.path, shallow=False)
>>>>>>>> + except FileNotFoundError:
>>>>>>>> + require_unbound_reload = True
>>>>>>> If you want to go this route, you will have to flush the temporary file first. Otherwise you might have some data left in the buffer that has not been written (because the file is still open). You will then compare incomplete output. This would not become a problem if you used the file descriptors.
>>>>>> The temp file is closed at the point of doing the filecmp, and the filecmp cache has been flushed. How do I flush the temporary file further. Reading up, is it just having the last statement in the with block as f.flush() with the same indent as "for l in sorted(leases):"
>>>>> If you have left the with block you will already have flushed the buffers and closed the file. It looks like I misread the indentation in the patch then.
>>>>>
>>>>>>> Catching FileNotFound would work for me.
>>>>>> Good
>>>>>>>> +
>>>>>>>> + # 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 require_unbound_reload
>>>>>>>>
>>>>>>>> def _control(self, *args):
>>>>>>>> command = ["unbound-control"]
>>>>>>>> -- 
>>>>>>>> 2.43.0
>>>>>>>>
>>>>>>>>
>>>>>>>> Nick
>>>>>
>>>> ---
>>>>   config/unbound/unbound-dhcp-leases-bridge | 50 +++++++++++++++++++----
>>>>   1 file changed, 41 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge
>>>> index e9f022aff..80d9523ea 100644
>>>> --- a/config/unbound/unbound-dhcp-leases-bridge
>>>> +++ b/config/unbound/unbound-dhcp-leases-bridge
>>>> @@ -516,26 +516,37 @@ 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, key=lambda x: x.ipaddr):
>>>>                   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)
>>>> +        try:
>>>> +            require_unbound_reload = not self.compare(f.name, self.path)
>>>> +        except FileNotFoundError:
>>>> +            require_unbound_reload = True
>>>> +
>>>> +        # 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 require_unbound_reload
>>>>
>>>>       def _control(self, *args):
>>>>           command = ["unbound-control"]
>>>> @@ -551,6 +562,27 @@ class UnboundConfigWriter(object):
>>>>
>>>>               raise e
>>>>
>>>> +    def compare(self, if1, if2):
>>>> +        buffersize = 16 * 1024
>>>> +
>>>> +        f1 = open(if1, "r")
>>>> +        f2 = open(if2, "r")
>>>> +
>>>> +        # Reset file handles
>>>> +        f1.seek(0)
>>>> +        f2.seek(0)
>>>> +
>>>> +        while True:
>>>> +            b1 = f1.read(buffersize)
>>>> +            b2 = f2.read(buffersize)
>>>> +
>>>> +            if not b1 == b2:
>>>> +                return False
>>>> +
>>>> +            elif not b1 or not b2:
>>>> +                break
>>>> +
>>>> +        return True
>>>>
>>>>   if __name__ == "__main__":
>>>>       parser = argparse.ArgumentParser(description="Bridge for DHCP Leases and Unbound DNS")
  

Hello Nick,

> On 26 Apr 2024, at 17:48, Nick Howitt <nick@howitts.co.uk> wrote:
> 
> 
> 
> On 26/04/2024 16:12, Michael Tremer wrote:
>> Hello Nick,
>> 
>> 
>>> On 25 Apr 2024, at 13:43, Nick Howitt <nick@howitts.co.uk> wrote:
>>> 
>>> Please can I bump this again to request a review of my patch submitted on 28th March? Thanks, Nick.
>>> 
>> It is a little bit difficult to test your proposed changes if testers have to copy and paste them out of an email.
> Confused. I was told I had to submit the diff in an email with [PATCH] in the title and not to attach a file. Please can you tell me how I should be operating?

No worries, we are going to sort this out.

So the first email in the thread was a correct patch. It was just the patch in the format that Git uses and it was properly parsed by Patchwork. Later in the same thread, the patch was just copied and pasted which was impossible for Patchwork to pick up. There is nothing wrong with using bits of code in the conversation, but it would have been nice to have the final version submitted once again.

>> I have just submitted a patch set to this list that breaks down all the changes that you proposed and I cleaned up the code so that we can use the filecmp function without any extra steps. We can simply jump out of the write function and return False to prevent Unbound being reloaded.
> I thought you didn't want me to use filecmp and pushed me to use your file compare routine.

Yeah, I would have preferred that too, but I thought using the module might be quicker now.

>> We will also avoid replacing the file all the time as that causes lots of unnecessary disk I/O. I am not saying it makes a significant difference, but flash has a limited write count that we are still wasting.
> OK, but my changes were no worse than before, either doing a delete or rename whereas before it always did a rename. The "unnecessary I/O" was already there.
>> 
>> Please review and you if you are happy, please add the appropriate Git tag.
> Sorry but I don't know what you mean by "adding the appropriate Git tag". I am only using Git locally and have no repo access.

  https://www.ipfire.org/docs/devel/git/tags

> How can I pull your changes? Otherwise I'll need to patch my set up manually.

You can apply the patch to your local repository using “git am”:

  https://git-scm.com/docs/git-am

> I also have another problem. I have temporarily had to remove my IPF gateway for a business reason and don't know when I'll be able to add it back in. I'll have to see if I can insert it in my LAN somehow. At the same time it is also running Jon's mods which I'll have to undo.
> 
> With respect to Jon's mods, I've asked a couple of times on bugzilla, but is there any point in his routine maintaining /etc/unbound/dhcp-leases.conf. As far as I see the file is only read once when unbound starts, so it only has to be valid when unbound starts and not at any other point in time. To this effect he can remove the file maintenance and I attached a modified copy unbound-dhcp-leases-bridge to his repo which generates a new dhcp-leases.conf and exits, removing all the daemonising bit and the inode watcher. This file can be run as part of the unbound init routine just before the line that actually starts or reloads unbound. It also has the advantage of cutting down a load of disk I/O, which is important as you've just pointed out.

I don’t think that we can fully remove the daemon part, because I don’t think that there is a way to get a script (shell or otherwise) fast enough to perform the job. I also think that we benefit a lot from keeping some state in memory, whereas the script will have to parse the old file first, generate a new file and then compare the result. That will take a long time.

Do we needed the leases file generated at startup? Yes, I think so, too. DNS is quite essential for the network and for local services to be able to start. So it has to come up very early. If it comes up with proper configuration we avoid any other problems where hosts might resolve names too early and get a different result compared to a couple of seconds later when the zone is fully initialised.

So all in all, I think the route I would most likely look at is changing the daemon to open a socket. We write minimal scripts which are called by dhcpd that simply pipe their data into the socket which the daemon will then parse. With all available leases in memory already, we can check if a lease has changed or if it has been renewed without any further changes. If nothing has changed, we don’t need to do anything. In case of a change, we will have to write the file again and have Unbound reload, or we even call unbound-control and make the one change without a reload. Doing that in the daemon should give us more flexibility and we avoid any performance penalties as the daemon could run multiple commands concurrently which dhcpd can’t do.

> Also on his patches, I had a significantly faster way of determining the domain name (in bash) and he and I were looking for your feedback on if we'd be allowed to use it.

Anything is allowed. It just has to be fast. And for that, I believe, calling any other binary will be a problem.

-Michael

> 
> Regards,
> 
> Nick
>> 
>> Best,
>> -Michael
>> 
>> 
>>> On 10/04/2024 12:04, Nick Howitt wrote:
>>> 
>>>> Bump
>>>> 
>>>> On 28/03/2024 17:25, Nick Howitt wrote:
>>>> 
>>>>> Next version. 
>>>>> 
>>>>> On 28/03/2024 10:41, Michael Tremer wrote:
>>>>> 
>>>>>> Hello, 
>>>>>> 
>>>>>> 
>>>>>>> On 27 Mar 2024, at 15:28, Nick Howitt <nick@howitts.co.uk> wrote: 
>>>>>>> 
>>>>>>> 
>>>>>>> 
>>>>>>> On 26/03/2024 11:10, Michael Tremer wrote:
>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>>> On 25 Mar 2024, at 13:11, Nick Howitt <nick@howitts.co.uk> wrote: 
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> On 25/03/2024 11:31, Michael Tremer wrote:
>>>>>>>>> 
>>>>>>>>>> Hello, 
>>>>>>>>>> I don’t think that the risk is very high, but I would account for this problem. 
>>>>>>>>>> With my proposed changes from my previous email this should not be a large problem at all. When opening /etc/unbound/dhcp-leases.conf with mode “r+” you can catch the Exception and skip the comparison and open the file again with “w”.
>>>>>>>>>> 
>>>>>>>>>>> On 25 Mar 2024, at 10:10, Nick Howitt <nick@howitts.co.uk> wrote: 
>>>>>>>>>>> 
>>>>>>>>>>> If there is a risk that the /etc/unbound/dhcp-leases.conf does not exist, for example on a clean install (I don't know), the filecmp.cmp line needs to be wrapped in a try...except to stop it blowing up: 
>>>>>>>>>>> 
>>>>>>>>>>> try: 
>>>>>>>>>>> RequireUnboundReload = not filecmp.cmp(f.name, self.path, shallow=False) 
>>>>>>>>>>> except: 
>>>>>>>>>>> RequireUnboundReload = True
>>>>>>>>>>> 
>>>>>>>>>> I generally don’t like blanco exception catches. You will never know about the daemons that you are hiding.
>>>>>>>>>> 
>>>>>>>>>>> This will force the file to be to be generated, even if it is only copying in a blank temp file, and unbound will restart. It will only ever happen once. 
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> On 24/03/2024 15:49, Nick Howitt wrote:
>>>>>>>>>>> 
>>>>>>>>>>>> Hi all, 
>>>>>>>>>>>> **** This is a repost of an email incorrectly submitted. **** 
>>>>>>>>>>>> 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 <nick@howitts.co.uk> 
>>>>>>>>>>>> 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"]
>>>>>>>>>>>> 
>>>>>>>>> Resubmitting the patch. 
>>>>>>>>> 
>>>>>>>>> Following my previous reply, I would like to stick with filecmp as I am using it with "shallow=False" to avoid doing the comparison just with stat() calls. Note that I also flush the filecmp cache before using the command so it is running with the latest file data. Talking about file descriptors, atomic file replacement etc., scares me.
>>>>>>>>> 
>>>>>>>> File descriptors are great. They avoid race-conditions and all sorts of things.
>>>>>>>> 
>>>>>>> But I have never used them so I don't intend to go that way. Too much for me to get my brain round and a limited ability to test.
>>>>>>> 
>>>>>> Well, I pretty much gave you the entire solution… There was nothing left to do apart from copying and pasting the snippets. 
>>>>>> 
>>>>>> 
>>>>> Ok I've tried to use your version but struggled to get it going. If I used it as was and called compare(file1_name, file2_name) it failed as "compare" was not defined. 
>>>>> I could call it using self.compare but then it failed with "TypeError: UnboundConfigWriter.compare() takes 2 positional arguments but 3 were given". I changed the function to compare(self, f1, f2). 
>>>>> Then it failed with "AttributeError: 'str' object has no attribute 'seek'", so I changed it again to accept file names then open the files before doing the seeks. 
>>>>> I really hope it is OK as I am out of my depth when defining functions with "self" as an argument. Please can you check it? If it is not OK, can I revert to filecmp? 
>>>>> 
>>>>> I have made one futher change, the sorted(leases) line was not working and not sorting the object ever, and I saw lines changing places in the resulting leases file. I have changed it to "sorted(leases, key=lambda x: x.ipaddr)". It definitely sorts this time. 
>>>>> 
>>>>> 
>>>>>>>> I don’t think that module is the best choice really if you have to hack around and flush the cache. Why would it even cache things in the first place? I don’t see where the cache helps.
>>>>>>>> 
>>>>>>> From the filecmp.clear_cache documentation: 
>>>>>>> 
>>>>>>> Clear the filecmp cache. This may be useful if a file is compared so 
>>>>>>> quickly after it is modified that it is within the mtime resolution 
>>>>>>> of the underlying filesystem. 
>>>>>>> 
>>>>>>> Now this makes me believe that the clear_cache is only relevant when using shallow=True, which I'm not and I don't know how to check the underlying code. It is used to make sure the stat() info is updated, but we are not using the stat() info. I guess we could as easily add a one second sleep instead as that is way longer than the mtime resolution.
>>>>>>> 
>>>>>> The function is here and it is rather simple: https://github.com/python/cpython/blob/3.12/Lib/filecmp.py#L30 
>>>>>> 
>>>>>> It will always stat() both files and compares three items in case shallow is False: The type of the file, the size and mtime. If the file is not a regular file, the function will exist rather early. So in our case, that is all work the function is doing for thing. It boils down to something very similar to what I suggested to use to compare the files. 
>>>>>> 
>>>>>> On top of that, there is a cache which will always be used but will never return a match for us because the temporary file at least will always have another mtime. 
>>>>>> 
>>>>>> You could clear the cache after every call, but I would rather not have our code messy and care about implementation details of some Python module. 
>>>>>> 
>>>>>> 
>>>>>>> The module is chosen because all the references I read about comparing files preferred this one-liner for comparing files, although other methods were given and there were lots of discussions about large files (GB+) because if you chomp them it may be much quicker to say they are not the same when you find the first block that does not match. Having said this, I don't know how filecmp works, but we only have a small file. At the same time, I am not a regular programmer so simple one-liners are good for me.
>>>>>>> 
>>>>>> You would always want to read both files block for block and compare the blocks. There is no way to implement this significantly faster. 
>>>>>> 
>>>>>> 
>>>>>>>>> I have modified the earlier try/except I proposed to catch only FileNotFoundError, and I have changed CamelCase to snake_case. 
>>>>>>>>> 
>>>>>>>>> --- 
>>>>>>>>> config/unbound/unbound-dhcp-leases-bridge | 31 ++++++++++++++++------- 
>>>>>>>>> 1 file changed, 22 insertions(+), 9 deletions(-) 
>>>>>>>>> 
>>>>>>>>> diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge 
>>>>>>>>> index e9f022aff..a9ec8e1e2 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,38 @@ 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() 
>>>>>>>>> + try: 
>>>>>>>>> + require_unbound_reload = not filecmp.cmp(f.name, self.path, shallow=False) 
>>>>>>>>> + except FileNotFoundError: 
>>>>>>>>> + require_unbound_reload = True
>>>>>>>>> 
>>>>>>>> If you want to go this route, you will have to flush the temporary file first. Otherwise you might have some data left in the buffer that has not been written (because the file is still open). You will then compare incomplete output. This would not become a problem if you used the file descriptors.
>>>>>>>> 
>>>>>>> The temp file is closed at the point of doing the filecmp, and the filecmp cache has been flushed. How do I flush the temporary file further. Reading up, is it just having the last statement in the with block as f.flush() with the same indent as "for l in sorted(leases):"
>>>>>>> 
>>>>>> If you have left the with block you will already have flushed the buffers and closed the file. It looks like I misread the indentation in the patch then. 
>>>>>> 
>>>>>> 
>>>>>>>> Catching FileNotFound would work for me.
>>>>>>>> 
>>>>>>> Good
>>>>>>> 
>>>>>>>> 
>>>>>>>>> + 
>>>>>>>>> + # 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 require_unbound_reload 
>>>>>>>>> 
>>>>>>>>> def _control(self, *args): 
>>>>>>>>> command = ["unbound-control"] 
>>>>>>>>> -- 
>>>>>>>>> 2.43.0 
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> Nick
>>>>>>>>> 
>>>>>> 
>>>>>> 
>>>>> --- 
>>>>> config/unbound/unbound-dhcp-leases-bridge | 50 +++++++++++++++++++---- 
>>>>> 1 file changed, 41 insertions(+), 9 deletions(-) 
>>>>> 
>>>>> diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge 
>>>>> index e9f022aff..80d9523ea 100644 
>>>>> --- a/config/unbound/unbound-dhcp-leases-bridge 
>>>>> +++ b/config/unbound/unbound-dhcp-leases-bridge 
>>>>> @@ -516,26 +516,37 @@ 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, key=lambda x: x.ipaddr): 
>>>>> 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) 
>>>>> + try: 
>>>>> + require_unbound_reload = not self.compare(f.name, self.path) 
>>>>> + except FileNotFoundError: 
>>>>> + require_unbound_reload = True 
>>>>> + 
>>>>> + # 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 require_unbound_reload 
>>>>> 
>>>>> def _control(self, *args): 
>>>>> command = ["unbound-control"] 
>>>>> @@ -551,6 +562,27 @@ class UnboundConfigWriter(object): 
>>>>> 
>>>>> raise e 
>>>>> 
>>>>> + def compare(self, if1, if2): 
>>>>> + buffersize = 16 * 1024 
>>>>> + 
>>>>> + f1 = open(if1, "r") 
>>>>> + f2 = open(if2, "r") 
>>>>> + 
>>>>> + # Reset file handles 
>>>>> + f1.seek(0) 
>>>>> + f2.seek(0) 
>>>>> + 
>>>>> + while True: 
>>>>> + b1 = f1.read(buffersize) 
>>>>> + b2 = f2.read(buffersize) 
>>>>> + 
>>>>> + if not b1 == b2: 
>>>>> + return False 
>>>>> + 
>>>>> + elif not b1 or not b2: 
>>>>> + break 
>>>>> + 
>>>>> + return True 
>>>>> 
>>>>> if __name__ == "__main__": 
>>>>> parser = argparse.ArgumentParser(description="Bridge for DHCP Leases and Unbound DNS")
>>>>> 
>>>> 
>>> 
>> 
>