On Thu, Oct 29, 2026 at 11:42:25PM -0400, Laine Stump wrote:
On 10/26/26 8:18 AM, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <berrange@redhat.com>
The current addnhostsWrite() method implements logic that is already present in virFileRewrite.
...and then some - it also does an fsync() before closing the new file, I guess to make sure that the new file really truly is all on disk and ready to use by the time the rename happens.
The fsync() combined with rename() is important for crash safety. Helps ensure that after host OS crash, you should get either the old file, or the new file, and not some intermediate half written mess.
Am I correct in thinking you included patch 3 (and also upcoming patch 5) just to make it more obvious that this was just doing a straight-across replacement of functionality? Because otherwise it would totally be okay with me to squash 3&4 together, and also 5&6.
Yeah, I wanted to separate a clear functional behaviour change from what is effectively just a refactoring here.
BTW, I also noticed that these patches remove the only uses of fputs() that gets compiled into any daemons (all that's left is a couple of tests, and a few uses in tools/vsh.c). Is fputs() one of those problematic (not async-signal safe) functions that we should ban via a syntax-check rule?
All of stdio (ie FILE* APIs) is unsafe from signal handlers and fork/exec as it has internal mutexes that are held. This dnsmasq code usage was harmless.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Laine Stump <laine@redhat.com>
--- src/util/virdnsmasq.c | 82 ++++++++++++++++--------------------------- 1 file changed, 31 insertions(+), 51 deletions(-)
diff --git a/src/util/virdnsmasq.c b/src/util/virdnsmasq.c index 817e060a44..656665e174 100644 --- a/src/util/virdnsmasq.c +++ b/src/util/virdnsmasq.c @@ -154,77 +154,57 @@ addnhostsNew(const char *name, return NULL; } + static int -addnhostsWrite(const char *path, - dnsmasqAddnHost *hosts, - unsigned int nhosts) +addnhostsWrite(int fd, + const char *path, + const void *opaque) { - g_autofree char *tmp = NULL; - FILE *f; + const dnsmasqAddnHostsfile *addnhostsfile = opaque; size_t i, j; - int rc = 0; /* even if there are 0 hosts, create a 0 length file, to allow * for runtime addition. */ - tmp = g_strdup_printf("%s.new", path); - - 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); - unlink(tmp); - return rc; - } - - for (j = 0; j < hosts[i].nhostnames; j++) { - if (fputs(hosts[i].hostnames[j], f) == EOF || fputc('\t', f) == EOF) { - rc = -errno; - VIR_FORCE_FCLOSE(f); - unlink(tmp); - return rc; - } - } - - if (fputc('\n', f) == EOF) { - rc = -errno; - VIR_FORCE_FCLOSE(f); - unlink(tmp); - return rc; + for (i = 0; i < addnhostsfile->nhosts; i++) { + if (safewrite(fd, addnhostsfile->hosts[i].ip, + strlen(addnhostsfile->hosts[i].ip)) < 0) + goto error; + if (safewrite(fd, "\t", 1) < 0) + goto error; + + for (j = 0; j < addnhostsfile->hosts[i].nhostnames; j++) { + if (safewrite(fd, addnhostsfile->hosts[i].hostnames[j], + strlen(addnhostsfile->hosts[i].hostnames[j])) < 0) + goto error; + if (safewrite(fd, "\t", 1) < 0) + goto error; } - } - - if (VIR_FCLOSE(f) == EOF) - return -errno; - if (rename(tmp, path) < 0) { - rc = -errno; - unlink(tmp); - return rc; + if (safewrite(fd, "\n", 1) < 0) + goto error; } return 0; + + error: + virReportSystemError(errno, + _("cannot write config file '%1$s'"), + path); + return -1; } static int addnhostsSave(dnsmasqAddnHostsfile *addnhostsfile) { - int err = addnhostsWrite(addnhostsfile->path, addnhostsfile->hosts, - addnhostsfile->nhosts); - - if (err < 0) { - virReportSystemError(-err, _("cannot write config file '%1$s'"), - addnhostsfile->path); - return -1; - } - - return 0; + return virFileRewrite(addnhostsfile->path, + 0600, -1, -1, + addnhostsWrite, + addnhostsfile); } + static int genericFileDelete(char *path) {
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 :|