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

Re: [libvirt] [Resend][PATCH 1/3] Add dnsmasq module



Thanks a lot for your detailed feedback!

On Tue, Apr 20, 2010 at 07:42:45PM +0200, Jim Meyering wrote:
> Satoru SATOH wrote:
> > This patch adds the files implements dnsmasq (hostsfile) module.
> 
> Here is some feedback not on the overall structure or utility,
> but more on a few implementation details that stood out.
> 
> ...
> > diff --git a/src/util/dnsmasq.c b/src/util/dnsmasq.c
> ...
> > +static void
> > +dhcphostFree(dnsmasqDhcpHost *host)
> > +{
> > +    VIR_FREE(host->host);
> > +}
> > +
> > +static void
> > +hostsfileFree(dnsmasqHostsfile *hostsfile)
> > +{
> > +    int i;
> 
> Making that
>   unsigned int i;
> is slightly better (if possible[*]),
> since it's obvious "i" will never need to be signed.
> 
> [*] whether it is possible depends on the type of the nhosts
> member and the set of gcc warnings enabled.  Build with
> --enable-compile-warnings=error to check.
 
absolutely right.

fixed locally and will post it again.

> > +
> > +    if (hostsfile->hosts) {
> > +        for (i = 0; i < hostsfile->nhosts; i++)
> > +            dhcphostFree(&hostsfile->hosts[i]);
> > +
> > +        VIR_FREE(hostsfile->hosts);
> > +
> > +        hostsfile->nhosts = 0;
> > +    }
> > +
> > +    hostsfile->path[0] = '\0';
> > +
> > +    VIR_FREE(hostsfile);
> > +}
> ...
> 
> > +static dnsmasqHostsfile *
> > +hostsfileNew(const char *name,
> > +             const char *config_dir)
> > +{
> > +    int err;
> > +    dnsmasqHostsfile *hostsfile;
> > +
> > +    if (VIR_ALLOC(hostsfile) < 0)
> > +        return NULL;
> > +
> > +    hostsfile->hosts = NULL;
> > +    hostsfile->nhosts = 0;
> > +
> > +    if ((err = virFileMakePath(config_dir))) {
> > +        virReportSystemError(err, _("cannot create config directory '%s'"),
> > +            config_dir);
> > +        goto error;
> > +    }
> > +
> > +    if (virFileBuildPath(config_dir, name, ".hostsfile", hostsfile->path,
> > +            sizeof(hostsfile->path)) < 0) {
> > +        virReportSystemError(err,
> > +            _("config filename '%s/%s.%s' is too long"),
> > +            config_dir, name, ".hostsfile");
> > +        goto error;
> > +    }
> > +
> > +    return hostsfile;
> > +
> > + error:
> > +    hostsfileFree(hostsfile);
> > +    return NULL;
> > +}
> > +
> > +static int
> > +hostsfileWrite(const char *path, dnsmasqDhcpHost *hosts, int nhosts)
> > +{
> > +    char tmp[PATH_MAX];
> 
> It's good to avoid use of PATH_MAX.
> Declare it like this instead, ...
 
I understand.

>     char *tmp;
> 
> > +    FILE *f;
> > +    int istmp;
> > +    int i;
> > +
> > +    if (nhosts == 0 && unlink(path) == 0)
> > +        return 0;
> > +
> > +    if (snprintf(tmp, PATH_MAX, "%s.new", path) >= PATH_MAX)
> > +        return EINVAL;
> 
> ... and here, use virAsprintf instead of snprintf.
> If the name ends up being too long, you'll catch it when fopen fails.
 
ok, replaced locally.

> > +    istmp = 1;
> > +
> > +    if (!(f = fopen(tmp, "w"))) {
> > +        istmp = 0;
> > +        if (!(f = fopen(path, "w")))
> > +            return errno;
> > +    }
> > +
> > +    for (i = 0; i < nhosts; i++) {
> > +        if (fputs(hosts[i].host, f) == EOF || fputc('\n', f) == EOF) {
> > +            fclose(f);
> 
> fclose may well clobber the fputs-set errno.
> Save it, with this, just before the fclose.
> 
>   int saved_errno = errno
> 
> > +            if (istmp)
> > +                unlink(tmp);
> 
> unlink may clobber errno, too.
> So here, you should use "return saved_errno;"
> 
> > +            return errno;
> > +        }
> > +    }
> > +
> > +    fclose(f);
> 
> The fputs calls can succeed, yet fclose's write or close
> can still fail.  So you must check for fclose failure, too.
> 
> > +
> > +    if (istmp && rename(tmp, path) < 0) {
> 
> Save errno here too, so unlink can't clobber it.
> 
>     int saved_errno = errno
> 
> > +        unlink(tmp);
> > +        return errno;
> 
> and use saved_errno here.
 
done.

> > +    }
> > +
> > +    if (istmp)
> > +        unlink(tmp);
> > +
> > +    return 0;
> > +}

-- 
Thanks,
Satoru SATOH


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