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;
> > +}
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list