[libvirt] [PATCH] network: Fix dnsmasq hostsfile creation logic and related tests
Michal Novotny
minovotn at redhat.com
Tue Jun 28 15:19:02 UTC 2011
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 at redhat.com>, RHCE, Red Hat
Virtualization | libvirt-php bindings | php-virt-control.org
More information about the libvir-list
mailing list