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

Re: [libvirt] [PATCH] network: Fix dnsmasq hostsfile creation logic and related tests



On 06/28/2011 01:10 PM, Matthias Bolte wrote:
> networkSaveDnsmasqHostsfile was added in 8fa9c2214247 (Apr 2010).
> It has a force flag. If the dnsmasq hostsfile already exists force
> needs to be true to overwrite it. networkBuildDnsmasqArgv sets force
> to false, networkDefine sets it to true. This results in the
> hostsfile being written only in networkDefine in the common case.
> If no error occurred networkSaveDnsmasqHostsfile returns true and
> networkBuildDnsmasqArgv adds the --dhcp-hostsfile to the dnsmasq
> command line.
>
> networkSaveDnsmasqHostsfile was changed in 89ae9849f744 (24 Jun 2011)
> to return a new dnsmasqContext instead of reusing one. This change broke
> the logic of the force flag as now networkSaveDnsmasqHostsfile returns
> NULL on error, but the early return -- if force was not set and the
> hostsfile exists -- returns 0. This turned the early return in an error
> case and networkBuildDnsmasqArgv didn't add the --dhcp-hostsfile option
> anymore if the hostsfile already exists. It did because networkDefine
> created the hostsfile already.
>
> Then 9d4e2845d498 fixed the return 0 case in networkSaveDnsmasqHostsfile
> but didn't apply the force option correctly to the new addnhosts file.
> Now force doesn't control an early return anymore, but influences the
> handling of the hostsfile context creation and dnsmasqSave is always
> called now. This commit also added test cases that reveal several
> problems. First, the tests now call functions that try to write the
> dnsmasq config files to disk. If someone runs this tests as root this
> might overwrite actively used dnsmasq config files, this is a no-go. Also
> the tests depend onconfigure --localstatedir, this needs to be fixed as
> well, because it makes the tests fail when localstatedir is different
> from /var.
>
> This patch does several things to fix this:
>
> 1) Move dnsmasqContext creation and saving out of networkBuildDnsmasqArgv
> to the caller to separate the command line generation from the config
> file writing. This makes the command line generation testable without the
> risk of interfering with system files, because the tests just don't call
> dnsmasqSave.
>
> 2) This refactoring of networkSaveDnsmasqHostsfile makes the force flag
> useless as the saving happens somewhere else now. This fixes the wrong
> usage of the force flag in combination with then newly added addnhosts
> file by removing the force flag.
>
> 3) Adapt the wrong test cases to the correct behavior, by adding the
> missing --dhcp-hostsfile option. Both affected tests contain DHCP host
> elements but missed the necessary --dhcp-hostsfile option.
>
> 4) Rename networkSaveDnsmasqHostsfile to networkBuildDnsmasqHostsfile,
> because it doesn't save the dnsmasqContext anymore.
>
> 5) Move all directory creations in dnsmasq context handling code from
> the *New functions to dnsmasqSave to avoid directory creations in system
> paths in the test cases.
>
> 6) Now that networkBuildDnsmasqArgv doesn't create the dnsmasqContext
> anymore the test case can create one with the localstatedir that is
> expected by the tests instead of the configure --localstatedir given one.
> ---
>  src/network/bridge_driver.c                        |   71 +++++++++-----------
>  src/network/bridge_driver.h                        |    5 +-
>  src/util/dnsmasq.c                                 |   28 ++++----
>  src/util/dnsmasq.h                                 |    1 +
>  .../nat-network-dns-txt-record.argv                |    3 +-
>  tests/networkxml2argvdata/nat-network.argv         |    3 +-
>  tests/networkxml2argvtest.c                        |    8 ++-
>  7 files changed, 63 insertions(+), 56 deletions(-)
>
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index aa170d6..f48fdcb 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -436,27 +436,17 @@ networkShutdown(void) {
>  }
>  
>  
> -static dnsmasqContext*
> -networkSaveDnsmasqHostsfile(virNetworkIpDefPtr ipdef,
> -                            virNetworkDNSDefPtr dnsdef,
> -                            char *name,
> -                            bool force)
> +static int
> +networkBuildDnsmasqHostsfile(dnsmasqContext *dctx,
> +                             virNetworkIpDefPtr ipdef,
> +                             virNetworkDNSDefPtr dnsdef)
>  {
>      unsigned int i, j;
>  
> -    dnsmasqContext *dctx = dnsmasqContextNew(name,
> -                                             DNSMASQ_STATE_DIR);
> -    if (dctx == NULL) {
> -        virReportOOMError();
> -        goto cleanup;
> -    }
> -
> -    if (!(! force && virFileExists(dctx->hostsfile->path))) {
> -        for (i = 0; i < ipdef->nhosts; i++) {
> -            virNetworkDHCPHostDefPtr host = &(ipdef->hosts[i]);
> -            if ((host->mac) && VIR_SOCKET_HAS_ADDR(&host->ip))
> -                dnsmasqAddDhcpHost(dctx, host->mac, &host->ip, host->name);
> -        }
> +    for (i = 0; i < ipdef->nhosts; i++) {
> +        virNetworkDHCPHostDefPtr host = &(ipdef->hosts[i]);
> +        if ((host->mac) && VIR_SOCKET_HAS_ADDR(&host->ip))
> +            dnsmasqAddDhcpHost(dctx, host->mac, &host->ip, host->name);
>      }
>  
>      if (dnsdef) {
> @@ -469,15 +459,7 @@ networkSaveDnsmasqHostsfile(virNetworkIpDefPtr ipdef,
>          }
>      }
>  
> -    if (dnsmasqSave(dctx) < 0)
> -        goto cleanup;
> -
> -    return dctx;
> -
> -cleanup:
> -    dnsmasqContextFree(dctx);
> -
> -    return NULL;
> +    return 0;
>  }
>  
>  
> @@ -485,12 +467,13 @@ static int
>  networkBuildDnsmasqArgv(virNetworkObjPtr network,
>                          virNetworkIpDefPtr ipdef,
>                          const char *pidfile,
> -                        virCommandPtr cmd) {
> +                        virCommandPtr cmd,
> +                        dnsmasqContext *dctx)
> +{
>      int r, ret = -1;
>      int nbleases = 0;
>      int ii;
>      virNetworkIpDefPtr tmpipdef;
> -    dnsmasqContext *dctx = NULL;
>  
>      /*
>       * NB, be careful about syntax for dnsmasq options in long format.
> @@ -617,14 +600,13 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
>          if (ipdef->nranges || ipdef->nhosts)
>              virCommandAddArg(cmd, "--dhcp-no-override");
>  
> -        if ((dctx = networkSaveDnsmasqHostsfile(ipdef, network->def->dns, network->def->name, false))) {
> +        if (networkBuildDnsmasqHostsfile(dctx, ipdef, network->def->dns) >= 0) {
>              if (dctx->hostsfile->nhosts)
>                  virCommandAddArgPair(cmd, "--dhcp-hostsfile",
>                                       dctx->hostsfile->path);
>              if (dctx->addnhostsfile->nhosts)
>                  virCommandAddArgPair(cmd, "--addn-hosts",
>                                       dctx->addnhostsfile->path);
> -            dnsmasqContextFree(dctx);
>          }
>  
>          if (ipdef->tftproot) {
> @@ -655,7 +637,7 @@ cleanup:
>  
>  int
>  networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout,
> -                                  char *pidfile)
> +                                  char *pidfile, dnsmasqContext *dctx)
>  {
>      virCommandPtr cmd = NULL;
>      int ret = -1, ii;
> @@ -684,7 +666,7 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdou
>          return 0;
>  
>      cmd = virCommandNew(DNSMASQ);
> -    if (networkBuildDnsmasqArgv(network, ipdef, pidfile, cmd) < 0) {
> +    if (networkBuildDnsmasqArgv(network, ipdef, pidfile, cmd, dctx) < 0) {
>          goto cleanup;
>      }
>  
> @@ -704,6 +686,7 @@ networkStartDhcpDaemon(virNetworkObjPtr network)
>      char *pidfile = NULL;
>      int ret = -1;
>      int err;
> +    dnsmasqContext *dctx = NULL;
>  
>      if ((err = virFileMakePath(NETWORK_PID_DIR)) != 0) {
>          virReportSystemError(err,
> @@ -730,8 +713,16 @@ networkStartDhcpDaemon(virNetworkObjPtr network)
>          goto cleanup;
>      }
>  
> -    ret = networkBuildDhcpDaemonCommandLine(network,&cmd, pidfile);
> -    if (ret<  0)
> +    dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR);
> +    if (dctx == NULL)
> +        goto cleanup;
> +
> +    ret = networkBuildDhcpDaemonCommandLine(network, &cmd, pidfile, dctx);
> +    if (ret < 0)
> +        goto cleanup;
> +
> +    ret = dnsmasqSave(dctx);
> +    if (ret < 0)
>          goto cleanup;
>  
>      if (virCommandRun(cmd, NULL) < 0)
> @@ -753,6 +744,7 @@ networkStartDhcpDaemon(virNetworkObjPtr network)
>  cleanup:
>      VIR_FREE(pidfile);
>      virCommandFree(cmd);
> +    dnsmasqContextFree(dctx);
>      return ret;
>  }
>  
> @@ -2205,6 +2197,7 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) {
>      virNetworkObjPtr network = NULL;
>      virNetworkPtr ret = NULL;
>      int ii;
> +    dnsmasqContext* dctx = NULL;
>  
>      networkDriverLock(driver);
>  
> @@ -2251,10 +2244,11 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) {
>          }
>      }
>      if (ipv4def) {
> -        dnsmasqContext* dctx = networkSaveDnsmasqHostsfile(ipv4def, network->def->dns, network->def->name, true);
> -        if (dctx == NULL)
> +        dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR);
> +        if (dctx == NULL ||
> +            networkBuildDnsmasqHostsfile(dctx, ipv4def, network->def->dns) < 0 ||
> +            dnsmasqSave(dctx) < 0)
>              goto cleanup;
> -        dnsmasqContextFree(dctx);
>      }
>  
>      VIR_INFO("Defining network '%s'", network->def->name);
> @@ -2262,6 +2256,7 @@ static virNetworkPtr networkDefine(virConnectPtr conn, const char *xml) {
>  
>  cleanup:
>      virNetworkDefFree(def);
> +    dnsmasqContextFree(dctx);
>      if (network)
>          virNetworkObjUnlock(network);
>      networkDriverUnlock(driver);
> diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h
> index a106e3d..2896c84 100644
> --- a/src/network/bridge_driver.h
> +++ b/src/network/bridge_driver.h
> @@ -30,9 +30,12 @@
>  # include "internal.h"
>  # include "network_conf.h"
>  # include "command.h"
> +# include "dnsmasq.h"
>  
>  int networkRegister(void);
> -int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, char *pidfile);
> +int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network,
> +                                      virCommandPtr *cmdout, char *pidfile,
> +                                      dnsmasqContext *dctx);
>  
>  typedef char *(*networkDnsmasqLeaseFileNameFunc)(const char *netname);
>  
> diff --git a/src/util/dnsmasq.c b/src/util/dnsmasq.c
> index 04a912a..4bdbb44 100644
> --- a/src/util/dnsmasq.c
> +++ b/src/util/dnsmasq.c
> @@ -142,7 +142,6 @@ static dnsmasqAddnHostsfile *
>  addnhostsNew(const char *name,
>               const char *config_dir)
>  {
> -    int err;
>      dnsmasqAddnHostsfile *addnhostsfile;
>  
>      if (VIR_ALLOC(addnhostsfile) < 0) {
> @@ -159,12 +158,6 @@ addnhostsNew(const char *name,
>          goto error;
>      }
>  
> -    if ((err = virFileMakePath(config_dir))) {
> -        virReportSystemError(err, _("cannot create config directory '%s'"),
> -                             config_dir);
> -        goto error;
> -    }
> -
>      return addnhostsfile;
>  
>   error:
> @@ -344,7 +337,6 @@ static dnsmasqHostsfile *
>  hostsfileNew(const char *name,
>               const char *config_dir)
>  {
> -    int err;
>      dnsmasqHostsfile *hostsfile;
>  
>      if (VIR_ALLOC(hostsfile) < 0) {
> @@ -361,12 +353,6 @@ hostsfileNew(const char *name,
>          goto error;
>      }
>  
> -    if ((err = virFileMakePath(config_dir))) {
> -        virReportSystemError(err, _("cannot create config directory '%s'"),
> -                             config_dir);
> -        goto error;
> -    }
> -
>      return hostsfile;
>  
>   error:
> @@ -468,6 +454,11 @@ dnsmasqContextNew(const char *network_name,
>          return NULL;
>      }
>  
> +    if (!(ctx->config_dir = strdup(config_dir))) {
> +        virReportOOMError();
> +        goto error;
> +    }
> +
>      if (!(ctx->hostsfile = hostsfileNew(network_name, config_dir)))
>          goto error;
>      if (!(ctx->addnhostsfile = addnhostsNew(network_name, config_dir)))
> @@ -492,6 +483,8 @@ dnsmasqContextFree(dnsmasqContext *ctx)
>      if (!ctx)
>          return;
>  
> +    VIR_FREE(ctx->config_dir);
> +
>      if (ctx->hostsfile)
>          hostsfileFree(ctx->hostsfile);
>      if (ctx->addnhostsfile)
> @@ -546,8 +539,15 @@ dnsmasqAddHost(dnsmasqContext *ctx,
>  int
>  dnsmasqSave(const dnsmasqContext *ctx)
>  {
> +    int err;
>      int ret = 0;
>  
> +    if ((err = virFileMakePath(ctx->config_dir))) {
> +        virReportSystemError(err, _("cannot create config directory '%s'"),
> +                             ctx->config_dir);
> +        return -1;
> +    }
> +
>      if (ctx->hostsfile)
>          ret = hostsfileSave(ctx->hostsfile);
>      if (ret == 0) {
> diff --git a/src/util/dnsmasq.h b/src/util/dnsmasq.h
> index 3f6320a..62f6f38 100644
> --- a/src/util/dnsmasq.h
> +++ b/src/util/dnsmasq.h
> @@ -60,6 +60,7 @@ typedef struct
>  
>  typedef struct
>  {
> +    char                 *config_dir;
>      dnsmasqHostsfile     *hostsfile;
>      dnsmasqAddnHostsfile *addnhostsfile;
>  } dnsmasqContext;
> diff --git a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv
> index 9d74543..be6ba4b 100644
> --- a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv
> +++ b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv
> @@ -5,4 +5,5 @@
>  --listen-address 2001:db8:ac10:fd01::1 --listen-address 10.24.10.1 \
>  --dhcp-range 192.168.122.2,192.168.122.254 \
>  --dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases \
> ---dhcp-lease-max=253 --dhcp-no-override\
> +--dhcp-lease-max=253 --dhcp-no-override \
> +--dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile\
> diff --git a/tests/networkxml2argvdata/nat-network.argv b/tests/networkxml2argvdata/nat-network.argv
> index 181623f..d7faee2 100644
> --- a/tests/networkxml2argvdata/nat-network.argv
> +++ b/tests/networkxml2argvdata/nat-network.argv
> @@ -4,4 +4,5 @@
>  --listen-address 2001:db8:ac10:fd01::1 --listen-address 10.24.10.1 \
>  --dhcp-range 192.168.122.2,192.168.122.254 \
>  --dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases \
> ---dhcp-lease-max=253 --dhcp-no-override\
> +--dhcp-lease-max=253 --dhcp-no-override \
> +--dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile\
> diff --git a/tests/networkxml2argvtest.c b/tests/networkxml2argvtest.c
> index 62de191..4a11d6f 100644
> --- a/tests/networkxml2argvtest.c
> +++ b/tests/networkxml2argvtest.c
> @@ -24,6 +24,7 @@ static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) {
>      virNetworkObjPtr obj = NULL;
>      virCommandPtr cmd = NULL;
>      char *pidfile = NULL;
> +    dnsmasqContext *dctx = NULL;
>  
>      if (virtTestLoadFile(inxml, &inXmlData) < 0)
>          goto fail;
> @@ -38,8 +39,12 @@ static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) {
>          goto fail;
>  
>      obj->def = dev;
> +    dctx = dnsmasqContextNew(dev->name, "/var/lib/libvirt/dnsmasq");
>  
> -    if (networkBuildDhcpDaemonCommandLine(obj, &cmd, pidfile) < 0)
> +    if (dctx == NULL)
> +        goto fail;
> +
> +    if (networkBuildDhcpDaemonCommandLine(obj, &cmd, pidfile, dctx) < 0)
>          goto fail;
>  
>      if (!(actual = virCommandToString(cmd)))
> @@ -59,6 +64,7 @@ static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) {
>      VIR_FREE(pidfile);
>      virCommandFree(cmd);
>      virNetworkObjFree(obj);
> +    dnsmasqContextFree(dctx);
>      return ret;
>  }
>  
ACK

Michal

-- 
Michal Novotny <minovotn redhat com>, RHCE, Red Hat
Virtualization | libvirt-php bindings | php-virt-control.org


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