Re: [libvirt] [PATCH 1/3] Add dnsmasq module (v2)
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;
> +}