[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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;
> > +}


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]