On 10/26/26 8:18 AM, Daniel P. Berrangé via Devel wrote:
From: Daniel P. Berrangé <[email protected]>
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.
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.
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?
Signed-off-by: Daniel P. Berrangé <[email protected]>
Reviewed-by: Laine Stump <[email protected]>
---
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)
{
