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

22 Apr 2010


      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