Re: [libvirt] [PATCH 1/3] Add dnsmasq module (v2)
Satoru SATOH wrote:
...
+static int
+hostsfileWrite(const char *path,
+ dnsmasqDhcpHost *hosts,
+ unsigned int nhosts)
+{
+ char *tmp;
+ FILE *f;
+ bool istmp = true;
+ unsigned int i;
+ int saved_errno;
+
+ if (nhosts == 0 && unlink(path) == 0)
+ return 0;
+
+ if (virAsprintf(&tmp, "%s.new", path) < 0)
+ return -1;
Any return after this point must be preceded by
code that frees "tmp". Otherwise it is a leak.
+ if (!(f = fopen(tmp, "w"))) {
+ istmp = false;
+ if (!(f = fopen(path, "w")))
+ return errno;
+ }
+
+ for (i = 0; i < nhosts; i++) {
+ if (fputs(hosts[i].host, f) == EOF || fputc('\n', f) == EOF) {
+ saved_errno = errno;
+ fclose(f);
+
+ if (istmp)
+ unlink(tmp);
+
+ return saved_errno;
+ }
+ }
+
+ fclose(f);
I mentioned this already.
You must detect write failure here.
+ if (istmp && rename(tmp, path) < 0) {
+ saved_errno = errno;
+ unlink(tmp);
+
+ return saved_errno;
+ }
+
+ if (istmp)
+ unlink(tmp);
Failure to remove this temporary file might deserve a warning.
No big deal either way.
+
+ return 0;
+}
...
+static int
+hostsfileDelete(dnsmasqHostsfile *hostsfile)
+{
+ if (!virFileExists(hostsfile->path))
+ return 0;
+
+ if (unlink(hostsfile->path) < 0) {
+ virReportSystemError(errno, _("cannot remove config file
'%s'"),
I think saying "failed to..." is slightly clearer than "cannot...".
> + hostsfile->path);
> + return -1;
> + }
+
+ return 0;
+}
> +
> +/**
> + * dnsmasqAddDhcpHost:
> + * @ctx: pointer to the dnsmasq context for each network
> + * @mac: pointer to the string contains mac address of the host
> + * @ip: pointer to the string contains ip address of the host
> + * @name: pointer to the string contains hostname of the host or NULL
> + *
> + * Add dhcp-host entry.
> + */
> +void
> +dnsmasqAddDhcpHost(dnsmasqContext *ctx,
> + const char *mac,
> + const char *ip,
> + const char *name)
> +{
> + if (ctx->hostsfile)
> + hostsfileAdd(ctx->hostsfile, mac, ip, name);
Would a caller ever want to know if this function fails?
If so, don't ignore hostsfileAdd failure.
That would be more consistent with how the following functions work, too.
BTW, shouldn't hostsfileAdd be spelled hostsFileAdd (with a capital "F")?
+}
+
+/**
+ * dnsmasqSave:
+ * @ctx: pointer to the dnsmasq context for each network
+ *
+ * Saves all the configurations associated with a context to disk.
+ */
+int
+dnsmasqSave(const dnsmasqContext *ctx)
+{
+ if (ctx->hostsfile)
+ return hostsfileSave(ctx->hostsfile);
+ return 0;
+}