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

Re: [libvirt] [PATCH v2 4/5] Network: Add additional hosts internal infrastructure



On 04/01/2011 06:45 AM, Michal Novotny wrote:
Hi,
this is the patch to introduce the internal infrastructure for
additional hosts for network bridge driver using the addnhosts*
API functions.

This is necessary for next part of the patch to support DNS
hosts definition in the network XML description.

Michal

Signed-off-by: Michal Novotny<minovotn redhat com>
---
  src/libvirt_private.syms    |    1 +
  src/network/bridge_driver.c |    3 +
  src/util/dnsmasq.c          |  266 ++++++++++++++++++++++++++++++++++++++++++-
  src/util/dnsmasq.h          |   22 ++++-
  4 files changed, 287 insertions(+), 5 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 65a86d3..73c3f77 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -186,6 +186,7 @@ virUnrefStream;

  # dnsmasq.h
  dnsmasqAddDhcpHost;
+dnsmasqAddHost;
  dnsmasqContextFree;
  dnsmasqContextNew;
  dnsmasqDelete;
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 41b14f9..4ad3143 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -589,6 +589,9 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
              if (dctx->hostsfile->nhosts)
                  virCommandAddArgPair(cmd, "--dhcp-hostsfile",
                                       dctx->hostsfile->path);
+            if (dctx->addnhostsfile->nhosts)
+                virCommandAddArgPair(cmd, "--addn-hosts",
+                                     dctx->addnhostsfile->path);

              dnsmasqContextFree(dctx);
          }
diff --git a/src/util/dnsmasq.c b/src/util/dnsmasq.c
index be230e1..fee3b90 100644
--- a/src/util/dnsmasq.c
+++ b/src/util/dnsmasq.c
@@ -48,6 +48,7 @@

  #define VIR_FROM_THIS VIR_FROM_NETWORK
  #define DNSMASQ_HOSTSFILE_SUFFIX "hostsfile"
+#define DNSMASQ_ADDNHOSTSFILE_SUFFIX "addnhosts"

  static void
  dhcphostFree(dnsmasqDhcpHost *host)
@@ -56,6 +57,231 @@ dhcphostFree(dnsmasqDhcpHost *host)
  }

  static void
