Thanks again for your advice! On Fri, Apr 23, 2010 at 10:42:14AM +0200, Jim Meyering wrote:
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.
oops, abosolutely right. fixed locally.
+ 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.
sorry, missed.
+ 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.
fixed.
+ + 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.
right but I want to keep as it is for a while.
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; +}