On Thu, Oct 29, 2026 at 11:09:01AM -0400, Laine Stump wrote:
On 10/26/26 8:18 AM, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
When writing the dnsmasq addnhosts file to a temporary file, there is a fallback which tries to open the original file. We expect creating the temporary file to always succeed, and if it fails something serious has gone wrong that we want to have reported. So this fallback is expected to be dead code.
I guess the point of writing to the temp file is either 1) so that the original is still there in the case that writing the new one fails, or 2) to make the change atomic? This code has been around so long that I only barely recognize the original author's name, much less have any idea why it is the way it is :-P (admittedly I didn't spend much time thinking about it). But anyway if it's being done, there must have been some reason for it, and I can't think of any good reason why we would fail to open and write to "x.tmp" but then succeed to write to "x".
Mostly I think it is the atomicity that's important, as well as greater robustness on host crash, but this hand written code fails at the latter since it lacks the fsync() that virFileRewrite does.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
--- src/util/virdnsmasq.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-)
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 7d6de943cf..817e060a44 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -161,7 +161,6 @@ addnhostsWrite(const char *path, { g_autofree char *tmp = NULL; FILE *f; - bool istmp = true; size_t i, j; int rc = 0; @@ -171,20 +170,14 @@ addnhostsWrite(const char *path, tmp = g_strdup_printf("%s.new", path); - if (!(f = fopen(tmp, "w"))) { - istmp = false; - if (!(f = fopen(path, "w"))) - return -errno; - } + if (!(f = fopen(tmp, "w"))) + return -errno; for (i = 0; i < nhosts; i++) { if (fputs(hosts[i].ip, f) == EOF || fputc('\t', f) == EOF) { rc = -errno; VIR_FORCE_FCLOSE(f); - - if (istmp) - unlink(tmp); - + unlink(tmp); return rc; } @@ -192,10 +185,7 @@ addnhostsWrite(const char *path, if (fputs(hosts[i].hostnames[j], f) == EOF || fputc('\t', f) == EOF) { rc = -errno; VIR_FORCE_FCLOSE(f); - - if (istmp) - unlink(tmp); - + unlink(tmp); return rc; } } @@ -203,10 +193,7 @@ addnhostsWrite(const char *path, if (fputc('\n', f) == EOF) { rc = -errno; VIR_FORCE_FCLOSE(f); - - if (istmp) - unlink(tmp); - + unlink(tmp); return rc; } } @@ -214,7 +201,7 @@ addnhostsWrite(const char *path, if (VIR_FCLOSE(f) == EOF) return -errno; - if (istmp && rename(tmp, path) < 0) { + if (rename(tmp, path) < 0) { rc = -errno; unlink(tmp); return rc;
With regards, Daniel -- |: https://berrange.com ~~ https://hachyderm.io/@berrange :| |: https://libvirt.org ~~ https://entangle-photo.org :| |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|