+addnhostFree(dnsmasqAddnHost *host)
+{
+    VIR_FREE(host->hostnames);
+    VIR_FREE(host->ip);
+}
+
+static void
+addnhostsFree(dnsmasqAddnHostsfile *addnhostsfile)
+{
+    unsigned int i;
+
+    if (addnhostsfile->hosts) {
+        for (i = 0; i<  addnhostsfile->nhosts; i++)
+            addnhostFree(&addnhostsfile->hosts[i]);
+
+        VIR_FREE(addnhostsfile->hosts);
+
+        addnhostsfile->nhosts = 0;
+    }
+
+    VIR_FREE(addnhostsfile->path);
+
+    VIR_FREE(addnhostsfile);
+}
+
+static int
+addnhostsAdd(dnsmasqAddnHostsfile *addnhostsfile,
+             virSocketAddr *ip,
+             const char *name)
+{
+    char *ipstr = NULL;
+    int idx = -1;
+    int i;
+
+    if (!(ipstr = virSocketFormatAddr(ip)))
+        return -1;
+
+    for (i = 0; i<  addnhostsfile->nhosts; i++) {
+        if (STREQ((const char *)addnhostsfile->hosts[i].ip, (const char *)ipstr)) {
+            idx = i;
+            break;
+        }
+    }
+
+    if (idx<  0) {
+        if (VIR_REALLOC_N(addnhostsfile->hosts, addnhostsfile->nhosts + 1)<  0)
+            goto alloc_error;
+
+        idx = addnhostsfile->nhosts;
+        if (VIR_ALLOC(addnhostsfile->hosts[idx].hostnames)<  0)
+            goto alloc_error;
+
+        if (virAsprintf(&addnhostsfile->hosts[idx].ip, "%s", ipstr)<  0)
+            goto alloc_error;
+
+        addnhostsfile->hosts[idx].nhostnames = 0;
+        addnhostsfile->nhosts++;
+    }
+
+    if (VIR_REALLOC_N(addnhostsfile->hosts[idx].hostnames, addnhostsfile->hosts[idx].nhostnames + 1)<  0)
+        goto alloc_error;
+
+    if (virAsprintf(&addnhostsfile->hosts[idx].hostnames[addnhostsfile->hosts[idx].nhostnames], "%s", name)<  0)
+        goto alloc_error;
+
+    VIR_FREE(ipstr);
+
+    addnhostsfile->hosts[idx].nhostnames++;
+
+    return 0;
+
+ alloc_error:
+    virReportOOMError();
+    VIR_FREE(ipstr);
+    return -1;
+}
+
+static dnsmasqAddnHostsfile *
+addnhostsNew(const char *name,
+             const char *config_dir)
+{
+    int err;
+    dnsmasqAddnHostsfile *addnhostsfile;
+
+    if (VIR_ALLOC(addnhostsfile)<  0) {
+        virReportOOMError();
+        return NULL;
+    }
+
+    addnhostsfile->hosts = NULL;
+    addnhostsfile->nhosts = 0;
+
+    if (virAsprintf(&addnhostsfile->path, "%s/%s.%s", config_dir, name,
+                    DNSMASQ_ADDNHOSTSFILE_SUFFIX)<  0) {
+        virReportOOMError();
+        goto error;
+    }
+
+    if ((err = virFileMakePath(config_dir))) {
+        virReportSystemError(err, _("cannot create config directory '%s'"),
+                             config_dir);
+        goto error;
+    }
+
+    return addnhostsfile;
+
+ error:
+    addnhostsFree(addnhostsfile);
+    return NULL;
+}
+
+static int
+addnhostsWrite(const char *path,
+               dnsmasqAddnHost *hosts,
+               unsigned int nhosts)
+{
+    char *tmp;
+    FILE *f;
+    bool istmp = true;
+    unsigned int i, ii;
+    int rc = 0;
+
+    if (nhosts == 0)
+        return rc;
+
+    if (virAsprintf(&tmp, "%s.new", path)<  0)
+        return ENOMEM;


Oooh! Figuring that you had cut-pasted this code from the existing hostsFileWrite(), and knowing that in general we like to return -errno on failure (although there are still some exceptions), I went to look at the original code and found a bug! (hostsWriteFile() is returning errno on failure, but at least one caller is checking for ret < 0).

I'm sending in a patch for that momentarily, and you should base the next revision of your code on that.


+
+    if (!(f = fopen(tmp, "w"))) {
+        istmp = false;
+        if (!(f = fopen(path, "w"))) {
+            rc = errno;
+            goto cleanup;
+        }
+    }
+
+    for (i = 0; i<  nhosts; i++) {
+        if (fputs(hosts[i].ip, f) == EOF || fputc('\t', f) == EOF) {
+            rc = errno;
+            VIR_FORCE_FCLOSE(f);
+
+            if (istmp)
+                unlink(tmp);
+
+            goto cleanup;
+        }
+
+        for (ii = 0; ii<  hosts[i].nhostnames; ii++) {
+            if (fputs(hosts[i].hostnames[ii], f) == EOF || fputc('\t', f) == EOF) {
+                rc = errno;
+                VIR_FORCE_FCLOSE(f);
+
+                if (istmp)
+                    unlink(tmp);
+
+                goto cleanup;
+            }
+        }
+
+        if (fputc('\n', f) == EOF) {
+            rc = errno;
+            VIR_FORCE_FCLOSE(f);
+
+            if (istmp)
+                unlink(tmp);
+
+            goto cleanup;
+        }
+    }
+
+    if (VIR_FCLOSE(f) == EOF) {
+        rc = errno;
+        goto cleanup;
+    }
+
+    if (istmp) {
+        if (rename(tmp, path)<  0) {
+            rc = errno;
+            unlink(tmp);
+            goto cleanup;
+        }
+
+        if (unlink(tmp)<  0) {
+            rc = errno;
+            goto cleanup;
+        }
+    }
+
+ cleanup:
+    VIR_FREE(tmp);
+
+    return rc;
+}
+
+static int
+addnhostsSave(dnsmasqAddnHostsfile *addnhostsfile)
+{
+    int err = addnhostsWrite(addnhostsfile->path, addnhostsfile->hosts,
+                             addnhostsfile->nhosts);
+
+    if (err<  0) {
+        virReportSystemError(err, _("cannot write config file '%s'"),
+                             addnhostsfile->path);
+        return -1;
+    }
+
+    return 0;
+}
+
+static int
+addnhostsDelete(dnsmasqAddnHostsfile *addnhostsfile)
+{
+    if (!virFileExists(addnhostsfile->path))
+        return 0;
+
+    if (unlink(addnhostsfile->path)<  0) {
+        virReportSystemError(errno, _("cannot remove config file '%s'"),
+                             addnhostsfile->path);
+        return -1;
+    }
+
+    return 0;
+}

Except for the type of the arg, this function is identical to hostsfileDelete. How about changing hostsfileDelete() to take a "const char *file", then changing its name to something more generic, and calling the same function from both places?


