This change no longer renames the file, but removes the old link and creates a new link for the temporary file. That helps us to jump out of the code at any point without worrying about cleaning up the temporary file.
Signed-off-by: Michael Tremer michael.tremer@ipfire.org --- config/unbound/unbound-dhcp-leases-bridge | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge index e9f022aff..5d5696af0 100644 --- a/config/unbound/unbound-dhcp-leases-bridge +++ b/config/unbound/unbound-dhcp-leases-bridge @@ -526,16 +526,22 @@ class UnboundConfigWriter(object): def write_dhcp_leases(self, leases): log.debug("Writing DHCP leases...")
- with tempfile.NamedTemporaryFile(mode="w", delete=False) as f: + with tempfile.NamedTemporaryFile(mode="w") as f: for l in leases: for rr in l.rrset: f.write("local-data: "%s"\n" % " ".join(rr))
+ # Flush the file + f.flush() + # Make file readable for everyone os.fchmod(f.fileno(), stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat.S_IROTH)
+ # Remove the old file + os.unlink(self.path) + # Move the file to its destination - os.rename(f.name, self.path) + os.link(f.name, self.path)
def _control(self, *args): command = ["unbound-control"]
This patches changes that leases will always be written in alphanumerical order so that we can later compare the newly generated file with the previous version. If it has not changed, we skip reload Unbound.
Suggested-by: Nick Howitt nick@howitts.co.uk Signed-off-by: Michael Tremer michael.tremer@ipfire.org --- config/unbound/unbound-dhcp-leases-bridge | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge index 5d5696af0..80c8267e8 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,24 +517,29 @@ 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")
def write_dhcp_leases(self, leases): log.debug("Writing DHCP leases...")
with tempfile.NamedTemporaryFile(mode="w") 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))
# Flush the file f.flush()
+ # Compare if the new leases file has changed from the previous version + if filecmp.cmp(f.name, self.path, shallow=False): + log.debug("The generated leases file has not changed") + + return False + # Make file readable for everyone os.fchmod(f.fileno(), stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat.S_IROTH)
@@ -543,6 +549,8 @@ class UnboundConfigWriter(object): # Move the file to its destination os.link(f.name, self.path)
+ return True + def _control(self, *args): command = ["unbound-control"] command.extend(args)
Can I just ask here as I don't know python file i/o at all.
The reason I moved filecmp outside the "with tempfile....." was because the file was being held open without an EOF so the filecmp always failed, making it useless. When you do the f.flush(), foes the EOF get added to the temp file, or does the same problem that I had exist here?
On 26/04/2024 16:09, Michael Tremer wrote:
This patches changes that leases will always be written in alphanumerical order so that we can later compare the newly generated file with the previous version. If it has not changed, we skip reload Unbound.
Suggested-by: Nick Howittnick@howitts.co.uk Signed-off-by: Michael Tremermichael.tremer@ipfire.org
config/unbound/unbound-dhcp-leases-bridge | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge index 5d5696af0..80c8267e8 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,24 +517,29 @@ 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 cacheself._control("reload_keep_cache")
# Reload the configuration without dropping the cacheself._control("reload_keep_cache")def write_dhcp_leases(self, leases): log.debug("Writing DHCP leases...")
with tempfile.NamedTemporaryFile(mode="w") 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)) # Flush the file f.flush()# Compare if the new leases file has changed from the previous versionif filecmp.cmp(f.name, self.path, shallow=False):log.debug("The generated leases file has not changed")return False# Make file readable for everyone os.fchmod(f.fileno(), stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat.S_IROTH)@@ -543,6 +549,8 @@ class UnboundConfigWriter(object): # Move the file to its destination os.link(f.name, self.path)
return True- def _control(self, *args): command = ["unbound-control"] command.extend(args)
Hello,
On 26 Apr 2024, at 17:54, Nick Howitt nick@howitts.co.uk wrote:
Can I just ask here as I don't know python file i/o at all.
There is pretty much no difference to the regular C API, except that Python has a little bit more aggressive buffering.
The reason I moved filecmp outside the "with tempfile....." was because the file was being held open without an EOF so the filecmp always failed, making it useless. When you do the f.flush(), foes the EOF get added to the temp file, or does the same problem that I had exist here?
Normally, if you call f.write(…) the data might not always be written to disk immediately. There is a small buffer to avoid tiny write operations. flush() manually writes everything to disk. That way, we can keep the file handle open (so that we can later delete the file) and it can be opened again to call compare.
Alternatively, there could have been a compare function that works on the already opened file descriptor.
On 26/04/2024 16:09, Michael Tremer wrote:
This patches changes that leases will always be written in alphanumerical order so that we can later compare the newly generated file with the previous version. If it has not changed, we skip reload Unbound.
Suggested-by: Nick Howitt nick@howitts.co.uk Signed-off-by: Michael Tremer michael.tremer@ipfire.org
config/unbound/unbound-dhcp-leases-bridge | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge index 5d5696af0..80c8267e8 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,24 +517,29 @@ 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")
def write_dhcp_leases(self, leases): log.debug("Writing DHCP leases...")
with tempfile.NamedTemporaryFile(mode="w") 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))
# Flush the file f.flush()
- # Compare if the new leases file has changed from the previous version
- if filecmp.cmp(f.name, self.path, shallow=False):
- log.debug("The generated leases file has not changed")
- return False
# Make file readable for everyone os.fchmod(f.fileno(), stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat.S_IROTH)
@@ -543,6 +549,8 @@ class UnboundConfigWriter(object): # Move the file to its destination os.link(f.name, self.path)
- return True
def _control(self, *args): command = ["unbound-control"] command.extend(args)
On 26/04/2024 16:09, Michael Tremer wrote:
This patches changes that leases will always be written in alphanumerical order so that we can later compare the newly generated file with the previous version. If it has not changed, we skip reload Unbound.
Suggested-by: Nick Howittnick@howitts.co.uk Signed-off-by: Michael Tremermichael.tremer@ipfire.org
config/unbound/unbound-dhcp-leases-bridge | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge index 5d5696af0..80c8267e8 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,24 +517,29 @@ 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 cacheself._control("reload_keep_cache")
# Reload the configuration without dropping the cacheself._control("reload_keep_cache")def write_dhcp_leases(self, leases): log.debug("Writing DHCP leases...")
with tempfile.NamedTemporaryFile(mode="w") 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)) # Flush the file f.flush()# Compare if the new leases file has changed from the previous versionif filecmp.cmp(f.name, self.path, shallow=False):log.debug("The generated leases file has not changed")return False# Make file readable for everyone os.fchmod(f.fileno(), stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat.S_IROTH)@@ -543,6 +549,8 @@ class UnboundConfigWriter(object): # Move the file to its destination os.link(f.name, self.path)
return True- def _control(self, *args): command = ["unbound-control"] command.extend(args)
|Signed-off-by: Nick Howitt nick@howitts.co.uk|
This patch catches any errors if the file did not previously exist and therefore skips the comparison.
Signed-off-by: Michael Tremer michael.tremer@ipfire.org --- config/unbound/unbound-dhcp-leases-bridge | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge index 80c8267e8..7f89f620a 100644 --- a/config/unbound/unbound-dhcp-leases-bridge +++ b/config/unbound/unbound-dhcp-leases-bridge @@ -535,17 +535,22 @@ class UnboundConfigWriter(object): f.flush()
# Compare if the new leases file has changed from the previous version - if filecmp.cmp(f.name, self.path, shallow=False): - log.debug("The generated leases file has not changed") + try: + if filecmp.cmp(f.name, self.path, shallow=False): + log.debug("The generated leases file has not changed")
- return False + return False + + # Remove the old file + os.unlink(self.path) + + # If the previous file did not exist, just keep falling through + except FileNotFoundError: + pass
# Make file readable for everyone os.fchmod(f.fileno(), stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat.S_IROTH)
- # Remove the old file - os.unlink(self.path) - # Move the file to its destination os.link(f.name, self.path)
On 26/04/2024 16:09, Michael Tremer wrote:
This patch catches any errors if the file did not previously exist and therefore skips the comparison.
Signed-off-by: Michael Tremermichael.tremer@ipfire.org
config/unbound/unbound-dhcp-leases-bridge | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge index 80c8267e8..7f89f620a 100644 --- a/config/unbound/unbound-dhcp-leases-bridge +++ b/config/unbound/unbound-dhcp-leases-bridge @@ -535,17 +535,22 @@ class UnboundConfigWriter(object): f.flush()
# Compare if the new leases file has changed from the previous version
if filecmp.cmp(f.name, self.path, shallow=False):log.debug("The generated leases file has not changed")
try:if filecmp.cmp(f.name, self.path, shallow=False):log.debug("The generated leases file has not changed")
return False
return False# Remove the old fileos.unlink(self.path)# If the previous file did not exist, just keep falling throughexcept FileNotFoundError:pass # Make file readable for everyone os.fchmod(f.fileno(), stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat.S_IROTH)
# Remove the old fileos.unlink(self.path)# Move the file to its destination os.link(f.name, self.path)
|Signed-off-by: Nick Howitt nick@howitts.co.uk|
On 26/04/2024 16:09, Michael Tremer wrote:
This change no longer renames the file, but removes the old link and creates a new link for the temporary file. That helps us to jump out of the code at any point without worrying about cleaning up the temporary file.
Signed-off-by: Michael Tremermichael.tremer@ipfire.org
config/unbound/unbound-dhcp-leases-bridge | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/config/unbound/unbound-dhcp-leases-bridge b/config/unbound/unbound-dhcp-leases-bridge index e9f022aff..5d5696af0 100644 --- a/config/unbound/unbound-dhcp-leases-bridge +++ b/config/unbound/unbound-dhcp-leases-bridge @@ -526,16 +526,22 @@ class UnboundConfigWriter(object): def write_dhcp_leases(self, leases): log.debug("Writing DHCP leases...")
with tempfile.NamedTemporaryFile(mode="w", delete=False) as f:
with tempfile.NamedTemporaryFile(mode="w") as f: for l in leases: for rr in l.rrset: f.write("local-data: \"%s\"\n" % " ".join(rr))# Flush the filef.flush()# Make file readable for everyone os.fchmod(f.fileno(), stat.S_IRUSR|stat.S_IWUSR|stat.S_IRGRP|stat.S_IROTH)# Remove the old fileos.unlink(self.path)# Move the file to its destination
os.rename(f.name, self.path)
os.link(f.name, self.path)def _control(self, *args): command = ["unbound-control"]
|Signed-off-by: Nick Howitt nick@howitts.co.uk|
-
Michael Tremer -
Nick Howitt