+
+static void
  hostsfileFree(dnsmasqHostsfile *hostsfile)
  {
      unsigned int i;
@@ -255,6 +481,8 @@ dnsmasqContextNew(const char *network_name,

      if (!(ctx->hostsfile = hostsfileNew(network_name, config_dir)))
          goto error;
+    if (!(ctx->addnhostsfile = addnhostsNew(network_name, config_dir)))
+        goto error;

      return ctx;

@@ -277,6 +505,8 @@ dnsmasqContextFree(dnsmasqContext *ctx)

      if (ctx->hostsfile)
          hostsfileFree(ctx->hostsfile);
+    if (ctx->addnhostsfile)
+        addnhostsFree(ctx->addnhostsfile);

      VIR_FREE(ctx);
  }
@@ -300,6 +530,24 @@ dnsmasqAddDhcpHost(dnsmasqContext *ctx,
          hostsfileAdd(ctx->hostsfile, mac, ip, name);
  }

+/*
+ * dnsmasqAddHost:
+ * @ctx: pointer to the dnsmasq context for each network
+ * @ip: pointer to the socket address contains ip of the host
+ * @name: pointer to the string contains hostname of the host
+ *
+ * Add additional host entry.
+ */
+
+void
+dnsmasqAddHost(dnsmasqContext *ctx,
+               virSocketAddr *ip,
+               const char *name)
+{
+    if (ctx->addnhostsfile)
+        addnhostsAdd(ctx->addnhostsfile, ip, name);
+}
+
  /**
   * dnsmasqSave:
   * @ctx: pointer to the dnsmasq context for each network
@@ -309,10 +557,15 @@ dnsmasqAddDhcpHost(dnsmasqContext *ctx,
  int
  dnsmasqSave(const dnsmasqContext *ctx)
  {
+    int ret1 = 0;
+    int ret2 = 0;
+
      if (ctx->hostsfile)
-        return hostsfileSave(ctx->hostsfile);
+        ret1 = hostsfileSave(ctx->hostsfile);
+    if (ctx->addnhostsfile)
+        ret2 = addnhostsSave(ctx->addnhostsfile);

-    return 0;
+    return ((ret1 == 0)&&  (ret2 == 0)) ? 0 : -1;


Does it really need to be this complicated? Or can you just have a single "ret", and skip the 2nd save if the first save fails?


  }


@@ -325,10 +578,15 @@ dnsmasqSave(const dnsmasqContext *ctx)
  int
  dnsmasqDelete(const dnsmasqContext *ctx)
  {
+    int ret1 = 0;
+    int ret2 = 0;
+
      if (ctx->hostsfile)
-        return hostsfileDelete(ctx->hostsfile);
+        ret1 = hostsfileDelete(ctx->hostsfile);
+    if (ctx->addnhostsfile)
+        ret2 = addnhostsDelete(ctx->addnhostsfile);

-    return 0;
+    return ((ret1 == 0)&&  (ret2 == 0)) ? 0 : -1;

I think the "try 2nd even if 1st fails" method *is* appropriate here though.

  }

  /**
diff --git a/src/util/dnsmasq.h b/src/util/dnsmasq.h
index 02a961f..3f6320a 100644
--- a/src/util/dnsmasq.h
+++ b/src/util/dnsmasq.h
@@ -44,7 +44,24 @@ typedef struct

  typedef struct
  {
-    dnsmasqHostsfile *hostsfile;
+    unsigned int    nhostnames;
+    char            *ip;
+    char            **hostnames;
+
+} dnsmasqAddnHost;
+
+typedef struct
+{
+    unsigned int     nhosts;
+    dnsmasqAddnHost *hosts;
+
+    char            *path;  /* Absolute path of dnsmasq's hostsfile. */
+} dnsmasqAddnHostsfile;
+
+typedef struct
+{
+    dnsmasqHostsfile     *hostsfile;
+    dnsmasqAddnHostsfile *addnhostsfile;
  } dnsmasqContext;

  dnsmasqContext * dnsmasqContextNew(const char *network_name,
@@ -54,6 +71,9 @@ void             dnsmasqAddDhcpHost(dnsmasqContext *ctx,
                                      const char *mac,
                                      virSocketAddr *ip,
                                      const char *name);
+void             dnsmasqAddHost(dnsmasqContext *ctx,
+                                virSocketAddr *ip,
+                                const char *name);
  int              dnsmasqSave(const dnsmasqContext *ctx);
  int              dnsmasqDelete(const dnsmasqContext *ctx);
  int              dnsmasqReload(pid_t pid);


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