[libvirt] [PATCH 0/2] v3: dnsmasq conf-file and interface= patches
I am sorry about the clutter with my multiple patch submissions. I am just trynig to get it right so that, the next time, I will not make so many stupid mistakes. Gene Czarcinski (2): v3: put dnsmasq parameters into a file instead of the command line v3: add dnsmasq interface= parameter so bind-interfaces works src/network/bridge_driver.c | 183 ++++++++++++++------- src/network/bridge_driver.h | 8 +- tests/networkxml2argvdata/isolated-network.argv | 25 ++- .../networkxml2argvdata/nat-network-dns-hosts.argv | 15 +- .../nat-network-dns-srv-record-minimal.argv | 36 ++-- .../nat-network-dns-srv-record.argv | 36 ++-- .../nat-network-dns-txt-record.argv | 30 ++-- tests/networkxml2argvdata/nat-network.argv | 28 ++-- tests/networkxml2argvdata/netboot-network.argv | 29 ++-- .../networkxml2argvdata/netboot-proxy-network.argv | 26 ++- tests/networkxml2argvdata/routed-network.argv | 13 +- tests/networkxml2argvtest.c | 44 +---- 12 files changed, 278 insertions(+), 195 deletions(-) -- 1.7.11.7
This patch changes the way parameters are passed to dnsmasq. They are put into a conf-file instead of being on the dnsmasq command line. **NOTE ** This has updated the related tests for the new data format, etc. **NOTE** This patch does NOT include specifying interface= The command line now contains --conf-file=<filename> and a new parameter --conf-dir=<directoryname> has been added. The new file and directory are put in the same directory as the leases file. --- src/network/bridge_driver.c | 179 ++++++++++++++------- src/network/bridge_driver.h | 8 +- tests/networkxml2argvdata/isolated-network.argv | 24 +-- .../networkxml2argvdata/nat-network-dns-hosts.argv | 14 +- .../nat-network-dns-srv-record-minimal.argv | 35 ++-- .../nat-network-dns-srv-record.argv | 35 ++-- .../nat-network-dns-txt-record.argv | 29 ++-- tests/networkxml2argvdata/nat-network.argv | 27 ++-- tests/networkxml2argvdata/netboot-network.argv | 28 ++-- .../networkxml2argvdata/netboot-proxy-network.argv | 25 +-- tests/networkxml2argvdata/routed-network.argv | 12 +- tests/networkxml2argvtest.c | 44 +---- 12 files changed, 264 insertions(+), 196 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8837843..ab13df5 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -136,6 +136,16 @@ networkDnsmasqLeaseFileNameFunc networkDnsmasqLeaseFileName = networkDnsmasqLeaseFileNameDefault; static char * +networkDnsmasqConfigFileName(const char *netname) +{ + char *conffile; + + ignore_value(virAsprintf(&conffile, DNSMASQ_STATE_DIR "/%s.conf", + netname)); + return conffile; +} + +static char * networkRadvdPidfileBasename(const char *netname) { /* this is simple but we want to be sure it's consistently done */ @@ -559,23 +569,25 @@ networkBuildDnsmasqHostsfile(dnsmasqContext *dctx, return 0; } - + /* build the dnsmasq conf file contents */ static int -networkBuildDnsmasqArgv(virNetworkObjPtr network, +networkDnsmasqConfContents(virNetworkObjPtr network, virNetworkIpDefPtr ipdef, const char *pidfile, - virCommandPtr cmd, + char **configstr, dnsmasqContext *dctx) { - int r, ret = -1; + virBuffer configbuf = VIR_BUFFER_INITIALIZER;; + int r, ret = -1, ii; int nbleases = 0; - int ii; char *record = NULL; char *recordPort = NULL; char *recordWeight = NULL; char *recordPriority = NULL; virNetworkIpDefPtr tmpipdef; + *configstr = NULL; + /* * NB, be careful about syntax for dnsmasq options in long format. * @@ -595,28 +607,22 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, * very explicit on this. */ - /* - * Needed to ensure dnsmasq uses same algorithm for processing - * multiple namedriver entries in /etc/resolv.conf as GLibC. - */ - virCommandAddArgList(cmd, "--strict-order", "--bind-interfaces", NULL); - + /* create dnsmasq config file appropriate for this network */ + virBufferAsprintf(&configbuf, "# dnsmasq conf file created by libvirt\n" + "strict-order\n" + "bind-interfaces\n" + "except-interface=lo\n" + "domain-needed\n" + "local=/%s/\n", + network->def->domain ? network->def->domain : ""); if (network->def->domain) - virCommandAddArgPair(cmd, "--domain", network->def->domain); - /* need to specify local even if no domain specified */ - virCommandAddArgFormat(cmd, "--local=/%s/", - network->def->domain ? network->def->domain : ""); - virCommandAddArg(cmd, "--domain-needed"); + virBufferAsprintf(&configbuf, + "domain=%s\n" + "expand-hosts\n", + network->def->domain); if (pidfile) - virCommandAddArgPair(cmd, "--pid-file", pidfile); - - /* *no* conf file */ - virCommandAddArg(cmd, "--conf-file="); - - virCommandAddArgList(cmd, - "--except-interface", "lo", - NULL); + virBufferAsprintf(&configbuf, "pid-file=%s\n", pidfile); /* If this is an isolated network, set the default route option * (3) to be empty to avoid setting a default route that's @@ -626,16 +632,21 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, * to build a connection to the outside). */ if (network->def->forwardType == VIR_NETWORK_FORWARD_NONE) { - virCommandAddArgList(cmd, "--dhcp-option=3", - "--no-resolv", NULL); + virBufferAsprintf(&configbuf, "dhcp-option=3\n" + "no-resolv\n"); } + /* + * Needed to ensure dnsmasq uses same algorithm for processing + * multiple namedriver entries in /etc/resolv.conf as GLibC. + */ + if (network->def->dns != NULL) { virNetworkDNSDefPtr dns = network->def->dns; int i; for (i = 0; i < dns->ntxtrecords; i++) { - virCommandAddArgFormat(cmd, "--txt-record=%s,%s", + virBufferAsprintf(&configbuf, "txt-record=%s,%s\n", dns->txtrecords[i].name, dns->txtrecords[i].value); } @@ -673,7 +684,7 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, goto cleanup; } - virCommandAddArgPair(cmd, "--srv-host", record); + virBufferAsprintf(&configbuf, "srv-host=%s\n", record); VIR_FREE(record); VIR_FREE(recordPort); VIR_FREE(recordWeight); @@ -682,21 +693,14 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, } } - /* - * --interface does not actually work with dnsmasq < 2.47, - * due to DAD for ipv6 addresses on the interface. - * - * virCommandAddArgList(cmd, "--interface", ipdef->bridge, NULL); - * - * So listen on all defined IPv[46] addresses - */ for (ii = 0; (tmpipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii)); - ii++) { + ii++) + { char *ipaddr = virSocketAddrFormat(&tmpipdef->address); if (!ipaddr) goto cleanup; - virCommandAddArgList(cmd, "--listen-address", ipaddr, NULL); + virBufferAsprintf(&configbuf, "listen-address=%s\n", ipaddr); VIR_FREE(ipaddr); } @@ -710,8 +714,8 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, VIR_FREE(saddr); goto cleanup; } - virCommandAddArg(cmd, "--dhcp-range"); - virCommandAddArgFormat(cmd, "%s,%s", saddr, eaddr); + virBufferAsprintf(&configbuf, "dhcp-range=%s,%s\n", + saddr, eaddr); VIR_FREE(saddr); VIR_FREE(eaddr); nbleases += virSocketAddrGetRange(&ipdef->ranges[r].start, @@ -727,8 +731,7 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, char *bridgeaddr = virSocketAddrFormat(&ipdef->address); if (!bridgeaddr) goto cleanup; - virCommandAddArg(cmd, "--dhcp-range"); - virCommandAddArgFormat(cmd, "%s,static", bridgeaddr); + virBufferAsprintf(&configbuf, "dhcp-range=%s,static\n", bridgeaddr); VIR_FREE(bridgeaddr); } @@ -736,17 +739,13 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, char *leasefile = networkDnsmasqLeaseFileName(network->def->name); if (!leasefile) goto cleanup; - virCommandAddArgFormat(cmd, "--dhcp-leasefile=%s", leasefile); + virBufferAsprintf(&configbuf, "dhcp-leasefile=%s\n", leasefile); VIR_FREE(leasefile); - virCommandAddArgFormat(cmd, "--dhcp-lease-max=%d", nbleases); + virBufferAsprintf(&configbuf, "dhcp-lease-max=%d\n", nbleases); } if (ipdef->nranges || ipdef->nhosts) - virCommandAddArg(cmd, "--dhcp-no-override"); - - /* add domain to any non-qualified hostnames in /etc/hosts or addn-hosts */ - if (network->def->domain) - virCommandAddArg(cmd, "--expand-hosts"); + virBufferAsprintf(&configbuf, "dhcp-no-override\n"); if (networkBuildDnsmasqHostsfile(dctx, ipdef, network->def->dns) < 0) goto cleanup; @@ -756,38 +755,42 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, * file to allow for runtime additions. */ if (ipdef->nranges || ipdef->nhosts) - virCommandAddArgPair(cmd, "--dhcp-hostsfile", + virBufferAsprintf(&configbuf, "dhcp-hostsfile=%s\n", dctx->hostsfile->path); /* Likewise, always create this file and put it on the commandline, to allow for * for runtime additions. */ - virCommandAddArgPair(cmd, "--addn-hosts", + virBufferAsprintf(&configbuf, "addn-hosts=%s\n", dctx->addnhostsfile->path); if (ipdef->tftproot) { - virCommandAddArgList(cmd, "--enable-tftp", - "--tftp-root", ipdef->tftproot, - NULL); + virBufferAsprintf(&configbuf, "enable-tftp\n"); + virBufferAsprintf(&configbuf, "tftp-root=%s\n", ipdef->tftproot); } if (ipdef->bootfile) { - virCommandAddArg(cmd, "--dhcp-boot"); if (VIR_SOCKET_ADDR_VALID(&ipdef->bootserver)) { char *bootserver = virSocketAddrFormat(&ipdef->bootserver); if (!bootserver) goto cleanup; - virCommandAddArgFormat(cmd, "%s%s%s", + virBufferAsprintf(&configbuf, "dhcp-boot=%s%s%s\n", ipdef->bootfile, ",,", bootserver); VIR_FREE(bootserver); } else { - virCommandAddArg(cmd, ipdef->bootfile); + virBufferAsprintf(&configbuf, "dhcp-boot=%s\n", ipdef->bootfile); } } } + if (!(*configstr = virBufferContentAndReset(&configbuf))) { + virReportOOMError(); + goto cleanup; + } ret = 0; + cleanup: + virBufferFreeAndReset(&configbuf); VIR_FREE(record); VIR_FREE(recordPort); VIR_FREE(recordWeight); @@ -795,13 +798,18 @@ cleanup: return ret; } + /* build the dnsmasq command line */ int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, - char *pidfile, dnsmasqContext *dctx) + char *pidfile, dnsmasqContext *dctx, + char *configdir, + int testOnly, char **testConfigStr) { virCommandPtr cmd = NULL; int ret = -1, ii; virNetworkIpDefPtr ipdef; + char *configfile = NULL; + char *configstr = NULL; network->dnsmasqPid = -1; @@ -825,15 +833,41 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdou if (!virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, 0)) return 0; - cmd = virCommandNew(DNSMASQ); - if (networkBuildDnsmasqArgv(network, ipdef, pidfile, cmd, dctx) < 0) { + if (networkDnsmasqConfContents(network, ipdef, pidfile, &configstr, dctx) < 0) + goto cleanup; + if (!configstr) + goto cleanup; + + if (testOnly) { + *testConfigStr = configstr; + return 0; + } + + /* construct the filename */ + if (!(configfile = networkDnsmasqConfigFileName(network->def->name))) { + virReportOOMError(); + goto cleanup; + } + + /* Write the file */ + if (virFileWriteStr(configfile, configstr, 0600) < 0) { + virReportSystemError(errno, + _("couldn't write dnsmasq config file '%s'"), + configfile); goto cleanup; } + VIR_INFO("dnsmasq conf file %s written", configfile); + + cmd = virCommandNew(DNSMASQ); + virCommandAddArgFormat(cmd, "--conf-file=%s", configfile); + virCommandAddArgFormat(cmd, "--conf-dir=%s", configdir); if (cmdout) *cmdout = cmd; ret = 0; cleanup: + VIR_FREE(configstr); + VIR_FREE(configfile); if (ret < 0) virCommandFree(cmd); return ret; @@ -844,9 +878,12 @@ networkStartDhcpDaemon(virNetworkObjPtr network) { virCommandPtr cmd = NULL; char *pidfile = NULL; + char *testconfigstr = NULL; + char *configdir = NULL; int ret = -1; dnsmasqContext *dctx = NULL; + VIR_INFO("starting dhcp daemon (dnsmasq)"); if (!virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, 0)) { /* no IPv6 addresses, so we don't need to run radvd */ ret = 0; @@ -882,7 +919,18 @@ networkStartDhcpDaemon(virNetworkObjPtr network) if (dctx == NULL) goto cleanup; - ret = networkBuildDhcpDaemonCommandLine(network, &cmd, pidfile, dctx); + ignore_value(virAsprintf(&configdir, DNSMASQ_STATE_DIR "/%s.d", + network->def->name)); + if (virFileMakePath(configdir) < 0) { + virReportSystemError(errno, + _("cannot create directory %s"), + configdir); + goto cleanup; + } + + + ret = networkBuildDhcpDaemonCommandLine(network, &cmd, pidfile, dctx, + configdir, 0, &testconfigstr); if (ret < 0) goto cleanup; @@ -911,6 +959,7 @@ networkStartDhcpDaemon(virNetworkObjPtr network) ret = 0; cleanup: VIR_FREE(pidfile); + VIR_FREE(configdir); virCommandFree(cmd); dnsmasqContextFree(dctx); return ret; @@ -2841,6 +2890,14 @@ static int networkUndefine(virNetworkPtr net) { } } + { + char *configfile = networkDnsmasqConfigFileName(network->def->name); + if (!configfile) + goto cleanup; + unlink(configfile); + VIR_FREE(configfile); + } + if (dhcp_present) { char *leasefile; dnsmasqContext *dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR); diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index 0fae275..00675c4 100644 --- a/src/network/bridge_driver.h +++ b/src/network/bridge_driver.h @@ -48,15 +48,17 @@ int networkGetNetworkAddress(const char *netname, char **netaddr) int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, char *pidfile, - dnsmasqContext *dctx) - ; + dnsmasqContext *dctx, + char *configdir, + int testOnly, char **testConfigStr); # else /* Define no-op replacements that don't drag in any link dependencies. */ # define networkAllocateActualDevice(iface) 0 # define networkNotifyActualDevice(iface) 0 # define networkReleaseActualDevice(iface) 0 # define networkGetNetworkAddress(netname, netaddr) (-2) -# define networkBuildDhcpDaemonCommandLine(network, cmdout, pidfile, dctx) 0 +# define networkBuildDhcpDaemonCommandLine(network, cmdout, pidfile, \ + dctx, configdir, testOnly, testConfigStr) 0 # endif typedef char *(*networkDnsmasqLeaseFileNameFunc)(const char *netname); diff --git a/tests/networkxml2argvdata/isolated-network.argv b/tests/networkxml2argvdata/isolated-network.argv index 13e77b2..042158b 100644 --- a/tests/networkxml2argvdata/isolated-network.argv +++ b/tests/networkxml2argvdata/isolated-network.argv @@ -1,9 +1,15 @@ -@DNSMASQ@ --strict-order --bind-interfaces \ ---local=// --domain-needed --conf-file= \ ---except-interface lo --dhcp-option=3 --no-resolv \ ---listen-address 192.168.152.1 \ ---dhcp-range 192.168.152.2,192.168.152.254 \ ---dhcp-leasefile=/var/lib/libvirt/dnsmasq/private.leases --dhcp-lease-max=253 \ ---dhcp-no-override \ ---dhcp-hostsfile=/var/lib/libvirt/dnsmasq/private.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/private.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=// +dhcp-option=3 +no-resolv +listen-address=192.168.152.1 +dhcp-range=192.168.152.2,192.168.152.254 +dhcp-leasefile=/var/lib/libvirt/dnsmasq/private.leases +dhcp-lease-max=253 +dhcp-no-override +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/private.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/private.addnhosts diff --git a/tests/networkxml2argvdata/nat-network-dns-hosts.argv b/tests/networkxml2argvdata/nat-network-dns-hosts.argv index 03a0676..91eb682 100644 --- a/tests/networkxml2argvdata/nat-network-dns-hosts.argv +++ b/tests/networkxml2argvdata/nat-network-dns-hosts.argv @@ -1,4 +1,10 @@ -@DNSMASQ@ --strict-order --bind-interfaces --domain=example.com \ ---local=/example.com/ --domain-needed \ ---conf-file= --except-interface lo --listen-address 192.168.122.1 \ ---expand-hosts --addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=/example.com/ +domain=example.com +expand-hosts +listen-address=192.168.122.1 +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv b/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv index 210a60c..d92497b 100644 --- a/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv +++ b/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv @@ -1,17 +1,18 @@ -@DNSMASQ@ \ ---strict-order \ ---bind-interfaces \ ---local=// --domain-needed --conf-file= \ ---except-interface lo \ ---srv-host=name.tcp.,,,, \ ---listen-address 192.168.122.1 \ ---listen-address 192.168.123.1 \ ---listen-address 2001:db8:ac10:fe01::1 \ ---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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=// +srv-host=name.tcp.,,,, +listen-address=192.168.122.1 +listen-address=192.168.123.1 +listen-address=2001:db8:ac10:fe01::1 +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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2argvdata/nat-network-dns-srv-record.argv b/tests/networkxml2argvdata/nat-network-dns-srv-record.argv index 833d3cd..d8846c2 100644 --- a/tests/networkxml2argvdata/nat-network-dns-srv-record.argv +++ b/tests/networkxml2argvdata/nat-network-dns-srv-record.argv @@ -1,17 +1,18 @@ -@DNSMASQ@ \ ---strict-order \ ---bind-interfaces \ ---local=// --domain-needed --conf-file= \ ---except-interface lo \ ---srv-host=name.tcp.test-domain-name,.,1024,10,10 \ ---listen-address 192.168.122.1 \ ---listen-address 192.168.123.1 \ ---listen-address 2001:db8:ac10:fe01::1 \ ---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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=// +srv-host=name.tcp.test-domain-name,.,1024,10,10 +listen-address=192.168.122.1 +listen-address=192.168.123.1 +listen-address=2001:db8:ac10:fe01::1 +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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv index 3481507..bf00513 100644 --- a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv +++ b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv @@ -1,11 +1,18 @@ -@DNSMASQ@ --strict-order --bind-interfaces \ ---local=// --domain-needed --conf-file= \ ---except-interface lo '--txt-record=example,example value' \ ---listen-address 192.168.122.1 --listen-address 192.168.123.1 \ ---listen-address 2001:db8:ac10:fe01::1 \ ---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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=// +txt-record=example,example value +listen-address=192.168.122.1 +listen-address=192.168.123.1 +listen-address=2001:db8:ac10:fe01::1 +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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2argvdata/nat-network.argv b/tests/networkxml2argvdata/nat-network.argv index 37fd2fc..d542bbc 100644 --- a/tests/networkxml2argvdata/nat-network.argv +++ b/tests/networkxml2argvdata/nat-network.argv @@ -1,10 +1,17 @@ -@DNSMASQ@ --strict-order --bind-interfaces \ ---local=// --domain-needed --conf-file= \ ---except-interface lo --listen-address 192.168.122.1 \ ---listen-address 192.168.123.1 --listen-address 2001:db8:ac10:fe01::1 \ ---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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=// +listen-address=192.168.122.1 +listen-address=192.168.123.1 +listen-address=2001:db8:ac10:fe01::1 +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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2argvdata/netboot-network.argv b/tests/networkxml2argvdata/netboot-network.argv index 5408eb7..4f5fedd 100644 --- a/tests/networkxml2argvdata/netboot-network.argv +++ b/tests/networkxml2argvdata/netboot-network.argv @@ -1,10 +1,18 @@ -@DNSMASQ@ --strict-order --bind-interfaces --domain=example.com \ ---local=/example.com/ --domain-needed --conf-file= \ ---except-interface lo --listen-address 192.168.122.1 \ ---dhcp-range 192.168.122.2,192.168.122.254 \ ---dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases \ ---dhcp-lease-max=253 --dhcp-no-override --expand-hosts \ ---dhcp-hostsfile=/var/lib/libvirt/dnsmasq/netboot.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/netboot.addnhosts \ ---enable-tftp \ ---tftp-root /var/lib/tftproot --dhcp-boot pxeboot.img\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=/example.com/ +domain=example.com +expand-hosts +listen-address=192.168.122.1 +dhcp-range=192.168.122.2,192.168.122.254 +dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases +dhcp-lease-max=253 +dhcp-no-override +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/netboot.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/netboot.addnhosts +enable-tftp +tftp-root=/var/lib/tftproot +dhcp-boot=pxeboot.img diff --git a/tests/networkxml2argvdata/netboot-proxy-network.argv b/tests/networkxml2argvdata/netboot-proxy-network.argv index 21e01e3..8b9c03a 100644 --- a/tests/networkxml2argvdata/netboot-proxy-network.argv +++ b/tests/networkxml2argvdata/netboot-proxy-network.argv @@ -1,9 +1,16 @@ -@DNSMASQ@ --strict-order --bind-interfaces --domain=example.com \ ---local=/example.com/ --domain-needed --conf-file= \ ---except-interface lo --listen-address 192.168.122.1 \ ---dhcp-range 192.168.122.2,192.168.122.254 \ ---dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases \ ---dhcp-lease-max=253 --dhcp-no-override --expand-hosts \ ---dhcp-hostsfile=/var/lib/libvirt/dnsmasq/netboot.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/netboot.addnhosts \ ---dhcp-boot pxeboot.img,,10.20.30.40\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=/example.com/ +domain=example.com +expand-hosts +listen-address=192.168.122.1 +dhcp-range=192.168.122.2,192.168.122.254 +dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases +dhcp-lease-max=253 +dhcp-no-override +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/netboot.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/netboot.addnhosts +dhcp-boot=pxeboot.img,,10.20.30.40 diff --git a/tests/networkxml2argvdata/routed-network.argv b/tests/networkxml2argvdata/routed-network.argv index 9fedb2b..ad9e121 100644 --- a/tests/networkxml2argvdata/routed-network.argv +++ b/tests/networkxml2argvdata/routed-network.argv @@ -1,4 +1,8 @@ -@DNSMASQ@ --strict-order --bind-interfaces \ ---local=// --domain-needed --conf-file= \ ---except-interface lo --listen-address 192.168.122.1 \ ---addn-hosts=/var/lib/libvirt/dnsmasq/local.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=// +listen-address=192.168.122.1 +addn-hosts=/var/lib/libvirt/dnsmasq/local.addnhosts diff --git a/tests/networkxml2argvtest.c b/tests/networkxml2argvtest.c index 87519e4..78ac8cf 100644 --- a/tests/networkxml2argvtest.c +++ b/tests/networkxml2argvtest.c @@ -15,37 +15,6 @@ #include "memory.h" #include "network/bridge_driver.h" -/* Replace all occurrences of @token in @buf by @replacement and adjust size of - * @buf accordingly. Returns 0 on success and -1 on out-of-memory errors. */ -static int replaceTokens(char **buf, const char *token, const char *replacement) { - size_t token_start, token_end; - size_t buf_len, rest_len; - const size_t token_len = strlen(token); - const size_t replacement_len = strlen(replacement); - const int diff = replacement_len - token_len; - - buf_len = rest_len = strlen(*buf) + 1; - token_end = 0; - for (;;) { - char *match = strstr(*buf + token_end, token); - if (match == NULL) - break; - token_start = match - *buf; - rest_len -= token_start + token_len - token_end; - token_end = token_start + token_len; - buf_len += diff; - if (diff > 0) - if (VIR_REALLOC_N(*buf, buf_len) < 0) - return -1; - if (diff != 0) - memmove(*buf + token_end + diff, *buf + token_end, rest_len); - memcpy(*buf + token_start, replacement, replacement_len); - token_end += diff; - } - /* if diff < 0, we could shrink the buffer here... */ - return 0; -} - static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) { char *inXmlData = NULL; char *outArgvData = NULL; @@ -55,6 +24,7 @@ static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) { virNetworkObjPtr obj = NULL; virCommandPtr cmd = NULL; char *pidfile = NULL; + char *configdir = NULL; dnsmasqContext *dctx = NULL; if (virtTestLoadFile(inxml, &inXmlData) < 0) @@ -62,10 +32,6 @@ static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) { if (virtTestLoadFile(outargv, &outArgvData) < 0) goto fail; - - if (replaceTokens(&outArgvData, "@DNSMASQ@", DNSMASQ)) - goto fail; - if (!(dev = virNetworkDefParseString(inXmlData))) goto fail; @@ -78,12 +44,9 @@ static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) { if (dctx == NULL) goto fail; - if (networkBuildDhcpDaemonCommandLine(obj, &cmd, pidfile, dctx) < 0) - goto fail; - - if (!(actual = virCommandToString(cmd))) + if (networkBuildDhcpDaemonCommandLine(obj, &cmd, pidfile, + dctx, configdir, 1, &actual) < 0) goto fail; - if (STRNEQ(outArgvData, actual)) { virtTestDifference(stderr, outArgvData, actual); goto fail; @@ -147,7 +110,6 @@ mymain(void) if (virtTestRun("Network XML-2-Argv " name, \ 1, testCompareXMLToArgvHelper, (name)) < 0) \ ret = -1 - DO_TEST("isolated-network"); DO_TEST("routed-network"); DO_TEST("nat-network"); -- 1.7.11.7
On 10/23/2012 11:07 AM, Gene Czarcinski wrote:
This patch changes the way parameters are passed to dnsmasq. They are put into a conf-file instead of being on the dnsmasq command line.
I was thinking about this last night after I learned from you that the conf file *isn't* reread when dnsmasq received SIGHUP. That being the case, what are the other reasons for switching from commandline to conf file? I suppose one is that it eliminates clutter in the ps output (and gets it away from prying eyes (since anyone can see the commandline by looking in the output of ps -AlF). Another would be it avoids hitting the commandline limit (which is rather large, but still exists) when there are a lot of srv and txt entries. Anything else?
**NOTE ** This has updated the related tests for the new data format, etc.
**NOTE** This patch does NOT include specifying interface=
The command line now contains --conf-file=<filename> and a new parameter --conf-dir=<directoryname> has been added.
The new file and directory are put in the same directory as the leases file. --- src/network/bridge_driver.c | 179 ++++++++++++++------- src/network/bridge_driver.h | 8 +- tests/networkxml2argvdata/isolated-network.argv | 24 +-- .../networkxml2argvdata/nat-network-dns-hosts.argv | 14 +- .../nat-network-dns-srv-record-minimal.argv | 35 ++-- .../nat-network-dns-srv-record.argv | 35 ++-- .../nat-network-dns-txt-record.argv | 29 ++-- tests/networkxml2argvdata/nat-network.argv | 27 ++-- tests/networkxml2argvdata/netboot-network.argv | 28 ++-- .../networkxml2argvdata/netboot-proxy-network.argv | 25 +-- tests/networkxml2argvdata/routed-network.argv | 12 +- tests/networkxml2argvtest.c | 44 +---- 12 files changed, 264 insertions(+), 196 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8837843..ab13df5 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -136,6 +136,16 @@ networkDnsmasqLeaseFileNameFunc networkDnsmasqLeaseFileName = networkDnsmasqLeaseFileNameDefault;
static char * +networkDnsmasqConfigFileName(const char *netname) +{ + char *conffile; + + ignore_value(virAsprintf(&conffile, DNSMASQ_STATE_DIR "/%s.conf", + netname)); + return conffile; +} + +static char * networkRadvdPidfileBasename(const char *netname) { /* this is simple but we want to be sure it's consistently done */ @@ -559,23 +569,25 @@ networkBuildDnsmasqHostsfile(dnsmasqContext *dctx, return 0; }
- + /* build the dnsmasq conf file contents */ static int -networkBuildDnsmasqArgv(virNetworkObjPtr network, +networkDnsmasqConfContents(virNetworkObjPtr network, virNetworkIpDefPtr ipdef, const char *pidfile, - virCommandPtr cmd, + char **configstr, dnsmasqContext *dctx) { - int r, ret = -1; + virBuffer configbuf = VIR_BUFFER_INITIALIZER;; + int r, ret = -1, ii; int nbleases = 0; - int ii; char *record = NULL; char *recordPort = NULL; char *recordWeight = NULL; char *recordPriority = NULL; virNetworkIpDefPtr tmpipdef;
+ *configstr = NULL; + /* * NB, be careful about syntax for dnsmasq options in long format. * @@ -595,28 +607,22 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, * very explicit on this. */
- /* - * Needed to ensure dnsmasq uses same algorithm for processing - * multiple namedriver entries in /etc/resolv.conf as GLibC. - */ - virCommandAddArgList(cmd, "--strict-order", "--bind-interfaces", NULL); - + /* create dnsmasq config file appropriate for this network */ + virBufferAsprintf(&configbuf, "# dnsmasq conf file created by libvirt\n" + "strict-order\n" + "bind-interfaces\n" + "except-interface=lo\n" + "domain-needed\n" + "local=/%s/\n", + network->def->domain ? network->def->domain : ""); if (network->def->domain) - virCommandAddArgPair(cmd, "--domain", network->def->domain); - /* need to specify local even if no domain specified */ - virCommandAddArgFormat(cmd, "--local=/%s/", - network->def->domain ? network->def->domain : ""); - virCommandAddArg(cmd, "--domain-needed"); + virBufferAsprintf(&configbuf, + "domain=%s\n" + "expand-hosts\n", + network->def->domain);
if (pidfile) - virCommandAddArgPair(cmd, "--pid-file", pidfile); - - /* *no* conf file */ - virCommandAddArg(cmd, "--conf-file="); - - virCommandAddArgList(cmd, - "--except-interface", "lo", - NULL); + virBufferAsprintf(&configbuf, "pid-file=%s\n", pidfile);
/* If this is an isolated network, set the default route option * (3) to be empty to avoid setting a default route that's @@ -626,16 +632,21 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, * to build a connection to the outside). */ if (network->def->forwardType == VIR_NETWORK_FORWARD_NONE) { - virCommandAddArgList(cmd, "--dhcp-option=3", - "--no-resolv", NULL); + virBufferAsprintf(&configbuf, "dhcp-option=3\n" + "no-resolv\n"); }
+ /* + * Needed to ensure dnsmasq uses same algorithm for processing + * multiple namedriver entries in /etc/resolv.conf as GLibC. + */ + if (network->def->dns != NULL) { virNetworkDNSDefPtr dns = network->def->dns; int i;
for (i = 0; i < dns->ntxtrecords; i++) { - virCommandAddArgFormat(cmd, "--txt-record=%s,%s", + virBufferAsprintf(&configbuf, "txt-record=%s,%s\n", dns->txtrecords[i].name, dns->txtrecords[i].value); } @@ -673,7 +684,7 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, goto cleanup; }
- virCommandAddArgPair(cmd, "--srv-host", record); + virBufferAsprintf(&configbuf, "srv-host=%s\n", record); VIR_FREE(record); VIR_FREE(recordPort); VIR_FREE(recordWeight); @@ -682,21 +693,14 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, } }
- /* - * --interface does not actually work with dnsmasq < 2.47, - * due to DAD for ipv6 addresses on the interface. - * - * virCommandAddArgList(cmd, "--interface", ipdef->bridge, NULL); - * - * So listen on all defined IPv[46] addresses - */ for (ii = 0; (tmpipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii)); - ii++) { + ii++) + { char *ipaddr = virSocketAddrFormat(&tmpipdef->address); if (!ipaddr) goto cleanup; - virCommandAddArgList(cmd, "--listen-address", ipaddr, NULL); + virBufferAsprintf(&configbuf, "listen-address=%s\n", ipaddr); VIR_FREE(ipaddr); }
@@ -710,8 +714,8 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, VIR_FREE(saddr); goto cleanup; } - virCommandAddArg(cmd, "--dhcp-range"); - virCommandAddArgFormat(cmd, "%s,%s", saddr, eaddr); + virBufferAsprintf(&configbuf, "dhcp-range=%s,%s\n", + saddr, eaddr); VIR_FREE(saddr); VIR_FREE(eaddr); nbleases += virSocketAddrGetRange(&ipdef->ranges[r].start, @@ -727,8 +731,7 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, char *bridgeaddr = virSocketAddrFormat(&ipdef->address); if (!bridgeaddr) goto cleanup; - virCommandAddArg(cmd, "--dhcp-range"); - virCommandAddArgFormat(cmd, "%s,static", bridgeaddr); + virBufferAsprintf(&configbuf, "dhcp-range=%s,static\n", bridgeaddr); VIR_FREE(bridgeaddr); }
@@ -736,17 +739,13 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, char *leasefile = networkDnsmasqLeaseFileName(network->def->name); if (!leasefile) goto cleanup; - virCommandAddArgFormat(cmd, "--dhcp-leasefile=%s", leasefile); + virBufferAsprintf(&configbuf, "dhcp-leasefile=%s\n", leasefile); VIR_FREE(leasefile); - virCommandAddArgFormat(cmd, "--dhcp-lease-max=%d", nbleases); + virBufferAsprintf(&configbuf, "dhcp-lease-max=%d\n", nbleases); }
if (ipdef->nranges || ipdef->nhosts) - virCommandAddArg(cmd, "--dhcp-no-override"); - - /* add domain to any non-qualified hostnames in /etc/hosts or addn-hosts */ - if (network->def->domain) - virCommandAddArg(cmd, "--expand-hosts"); + virBufferAsprintf(&configbuf, "dhcp-no-override\n");
if (networkBuildDnsmasqHostsfile(dctx, ipdef, network->def->dns) < 0) goto cleanup; @@ -756,38 +755,42 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, * file to allow for runtime additions. */ if (ipdef->nranges || ipdef->nhosts) - virCommandAddArgPair(cmd, "--dhcp-hostsfile", + virBufferAsprintf(&configbuf, "dhcp-hostsfile=%s\n", dctx->hostsfile->path);
/* Likewise, always create this file and put it on the commandline, to allow for * for runtime additions. */ - virCommandAddArgPair(cmd, "--addn-hosts", + virBufferAsprintf(&configbuf, "addn-hosts=%s\n", dctx->addnhostsfile->path);
if (ipdef->tftproot) { - virCommandAddArgList(cmd, "--enable-tftp", - "--tftp-root", ipdef->tftproot, - NULL); + virBufferAsprintf(&configbuf, "enable-tftp\n"); + virBufferAsprintf(&configbuf, "tftp-root=%s\n", ipdef->tftproot); } if (ipdef->bootfile) { - virCommandAddArg(cmd, "--dhcp-boot"); if (VIR_SOCKET_ADDR_VALID(&ipdef->bootserver)) { char *bootserver = virSocketAddrFormat(&ipdef->bootserver);
if (!bootserver) goto cleanup; - virCommandAddArgFormat(cmd, "%s%s%s", + virBufferAsprintf(&configbuf, "dhcp-boot=%s%s%s\n", ipdef->bootfile, ",,", bootserver); VIR_FREE(bootserver); } else { - virCommandAddArg(cmd, ipdef->bootfile); + virBufferAsprintf(&configbuf, "dhcp-boot=%s\n", ipdef->bootfile); } } } + if (!(*configstr = virBufferContentAndReset(&configbuf))) { + virReportOOMError(); + goto cleanup; + }
ret = 0; + cleanup: + virBufferFreeAndReset(&configbuf); VIR_FREE(record); VIR_FREE(recordPort); VIR_FREE(recordWeight); @@ -795,13 +798,18 @@ cleanup: return ret; }
+ /* build the dnsmasq command line */ int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, - char *pidfile, dnsmasqContext *dctx) + char *pidfile, dnsmasqContext *dctx, + char *configdir, + int testOnly, char **testConfigStr) { virCommandPtr cmd = NULL; int ret = -1, ii; virNetworkIpDefPtr ipdef; + char *configfile = NULL; + char *configstr = NULL;
network->dnsmasqPid = -1;
@@ -825,15 +833,41 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdou if (!virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, 0)) return 0;
- cmd = virCommandNew(DNSMASQ); - if (networkBuildDnsmasqArgv(network, ipdef, pidfile, cmd, dctx) < 0) { + if (networkDnsmasqConfContents(network, ipdef, pidfile, &configstr, dctx) < 0) + goto cleanup; + if (!configstr) + goto cleanup; + + if (testOnly) { + *testConfigStr = configstr; + return 0; + } + + /* construct the filename */ + if (!(configfile = networkDnsmasqConfigFileName(network->def->name))) { + virReportOOMError(); + goto cleanup; + } + + /* Write the file */ + if (virFileWriteStr(configfile, configstr, 0600) < 0) { + virReportSystemError(errno, + _("couldn't write dnsmasq config file '%s'"), + configfile); goto cleanup; } + VIR_INFO("dnsmasq conf file %s written", configfile); + + cmd = virCommandNew(DNSMASQ); + virCommandAddArgFormat(cmd, "--conf-file=%s", configfile); + virCommandAddArgFormat(cmd, "--conf-dir=%s", configdir);
if (cmdout) *cmdout = cmd; ret = 0; cleanup: + VIR_FREE(configstr); + VIR_FREE(configfile); if (ret < 0) virCommandFree(cmd); return ret; @@ -844,9 +878,12 @@ networkStartDhcpDaemon(virNetworkObjPtr network) { virCommandPtr cmd = NULL; char *pidfile = NULL; + char *testconfigstr = NULL; + char *configdir = NULL; int ret = -1; dnsmasqContext *dctx = NULL;
+ VIR_INFO("starting dhcp daemon (dnsmasq)"); if (!virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, 0)) { /* no IPv6 addresses, so we don't need to run radvd */ ret = 0; @@ -882,7 +919,18 @@ networkStartDhcpDaemon(virNetworkObjPtr network) if (dctx == NULL) goto cleanup;
- ret = networkBuildDhcpDaemonCommandLine(network, &cmd, pidfile, dctx); + ignore_value(virAsprintf(&configdir, DNSMASQ_STATE_DIR "/%s.d", + network->def->name)); + if (virFileMakePath(configdir) < 0) { + virReportSystemError(errno, + _("cannot create directory %s"), + configdir); + goto cleanup; + } + + + ret = networkBuildDhcpDaemonCommandLine(network, &cmd, pidfile, dctx, + configdir, 0, &testconfigstr); if (ret < 0) goto cleanup;
@@ -911,6 +959,7 @@ networkStartDhcpDaemon(virNetworkObjPtr network) ret = 0; cleanup: VIR_FREE(pidfile); + VIR_FREE(configdir); virCommandFree(cmd); dnsmasqContextFree(dctx); return ret; @@ -2841,6 +2890,14 @@ static int networkUndefine(virNetworkPtr net) { } }
+ { + char *configfile = networkDnsmasqConfigFileName(network->def->name); + if (!configfile) + goto cleanup; + unlink(configfile); + VIR_FREE(configfile); + } + if (dhcp_present) { char *leasefile; dnsmasqContext *dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR); diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index 0fae275..00675c4 100644 --- a/src/network/bridge_driver.h +++ b/src/network/bridge_driver.h @@ -48,15 +48,17 @@ int networkGetNetworkAddress(const char *netname, char **netaddr)
int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, char *pidfile, - dnsmasqContext *dctx) - ; + dnsmasqContext *dctx, + char *configdir, + int testOnly, char **testConfigStr); # else /* Define no-op replacements that don't drag in any link dependencies. */ # define networkAllocateActualDevice(iface) 0 # define networkNotifyActualDevice(iface) 0 # define networkReleaseActualDevice(iface) 0 # define networkGetNetworkAddress(netname, netaddr) (-2) -# define networkBuildDhcpDaemonCommandLine(network, cmdout, pidfile, dctx) 0 +# define networkBuildDhcpDaemonCommandLine(network, cmdout, pidfile, \ + dctx, configdir, testOnly, testConfigStr) 0 # endif
typedef char *(*networkDnsmasqLeaseFileNameFunc)(const char *netname); diff --git a/tests/networkxml2argvdata/isolated-network.argv b/tests/networkxml2argvdata/isolated-network.argv index 13e77b2..042158b 100644 --- a/tests/networkxml2argvdata/isolated-network.argv +++ b/tests/networkxml2argvdata/isolated-network.argv @@ -1,9 +1,15 @@ -@DNSMASQ@ --strict-order --bind-interfaces \ ---local=// --domain-needed --conf-file= \ ---except-interface lo --dhcp-option=3 --no-resolv \ ---listen-address 192.168.152.1 \ ---dhcp-range 192.168.152.2,192.168.152.254 \ ---dhcp-leasefile=/var/lib/libvirt/dnsmasq/private.leases --dhcp-lease-max=253 \ ---dhcp-no-override \ ---dhcp-hostsfile=/var/lib/libvirt/dnsmasq/private.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/private.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=// +dhcp-option=3 +no-resolv +listen-address=192.168.152.1 +dhcp-range=192.168.152.2,192.168.152.254 +dhcp-leasefile=/var/lib/libvirt/dnsmasq/private.leases +dhcp-lease-max=253 +dhcp-no-override +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/private.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/private.addnhosts diff --git a/tests/networkxml2argvdata/nat-network-dns-hosts.argv b/tests/networkxml2argvdata/nat-network-dns-hosts.argv index 03a0676..91eb682 100644 --- a/tests/networkxml2argvdata/nat-network-dns-hosts.argv +++ b/tests/networkxml2argvdata/nat-network-dns-hosts.argv @@ -1,4 +1,10 @@ -@DNSMASQ@ --strict-order --bind-interfaces --domain=example.com \ ---local=/example.com/ --domain-needed \ ---conf-file= --except-interface lo --listen-address 192.168.122.1 \ ---expand-hosts --addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=/example.com/ +domain=example.com +expand-hosts +listen-address=192.168.122.1 +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv b/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv index 210a60c..d92497b 100644 --- a/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv +++ b/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv @@ -1,17 +1,18 @@ -@DNSMASQ@ \ ---strict-order \ ---bind-interfaces \ ---local=// --domain-needed --conf-file= \ ---except-interface lo \ ---srv-host=name.tcp.,,,, \ ---listen-address 192.168.122.1 \ ---listen-address 192.168.123.1 \ ---listen-address 2001:db8:ac10:fe01::1 \ ---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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=// +srv-host=name.tcp.,,,, +listen-address=192.168.122.1 +listen-address=192.168.123.1 +listen-address=2001:db8:ac10:fe01::1 +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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2argvdata/nat-network-dns-srv-record.argv b/tests/networkxml2argvdata/nat-network-dns-srv-record.argv index 833d3cd..d8846c2 100644 --- a/tests/networkxml2argvdata/nat-network-dns-srv-record.argv +++ b/tests/networkxml2argvdata/nat-network-dns-srv-record.argv @@ -1,17 +1,18 @@ -@DNSMASQ@ \ ---strict-order \ ---bind-interfaces \ ---local=// --domain-needed --conf-file= \ ---except-interface lo \ ---srv-host=name.tcp.test-domain-name,.,1024,10,10 \ ---listen-address 192.168.122.1 \ ---listen-address 192.168.123.1 \ ---listen-address 2001:db8:ac10:fe01::1 \ ---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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=// +srv-host=name.tcp.test-domain-name,.,1024,10,10 +listen-address=192.168.122.1 +listen-address=192.168.123.1 +listen-address=2001:db8:ac10:fe01::1 +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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv index 3481507..bf00513 100644 --- a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv +++ b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv @@ -1,11 +1,18 @@ -@DNSMASQ@ --strict-order --bind-interfaces \ ---local=// --domain-needed --conf-file= \ ---except-interface lo '--txt-record=example,example value' \ ---listen-address 192.168.122.1 --listen-address 192.168.123.1 \ ---listen-address 2001:db8:ac10:fe01::1 \ ---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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=// +txt-record=example,example value +listen-address=192.168.122.1 +listen-address=192.168.123.1 +listen-address=2001:db8:ac10:fe01::1 +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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2argvdata/nat-network.argv b/tests/networkxml2argvdata/nat-network.argv index 37fd2fc..d542bbc 100644 --- a/tests/networkxml2argvdata/nat-network.argv +++ b/tests/networkxml2argvdata/nat-network.argv @@ -1,10 +1,17 @@ -@DNSMASQ@ --strict-order --bind-interfaces \ ---local=// --domain-needed --conf-file= \ ---except-interface lo --listen-address 192.168.122.1 \ ---listen-address 192.168.123.1 --listen-address 2001:db8:ac10:fe01::1 \ ---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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=// +listen-address=192.168.122.1 +listen-address=192.168.123.1 +listen-address=2001:db8:ac10:fe01::1 +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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2argvdata/netboot-network.argv b/tests/networkxml2argvdata/netboot-network.argv index 5408eb7..4f5fedd 100644 --- a/tests/networkxml2argvdata/netboot-network.argv +++ b/tests/networkxml2argvdata/netboot-network.argv @@ -1,10 +1,18 @@ -@DNSMASQ@ --strict-order --bind-interfaces --domain=example.com \ ---local=/example.com/ --domain-needed --conf-file= \ ---except-interface lo --listen-address 192.168.122.1 \ ---dhcp-range 192.168.122.2,192.168.122.254 \ ---dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases \ ---dhcp-lease-max=253 --dhcp-no-override --expand-hosts \ ---dhcp-hostsfile=/var/lib/libvirt/dnsmasq/netboot.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/netboot.addnhosts \ ---enable-tftp \ ---tftp-root /var/lib/tftproot --dhcp-boot pxeboot.img\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=/example.com/ +domain=example.com +expand-hosts +listen-address=192.168.122.1 +dhcp-range=192.168.122.2,192.168.122.254 +dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases +dhcp-lease-max=253 +dhcp-no-override +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/netboot.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/netboot.addnhosts +enable-tftp +tftp-root=/var/lib/tftproot +dhcp-boot=pxeboot.img diff --git a/tests/networkxml2argvdata/netboot-proxy-network.argv b/tests/networkxml2argvdata/netboot-proxy-network.argv index 21e01e3..8b9c03a 100644 --- a/tests/networkxml2argvdata/netboot-proxy-network.argv +++ b/tests/networkxml2argvdata/netboot-proxy-network.argv @@ -1,9 +1,16 @@ -@DNSMASQ@ --strict-order --bind-interfaces --domain=example.com \ ---local=/example.com/ --domain-needed --conf-file= \ ---except-interface lo --listen-address 192.168.122.1 \ ---dhcp-range 192.168.122.2,192.168.122.254 \ ---dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases \ ---dhcp-lease-max=253 --dhcp-no-override --expand-hosts \ ---dhcp-hostsfile=/var/lib/libvirt/dnsmasq/netboot.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/netboot.addnhosts \ ---dhcp-boot pxeboot.img,,10.20.30.40\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=/example.com/ +domain=example.com +expand-hosts +listen-address=192.168.122.1 +dhcp-range=192.168.122.2,192.168.122.254 +dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases +dhcp-lease-max=253 +dhcp-no-override +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/netboot.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/netboot.addnhosts +dhcp-boot=pxeboot.img,,10.20.30.40 diff --git a/tests/networkxml2argvdata/routed-network.argv b/tests/networkxml2argvdata/routed-network.argv index 9fedb2b..ad9e121 100644 --- a/tests/networkxml2argvdata/routed-network.argv +++ b/tests/networkxml2argvdata/routed-network.argv @@ -1,4 +1,8 @@ -@DNSMASQ@ --strict-order --bind-interfaces \ ---local=// --domain-needed --conf-file= \ ---except-interface lo --listen-address 192.168.122.1 \ ---addn-hosts=/var/lib/libvirt/dnsmasq/local.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=// +listen-address=192.168.122.1 +addn-hosts=/var/lib/libvirt/dnsmasq/local.addnhosts diff --git a/tests/networkxml2argvtest.c b/tests/networkxml2argvtest.c index 87519e4..78ac8cf 100644 --- a/tests/networkxml2argvtest.c +++ b/tests/networkxml2argvtest.c @@ -15,37 +15,6 @@ #include "memory.h" #include "network/bridge_driver.h"
-/* Replace all occurrences of @token in @buf by @replacement and adjust size of - * @buf accordingly. Returns 0 on success and -1 on out-of-memory errors. */ -static int replaceTokens(char **buf, const char *token, const char *replacement) { - size_t token_start, token_end; - size_t buf_len, rest_len; - const size_t token_len = strlen(token); - const size_t replacement_len = strlen(replacement); - const int diff = replacement_len - token_len; - - buf_len = rest_len = strlen(*buf) + 1; - token_end = 0; - for (;;) { - char *match = strstr(*buf + token_end, token); - if (match == NULL) - break; - token_start = match - *buf; - rest_len -= token_start + token_len - token_end; - token_end = token_start + token_len; - buf_len += diff; - if (diff > 0) - if (VIR_REALLOC_N(*buf, buf_len) < 0) - return -1; - if (diff != 0) - memmove(*buf + token_end + diff, *buf + token_end, rest_len); - memcpy(*buf + token_start, replacement, replacement_len); - token_end += diff; - } - /* if diff < 0, we could shrink the buffer here... */ - return 0; -} - static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) { char *inXmlData = NULL; char *outArgvData = NULL; @@ -55,6 +24,7 @@ static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) { virNetworkObjPtr obj = NULL; virCommandPtr cmd = NULL; char *pidfile = NULL; + char *configdir = NULL; dnsmasqContext *dctx = NULL;
if (virtTestLoadFile(inxml, &inXmlData) < 0) @@ -62,10 +32,6 @@ static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) {
if (virtTestLoadFile(outargv, &outArgvData) < 0) goto fail; - - if (replaceTokens(&outArgvData, "@DNSMASQ@", DNSMASQ)) - goto fail; - if (!(dev = virNetworkDefParseString(inXmlData))) goto fail;
@@ -78,12 +44,9 @@ static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) { if (dctx == NULL) goto fail;
- if (networkBuildDhcpDaemonCommandLine(obj, &cmd, pidfile, dctx) < 0) - goto fail; - - if (!(actual = virCommandToString(cmd))) + if (networkBuildDhcpDaemonCommandLine(obj, &cmd, pidfile, + dctx, configdir, 1, &actual) < 0) goto fail; - if (STRNEQ(outArgvData, actual)) { virtTestDifference(stderr, outArgvData, actual); goto fail; @@ -147,7 +110,6 @@ mymain(void) if (virtTestRun("Network XML-2-Argv " name, \ 1, testCompareXMLToArgvHelper, (name)) < 0) \ ret = -1 - DO_TEST("isolated-network"); DO_TEST("routed-network"); DO_TEST("nat-network");
On 10/23/2012 12:01 PM, Laine Stump wrote:
This patch changes the way parameters are passed to dnsmasq. They are put into a conf-file instead of being on the dnsmasq command line. I was thinking about this last night after I learned from you that the conf file *isn't* reread when dnsmasq received SIGHUP. That being the case, what are the other reasons for switching from commandline to conf file? I suppose one is that it eliminates clutter in the ps output (and gets it away from prying eyes (since anyone can see the commandline by looking in the output of ps -AlF). Another would be it avoids hitting
On 10/23/2012 11:07 AM, Gene Czarcinski wrote: the commandline limit (which is rather large, but still exists) when there are a lot of srv and txt entries. Anything else?
Not really. Maybe there is also the fact that I believe this is a "cleaner," but not necessarily better, approach. There is also the possibility that Simon could be convinced to add the capability of re-reading configuration files. I will ask to see what his reaction is. But, his reaction may be: "Sure, go ahead and do that; I look forward to your patches." I am not sure I want to wander in that swamp right now. The most important thing to me is adding the --conf-dir=<directory>. This will allow debugging different parameters without having to recompile/rebuild the entire libvirt set of packages. Now, when I first did this, I thought that the configuration files in the directory would be re-read. When it did not do that, and, upon doing a little research, found that it was working as designed. Oh, BTW, there is one other little thing about the command line parameters. To support IPv6 completely, the should be local=/<reverse-ip6-addr>.ip6.arpa/ parameters and these are very long. Putting things into a conf-file just makes it (IMHO) a lot simpler to understand. I do realize that this is open source and I can always "do my own thing" and continue using the patches myself. However, that can mean a lot of work just keeping up. One last observation/question: Why not? Gene
**NOTE ** This has updated the related tests for the new data format, etc.
**NOTE** This patch does NOT include specifying interface=
The command line now contains --conf-file=<filename> and a new parameter --conf-dir=<directoryname> has been added.
The new file and directory are put in the same directory as the leases file. --- src/network/bridge_driver.c | 179 ++++++++++++++------- src/network/bridge_driver.h | 8 +- tests/networkxml2argvdata/isolated-network.argv | 24 +-- .../networkxml2argvdata/nat-network-dns-hosts.argv | 14 +- .../nat-network-dns-srv-record-minimal.argv | 35 ++-- .../nat-network-dns-srv-record.argv | 35 ++-- .../nat-network-dns-txt-record.argv | 29 ++-- tests/networkxml2argvdata/nat-network.argv | 27 ++-- tests/networkxml2argvdata/netboot-network.argv | 28 ++-- .../networkxml2argvdata/netboot-proxy-network.argv | 25 +-- tests/networkxml2argvdata/routed-network.argv | 12 +- tests/networkxml2argvtest.c | 44 +---- 12 files changed, 264 insertions(+), 196 deletions(-)
<snip>
On 10/23/2012 12:57 PM, Gene Czarcinski wrote:
On 10/23/2012 12:01 PM, Laine Stump wrote:
This patch changes the way parameters are passed to dnsmasq. They are put into a conf-file instead of being on the dnsmasq command line. I was thinking about this last night after I learned from you that the conf file *isn't* reread when dnsmasq received SIGHUP. That being the case, what are the other reasons for switching from commandline to conf file? I suppose one is that it eliminates clutter in the ps output (and gets it away from prying eyes (since anyone can see the commandline by looking in the output of ps -AlF). Another would be it avoids hitting
On 10/23/2012 11:07 AM, Gene Czarcinski wrote: the commandline limit (which is rather large, but still exists) when there are a lot of srv and txt entries. Anything else?
Not really. Maybe there is also the fact that I believe this is a "cleaner," but not necessarily better, approach.
There is also the possibility that Simon could be convinced to add the capability of re-reading configuration files. I will ask to see what his reaction is. But, his reaction may be: "Sure, go ahead and do that; I look forward to your patches." I am not sure I want to wander in that swamp right now.
The most important thing to me is adding the --conf-dir=<directory>. This will allow debugging different parameters without having to recompile/rebuild the entire libvirt set of packages. Now, when I first did this, I thought that the configuration files in the directory would be re-read. When it did not do that, and, upon doing a little research, found that it was working as designed.
Oh, BTW, there is one other little thing about the command line parameters. To support IPv6 completely, the should be local=/<reverse-ip6-addr>.ip6.arpa/ parameters and these are very long. Putting things into a conf-file just makes it (IMHO) a lot simpler to understand.
I do realize that this is open source and I can always "do my own thing" and continue using the patches myself. However, that can mean a lot of work just keeping up.
Sure. I think it's a net gain. It's just that my main reason for wanting to do this switch was to avoid restarting dnsmasq for every little change (mainly because that's one of the things I've been working on lately - virNetworkUpdate()), and when I saw that carrot go away, I needed a sanity check to rationalize the change.
One last observation/question: Why not?
I often find myself falling into the hole of making change for change's sake, which leads to cleaner code, but makes backporting fixes to earlier releases more difficult. I need to constantly watch myself for too much of this behavior (some amount is good, as it does lean to cleaner code, but there is such a thing as too much of anything). I tried your earlier --interface patch on F17 last night, and built it on F16 and RHEL6 but didn't get a chance to test it. So instead I'm going to try a build with both of these patches on the three platforms today and (hopefully) push them both prior to the 1.0.0 freeze late tonight. That way they'll have a reasonable amount of testing, and will get even more (and especially on other distros) during the freeze, with time enough to tweak (or even back out if necessary) before 1.0.0.
On 10/23/2012 01:13 PM, Laine Stump wrote:
I tried your earlier --interface patch on F17 last night, and built it on F16 and RHEL6 but didn't get a chance to test it. So instead I'm going to try a build with both of these patches on the three platforms today and (hopefully) push them both prior to the 1.0.0 freeze late tonight. That way they'll have a reasonable amount of testing, and will get even more (and especially on other distros) during the freeze, with time enough to tweak (or even back out if necessary) before 1.0.0. OK, there are "branches" at v0.10.2-maint, v0.9.11-maint, and v0.9.6-maint. Would it be useful to attecmp backporting any of this stuff to those "levels"?
Here is what I sent to the dnsmasq discussion list:
It would be useful to libvirt if dnsmasq would reread the configuration file and/or the files in the configuration directory upon demand (via SIG<something>) as is done for some other files.
Right now, when minor changes are made to a network configuration, it is necessary to restart dnsmasq to get those changes adopted.
Is this a possibility this could be done or is this one of those things that yes, it could be done, but it would require rewriting most of dnsmasq. What types of changes might be accommodated? What if these changeable definitions were put into a special configuration file?
Example: v6 or v6 dhcp-range.
Example of something that would unlikely be possible: interface=
Can you provide a bit more info as to what you would like to change without restarting dnsmasq. Gene
On 10/23/2012 03:24 PM, Gene Czarcinski wrote:
On 10/23/2012 01:13 PM, Laine Stump wrote:
I tried your earlier --interface patch on F17 last night, and built it on F16 and RHEL6 but didn't get a chance to test it. So instead I'm going to try a build with both of these patches on the three platforms today and (hopefully) push them both prior to the 1.0.0 freeze late tonight. That way they'll have a reasonable amount of testing, and will get even more (and especially on other distros) during the freeze, with time enough to tweak (or even back out if necessary) before 1.0.0. OK, there are "branches" at v0.10.2-maint, v0.9.11-maint, and v0.9.6-maint. Would it be useful to attecmp backporting any of this stuff to those "levels"?
I wouldn't worry about that quite yet. Let's wait until it's pushed upstream. At the point, we'll probably want the first two (for F17 and F18, which have dnsmasq-2.63 which according to you causes problems).
Here is what I sent to the dnsmasq discussion list:
It would be useful to libvirt if dnsmasq would reread the configuration file and/or the files in the configuration directory upon demand (via SIG<something>) as is done for some other files.
Right now, when minor changes are made to a network configuration, it is necessary to restart dnsmasq to get those changes adopted.
Is this a possibility this could be done or is this one of those things that yes, it could be done, but it would require rewriting most of dnsmasq. What types of changes might be accommodated? What if these changeable definitions were put into a special configuration file?
Example: v6 or v6 dhcp-range.
Example of something that would unlikely be possible: interface=
Can you provide a bit more info as to what you would like to change without restarting dnsmasq.
Right now, the ranges of IP addresses, the IP addresses to listen on, the domain. There may be other things in the future as virNetworkUpdate() gets fleshed out.
On 10/23/2012 04:10 PM, Laine Stump wrote:
I wouldn't worry about that quite yet. Let's wait until it's pushed upstream. At the point, we'll probably want the first two (for F17 and F18, which have dnsmasq-2.63 which according to you causes problems). Not me, Simon Kelley the dnsmasq developer/maintainer/etc.
Rather than just pasting his comment here, got to look at the message he wrote: http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2012q4/006415.html There might be a way to make it work with just the gateway address (that is what listen-address really is because dnsmasq does not really need an address just the interface), but that is not how it was done. Gene
A big OOPS!!! On 10/23/2012 04:55 PM, Gene Czarcinski wrote:
On 10/23/2012 04:10 PM, Laine Stump wrote:
I wouldn't worry about that quite yet. Let's wait until it's pushed upstream. At the point, we'll probably want the first two (for F17 and F18, which have dnsmasq-2.63 which according to you causes problems). Not me, Simon Kelley the dnsmasq developer/maintainer/etc.
Rather than just pasting his comment here, got to look at the message he wrote:
http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2012q4/006415.html
There might be a way to make it work with just the gateway address (that is what listen-address really is because dnsmasq does not really need an address just the interface), but that is not how it was done. After I sent the message, I just got something in from Simon Kelley which has some new info: http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2012q4/006445.html
The heart of it is this: --------------------------------------------------
OK, so this is vaguely embarrassing. Having checked the actual code, rather than the changelog, I see that dnsmasq >=2.61 _already_ does the right thing. Setting --bind-interfaces* and a single --listen-address will cause the code to set SO_BINDTODEVICE on the DHCP socket(s).
So, there is not a problem with the existing libvirt command line. I disagree. I believe that the problem still exists.
What Simon says implies that everything is OK and nothing needs to be done but consider this: 1. What harm does it do to add the interface=<> specification in addition to everything else? 2. Note that Simon states "Setting --bind-interfaces* and a single --listen-address ". Well, I can define multiple IPv4 and/or IPv6 listen-addresses to be on a single virtual interface. From what Simon says, that means all bets are off. 3. I suspect that many/most instances of dnsmasq only has a single address and that is why the problem has not manifested itself. 4. I do not know if a v4 and v6 address counts as one or two. Gene
On 10/24/2012 07:23 AM, Gene Czarcinski wrote:
A big OOPS!!!
On 10/23/2012 04:55 PM, Gene Czarcinski wrote:
On 10/23/2012 04:10 PM, Laine Stump wrote:
I wouldn't worry about that quite yet. Let's wait until it's pushed upstream. At the point, we'll probably want the first two (for F17 and F18, which have dnsmasq-2.63 which according to you causes problems). Not me, Simon Kelley the dnsmasq developer/maintainer/etc.
Rather than just pasting his comment here, got to look at the message he wrote:
http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2012q4/006415.html
There might be a way to make it work with just the gateway address (that is what listen-address really is because dnsmasq does not really need an address just the interface), but that is not how it was done. After I sent the message, I just got something in from Simon Kelley which has some new info: http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2012q4/006445.html
The heart of it is this: --------------------------------------------------
OK, so this is vaguely embarrassing. Having checked the actual code, rather than the changelog, I see that dnsmasq >=2.61 _already_ does the right thing. Setting --bind-interfaces* and a single --listen-address will cause the code to set SO_BINDTODEVICE on the DHCP socket(s).
So, there is not a problem with the existing libvirt command line. I disagree. I believe that the problem still exists.
What Simon says implies that everything is OK and nothing needs to be done but consider this:
1. What harm does it do to add the interface=<> specification in addition to everything else?
2. Note that Simon states "Setting --bind-interfaces* and a single --listen-address ". Well, I can define multiple IPv4 and/or IPv6 listen-addresses to be on a single virtual interface. From what Simon says, that means all bets are off.
3. I suspect that many/most instances of dnsmasq only has a single address and that is why the problem has not manifested itself.
4. I do not know if a v4 and v6 address counts as one or two.
And one more round on this ... http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2012q4/006447.html --------------------------------------------
OK, learning from past mistakes and checking the code, this is what happens.
You can have as many --listen-address as you like, as long as the addresses all belong to the same interface. This applies to both IPv4 and IPv6, so if you have an interface with two addresses
192.168.0.10 and fd00::10
then
dnsmasq --listen-address=192.168.0.10 --listen-address=fd00::10
would set SO_BINDTODEVICE. But if those addresses belonged to two different interfaces, the same command line would not set SO_BINDTODEVICE. The same applies with more than one IPv4 or IPv6 address, so an interface with addresses
192.168.0.10 and 192.168.1.10
sets SO_BINDTODEVICE with
dnsmasq --listen-address=192.168.1.10 --listen-address=192.168.0.10
so it looks like libvirt is good.
So, interface= is NOT necessary and everything is working just fine using only --listen-address= I still believe that specifying interface= rather than the (possibly multiple) --list-address= is just plain cleaner but ... what now exists works. The problem only occurs when you do something that libvirt's use of dnsmasq does not do ... have multiple dnsmasq instances each with multiple interfaces and only using --listen-address. So ... put it in ... don't put it in ... your options. Gene
On 10/23/2012 04:10 PM, Laine Stump wrote:
Can you provide a bit more info as to what you would like to change without restarting dnsmasq. Right now, the ranges of IP addresses, the IP addresses to listen on, the domain. There may be other things in the future as virNetworkUpdate() gets fleshed out. http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2012q4/006440.html The most difficult problem is that dnsmasq throws away root permissions after start-up, so re-reading the configuration is impossible if acting on the new configuration involves anything that needs root (for instance, binding sockets to ports <1024 or opening the lease file.)
The philosophy is that things that would be really useful to change, without restarting dnsmasq, and certainly don't need root, are re-read on SIGHUP. That's dhcp-host information and dhcp-option information in special configuration files, /etc/hosts and /etc/ethers, and possibly /etc/resolv.conf, if it's not being polled anyway.
For anything else, restarting dnsmasq is a pain-free operation anyway: there's very little state and nothing important is lost over a reboot.
Gene
On 10/24/2012 07:46 AM, Gene Czarcinski wrote:
On 10/23/2012 04:10 PM, Laine Stump wrote:
Can you provide a bit more info as to what you would like to change without restarting dnsmasq. Right now, the ranges of IP addresses, the IP addresses to listen on, the domain. There may be other things in the future as virNetworkUpdate() gets fleshed out. http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2012q4/006440.html
The most difficult problem is that dnsmasq throws away root permissions after start-up, so re-reading the configuration is impossible if acting on the new configuration involves anything that needs root (for instance, binding sockets to ports <1024 or opening the lease file.)
The philosophy is that things that would be really useful to change, without restarting dnsmasq, and certainly don't need root, are re-read on SIGHUP. That's dhcp-host information and dhcp-option information in special configuration files, /etc/hosts and /etc/ethers, and possibly /etc/resolv.conf, if it's not being polled anyway.
For anything else, restarting dnsmasq is a pain-free operation anyway: there's very little state and nothing important is lost over a reboot.
One other thought occurs to me. As far as I know, the only way to get dnsmasq restarted is to do a net-destroy and then a net-start. While you can do this while a virtual guest is on that network, the virtual guest has to be rebooted for it to work again. Is there some way (some command) that will cause dnsmasq to be restarted (possibly with new parameters as it re-does its configuration). If this was done, then little, except some cached names, would be lost). If there is not, maybe there should be. However, there are likely limits such as not changing the gateway addresses on the interface. Gene Gene
On 10/24/2012 07:56 AM, Gene Czarcinski wrote:
On 10/24/2012 07:46 AM, Gene Czarcinski wrote:
On 10/23/2012 04:10 PM, Laine Stump wrote:
Can you provide a bit more info as to what you would like to change without restarting dnsmasq. Right now, the ranges of IP addresses, the IP addresses to listen on, the domain. There may be other things in the future as virNetworkUpdate() gets fleshed out. http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2012q4/006440.html
The most difficult problem is that dnsmasq throws away root permissions after start-up, so re-reading the configuration is impossible if acting on the new configuration involves anything that needs root (for instance, binding sockets to ports <1024 or opening the lease file.)
The philosophy is that things that would be really useful to change, without restarting dnsmasq, and certainly don't need root, are re-read on SIGHUP. That's dhcp-host information and dhcp-option information in special configuration files, /etc/hosts and /etc/ethers, and possibly /etc/resolv.conf, if it's not being polled anyway.
For anything else, restarting dnsmasq is a pain-free operation anyway: there's very little state and nothing important is lost over a reboot.
One other thought occurs to me.
As far as I know, the only way to get dnsmasq restarted is to do a net-destroy and then a net-start. While you can do this while a virtual guest is on that network, the virtual guest has to be rebooted for it to work again.
Well, you could indirectly trigger it by adding (and then removing) a dhcp <range> element with virsh net-update.
Is there some way (some command) that will cause dnsmasq to be restarted (possibly with new parameters as it re-does its configuration). If this was done, then little, except some cached names, would be lost). If there is not, maybe there should be. However, there are likely limits such as not changing the gateway addresses on the interface.
I recently made a patch to check for a running dnsmasq anytime libvirt is restarted. If it's not running, it will be restarted. If it is running, a SIGHUP will be sent. Aside from that, the (newly created) accepted way to change a network's config while the network is up is to use the virNetworkUpdate API (available via virsh net-update). It permits you to add/delete/modify certain parts of the network config and have those changes take effect immediately if desired. the bridge_driver backend of virNetworkUpdate decides when it is necessary to either SIGHUP or restart dnsmasq according to what parts of the network definition have changed. A "live" change of the entire network definition (i.e. with virNetworkDefine() isn't supported, and won't be - anything that you want to be able to change while the network is up should be added to the "sections" modifiable by virNetworkUpdate. This will permit us to easily figure out what re-initialization is needed for a particular change without needing to just redo everything (for example, if a dhcp static host is added, we just need to SIGHUP dnsmasq, but if a dynamic range is added/deleted, we need to restart dnsmasq. If an IP address is added/removed from the bridge, we should be able to write code to simply redo the IP addresses on the existing bridge, rather than tear it down and build a new bridge (although that one isn't implemented yet).
On 10/24/2012 01:31 PM, Laine Stump wrote:
Is there some way (some command) that will cause dnsmasq to be restarted (possibly with new parameters as it re-does its configuration). If this was done, then little, except some cached names, would be lost). If there is not, maybe there should be. However, there are likely limits such as not changing the gateway addresses on the interface. I recently made a patch to check for a running dnsmasq anytime libvirt is restarted. If it's not running, it will be restarted. If it is running, a SIGHUP will be sent.
Aside from that, the (newly created) accepted way to change a network's config while the network is up is to use the virNetworkUpdate API (available via virsh net-update). It permits you to add/delete/modify certain parts of the network config and have those changes take effect immediately if desired. the bridge_driver backend of virNetworkUpdate decides when it is necessary to either SIGHUP or restart dnsmasq according to what parts of the network definition have changed.
A "live" change of the entire network definition (i.e. with virNetworkDefine() isn't supported, and won't be - anything that you want to be able to change while the network is up should be added to the "sections" modifiable by virNetworkUpdate. This will permit us to easily figure out what re-initialization is needed for a particular change without needing to just redo everything (for example, if a dhcp static host is added, we just need to SIGHUP dnsmasq, but if a dynamic range is added/deleted, we need to restart dnsmasq. If an IP address is added/removed from the bridge, we should be able to write code to simply redo the IP addresses on the existing bridge, rather than tear it down and build a new bridge (although that one isn't implemented yet). Now this sounds like pretty much what I wanted. It is OK to restart dnsmasq but not OK to take the network down and then back up while a virtual guest is using it. Changing dhcp-range, hosts, or dhcp-host should be no problem because the network interface remains.
That said, there is something critical and that is the "gateway" addresses. Other software (not dsnmasq) needs to know these addresses because they are really the interface between reality and the virtual network. That should not be able to be changed without taking down the network. Dnsmasq does not need the listen-address specifications if it is given the drive name. It will then listen on port 53 for all networks defined on the interface. For dhcp dnsmasq listens on 0.0.0.0:67/68 for v4 and :::547 for v6. It then filers any incoming packet to make sure that a dhcp-range has been specified for a subnet defined on that interface. I know it is supported [or at least it works] but I am a little fuzzy about the usefulness of multiple IP addresses on the same interface (other than IPv4 and IPv6). Gene
On 10/24/2012 04:27 PM, Gene Czarcinski wrote:
On 10/24/2012 01:31 PM, Laine Stump wrote:
Is there some way (some command) that will cause dnsmasq to be restarted (possibly with new parameters as it re-does its configuration). If this was done, then little, except some cached names, would be lost). If there is not, maybe there should be. However, there are likely limits such as not changing the gateway addresses on the interface. I recently made a patch to check for a running dnsmasq anytime libvirt is restarted. If it's not running, it will be restarted. If it is running, a SIGHUP will be sent.
Aside from that, the (newly created) accepted way to change a network's config while the network is up is to use the virNetworkUpdate API (available via virsh net-update). It permits you to add/delete/modify certain parts of the network config and have those changes take effect immediately if desired. the bridge_driver backend of virNetworkUpdate decides when it is necessary to either SIGHUP or restart dnsmasq according to what parts of the network definition have changed.
A "live" change of the entire network definition (i.e. with virNetworkDefine() isn't supported, and won't be - anything that you want to be able to change while the network is up should be added to the "sections" modifiable by virNetworkUpdate. This will permit us to easily figure out what re-initialization is needed for a particular change without needing to just redo everything (for example, if a dhcp static host is added, we just need to SIGHUP dnsmasq, but if a dynamic range is added/deleted, we need to restart dnsmasq. If an IP address is added/removed from the bridge, we should be able to write code to simply redo the IP addresses on the existing bridge, rather than tear it down and build a new bridge (although that one isn't implemented yet). Now this sounds like pretty much what I wanted. It is OK to restart dnsmasq but not OK to take the network down and then back up while a virtual guest is using it. Changing dhcp-range, hosts, or dhcp-host should be no problem because the network interface remains.
That said, there is something critical and that is the "gateway" addresses. Other software (not dsnmasq) needs to know these addresses because they are really the interface between reality and the virtual network. That should not be able to be changed without taking down the network.
Depends. For the guests, all you need to do is set the linkstate down, then back up; for most OSes that causes the dhcp client to request a new lease. (really, we should probably do the same thing (at least optionally) when changing the range and the static hosts list, if we want to guarantee that all the guests are always obeying those settings.
Dnsmasq does not need the listen-address specifications if it is given the drive name. It will then listen on port 53 for all networks defined on the interface. For dhcp dnsmasq listens on 0.0.0.0:67/68 for v4 and :::547 for v6. It then filers any incoming packet to make sure that a dhcp-range has been specified for a subnet defined on that interface.
Is that 100% true? Awhile back someone filed a bug against libvirt saying that, because they had incorrectly specified the dynamic range to include the broadcast address of the subnet, they were getting a client that had been assigned the broadcast address as its IP.
I know it is supported [or at least it works] but I am a little fuzzy about the usefulness of multiple IP addresses on the same interface (other than IPv4 and IPv6).
Well, I don't think it has much effect for dhcp (maybe it does when a client sends a unicast dhcp renew request (if I'm remembering dhcp correctly - it's been awhile since I looked at the details :-), but for DNS it will respond to DNS requests on any of those addresses. Or are you talking about having multiple IPs in general? I guess it makes it possible for multiple subnets to share the same infrastructure (don't know how useful that is when the infrastructure is virtual :-). Anyway, if something works, someone will figure out a practical use for it.
On 10/24/2012 01:31 PM, Laine Stump wrote:
Is there some way (some command) that will cause dnsmasq to be restarted (possibly with new parameters as it re-does its configuration). If this was done, then little, except some cached names, would be lost). If there is not, maybe there should be. However, there are likely limits such as not changing the gateway addresses on the interface. I recently made a patch to check for a running dnsmasq anytime libvirt is restarted. If it's not running, it will be restarted. If it is running, a SIGHUP will be sent.
Aside from that, the (newly created) accepted way to change a network's config while the network is up is to use the virNetworkUpdate API (available via virsh net-update). It permits you to add/delete/modify certain parts of the network config and have those changes take effect immediately if desired. the bridge_driver backend of virNetworkUpdate decides when it is necessary to either SIGHUP or restart dnsmasq according to what parts of the network definition have changed.
A "live" change of the entire network definition (i.e. with virNetworkDefine() isn't supported, and won't be - anything that you want to be able to change while the network is up should be added to the "sections" modifiable by virNetworkUpdate. This will permit us to easily figure out what re-initialization is needed for a particular change without needing to just redo everything (for example, if a dhcp static host is added, we just need to SIGHUP dnsmasq, but if a dynamic range is added/deleted, we need to restart dnsmasq. If an IP address is added/removed from the bridge, we should be able to write code to simply redo the IP addresses on the existing bridge, rather than tear it down and build a new bridge (although that one isn't implemented yet). Now this sounds like pretty much what I wanted. It is OK to restart dnsmasq but not OK to take the network down and then back up while a virtual guest is using it. Changing dhcp-range, hosts, or dhcp-host should be no problem because the network interface remains.
That said, there is something critical and that is the "gateway" addresses. Other software (not dsnmasq) needs to know these addresses because they are really the interface between reality and the virtual network. That should not be able to be changed without taking down the network. Depends. For the guests, all you need to do is set the linkstate down,
On 10/24/2012 04:27 PM, Gene Czarcinski wrote: then back up; for most OSes that causes the dhcp client to request a new lease. (really, we should probably do the same thing (at least optionally) when changing the range and the static hosts list, if we want to guarantee that all the guests are always obeying those settings.
Dnsmasq does not need the listen-address specifications if it is given the drive name. It will then listen on port 53 for all networks defined on the interface. For dhcp dnsmasq listens on 0.0.0.0:67/68 for v4 and :::547 for v6. It then filers any incoming packet to make sure that a dhcp-range has been specified for a subnet defined on that interface. Is that 100% true? Awhile back someone filed a bug against libvirt saying that, because they had incorrectly specified the dynamic range to include the broadcast address of the subnet, they were getting a client that had been assigned the broadcast address as its IP. The darn corner cases always cause problems. This might even be a bug with dnsmasq ... it should at least gripe about any dhcp-range with includes the broadcast address.
I know it is supported [or at least it works] but I am a little fuzzy about the usefulness of multiple IP addresses on the same interface (other than IPv4 and IPv6). Well, I don't think it has much effect for dhcp (maybe it does when a client sends a unicast dhcp renew request (if I'm remembering dhcp correctly - it's been awhile since I looked at the details :-), but for DNS it will respond to DNS requests on any of those addresses. If you specify the interface, yes dnsmasq should (and does) look at all of the subnetworks. However, I do not know is that is true if you use
On 10/26/2012 10:36 AM, Laine Stump wrote: listen-address. I will try to remember to test that.
Or are you talking about having multiple IPs in general? I guess it makes it possible for multiple subnets to share the same infrastructure (don't know how useful that is when the infrastructure is virtual :-). Anyway, if something works, someone will figure out a practical use for it.
It is possible to run more than one subnet on the same fabric ... real or virtual. There might even be some real-world application where you want to separate a control network from the regular old (Internet accessible) network. Of course, anyone who did not encrypt that second network odes not have all of their oars in the water ;) And very real world application for running multiple networks on the same fabric (the same interface) is: dual stack IPv4 and IPv6. Gene
On 10/24/2012 01:31 PM, Laine Stump wrote:
Well, you could indirectly trigger it by adding (and then removing) a dhcp <range> element with virsh net-update.
Is there some way (some command) that will cause dnsmasq to be restarted (possibly with new parameters as it re-does its configuration). If this was done, then little, except some cached names, would be lost). If there is not, maybe there should be. However, there are likely limits such as not changing the gateway addresses on the interface.
I recently made a patch to check for a running dnsmasq anytime libvirt is restarted. If it's not running, it will be restarted. If it is running, a SIGHUP will be sent.
Aside from that, the (newly created) accepted way to change a network's config while the network is up is to use the virNetworkUpdate API (available via virsh net-update). It permits you to add/delete/modify certain parts of the network config and have those changes take effect immediately if desired. the bridge_driver backend of virNetworkUpdate decides when it is necessary to either SIGHUP or restart dnsmasq according to what parts of the network definition have changed.
A "live" change of the entire network definition (i.e. with virNetworkDefine() isn't supported, and won't be - anything that you want to be able to change while the network is up should be added to the "sections" modifiable by virNetworkUpdate. This will permit us to easily figure out what re-initialization is needed for a particular change without needing to just redo everything (for example, if a dhcp static host is added, we just need to SIGHUP dnsmasq, but if a dynamic range is added/deleted, we need to restart dnsmasq. If an IP address is added/removed from the bridge, we should be able to write code to simply redo the IP addresses on the existing bridge, rather than tear it down and build a new bridge (although that one isn't implemented yet). I finally got around to testing this .... how is this suppose to work? Is this a "work in progress?"
I did not try this first but it is what seemed to work ... I edited a network definition so that it space for a dhcp-range addition. Then, with the network not started, i used net-update to add a ip-dhcp-range and it worked (the file was updated). Edit the xml again to remove the added range, and start the network. Do the same thing again with net-update ... the file was updated and dnsmasq was restarted. What I started out to do and what did not work was to add a dns-host definition to a network. This should not require a restart but result a [Refresh] SIGHUP of the dnsmasq process. 1. Using net-edit to add the dns-host definition [resulted in networkCreate() being executed] and then issuing SIGHUP to the process and the addnhosts file was read and things worked as I wanted them to. 2. Trying to get the same thing accomplished using net-update [dns-host] was a failure. The only thing that happened was to get the following error message: "this function is not supported by the connection driver: can't update 'dns host' section of network 'default'" where I tried a bunch of different networks. Some of them already had a dns-host definition and some did not. Should I expect this to work? Is this a bug? Gene
On 10/23/2012 11:07 AM, Gene Czarcinski wrote:
This patch changes the way parameters are passed to dnsmasq. They are put into a conf-file instead of being on the dnsmasq command line.
**NOTE ** This has updated the related tests for the new data format, etc.
**NOTE** This patch does NOT include specifying interface=
The command line now contains --conf-file=<filename> and a new parameter --conf-dir=<directoryname> has been added.
And the idea here is that people can add stuff into that directory if they want to frob with the dnsmasq config? I can see this leading to support nightmares if we don't somehow mark the system as "tainted" any time there is a file in there (similar to what is done if qemu commandline extensions are used). Aside from that, and the small bits I mention below, this looks fine, and it's so far worked properly (with the --interface patch included) on F17 (dnsmasq-2.63) and F16 (dnsmasq-2.59), and RHEL6 (dnsmasq-2.48). I unfortunately don't have RHEL5 on real hardware to try out. I would like to push this (and --interface) before the freeze, but am uneasy about the conf-dir change. Can we split that into its own patch too, and consider it separately? (some people may want to weigh in on it, and we may want to require)
The new file and directory are put in the same directory as the leases file. ---
First some review of code that *isn't* there - it would be nice if networkRefreshDhcpDaemon() checked for the existence of the conf file and, if it wasn't there, called networkRestartDhcpDaemon() instead. Thinking about it, though, I suppose it's not really necessary, since the existing dnsmasq process should behave identically to the new one. (that won't be the case when the --interface option is added though. I'm not sure how to deal with that; I suppose we can just tell people they'll have to restart their networks in order to get that fix.)
src/network/bridge_driver.c | 179 ++++++++++++++------- src/network/bridge_driver.h | 8 +- tests/networkxml2argvdata/isolated-network.argv | 24 +-- .../networkxml2argvdata/nat-network-dns-hosts.argv | 14 +- .../nat-network-dns-srv-record-minimal.argv | 35 ++-- .../nat-network-dns-srv-record.argv | 35 ++-- .../nat-network-dns-txt-record.argv | 29 ++-- tests/networkxml2argvdata/nat-network.argv | 27 ++-- tests/networkxml2argvdata/netboot-network.argv | 28 ++-- .../networkxml2argvdata/netboot-proxy-network.argv | 25 +-- tests/networkxml2argvdata/routed-network.argv | 12 +- tests/networkxml2argvtest.c | 44 +---- 12 files changed, 264 insertions(+), 196 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8837843..ab13df5 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -136,6 +136,16 @@ networkDnsmasqLeaseFileNameFunc networkDnsmasqLeaseFileName = networkDnsmasqLeaseFileNameDefault;
static char * +networkDnsmasqConfigFileName(const char *netname) +{ + char *conffile; + + ignore_value(virAsprintf(&conffile, DNSMASQ_STATE_DIR "/%s.conf", + netname)); + return conffile; +} + +static char * networkRadvdPidfileBasename(const char *netname) { /* this is simple but we want to be sure it's consistently done */ @@ -559,23 +569,25 @@ networkBuildDnsmasqHostsfile(dnsmasqContext *dctx, return 0; }
- + /* build the dnsmasq conf file contents */ static int -networkBuildDnsmasqArgv(virNetworkObjPtr network, +networkDnsmasqConfContents(virNetworkObjPtr network,
Extra trailing space on the line above. You should run "make syntax-check" before sending patches, and fix anything it complains about.
virNetworkIpDefPtr ipdef, const char *pidfile, - virCommandPtr cmd, + char **configstr, dnsmasqContext *dctx) { - int r, ret = -1; + virBuffer configbuf = VIR_BUFFER_INITIALIZER;; + int r, ret = -1, ii; int nbleases = 0; - int ii; char *record = NULL; char *recordPort = NULL; char *recordWeight = NULL; char *recordPriority = NULL; virNetworkIpDefPtr tmpipdef;
+ *configstr = NULL; + /* * NB, be careful about syntax for dnsmasq options in long format. * @@ -595,28 +607,22 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, * very explicit on this. */
- /* - * Needed to ensure dnsmasq uses same algorithm for processing - * multiple namedriver entries in /etc/resolv.conf as GLibC. - */ - virCommandAddArgList(cmd, "--strict-order", "--bind-interfaces", NULL); - + /* create dnsmasq config file appropriate for this network */ + virBufferAsprintf(&configbuf, "# dnsmasq conf file created by libvirt\n" + "strict-order\n" + "bind-interfaces\n" + "except-interface=lo\n" + "domain-needed\n" + "local=/%s/\n", + network->def->domain ? network->def->domain : ""); if (network->def->domain) - virCommandAddArgPair(cmd, "--domain", network->def->domain); - /* need to specify local even if no domain specified */ - virCommandAddArgFormat(cmd, "--local=/%s/", - network->def->domain ? network->def->domain : ""); - virCommandAddArg(cmd, "--domain-needed"); + virBufferAsprintf(&configbuf, + "domain=%s\n" + "expand-hosts\n", + network->def->domain);
The "virBufferAsprintf" line has a trailing blank. Also the if has a multiline body, so it requires {} around it per the coding guildelines.
if (pidfile) - virCommandAddArgPair(cmd, "--pid-file", pidfile); - - /* *no* conf file */ - virCommandAddArg(cmd, "--conf-file="); - - virCommandAddArgList(cmd, - "--except-interface", "lo", - NULL); + virBufferAsprintf(&configbuf, "pid-file=%s\n", pidfile);
/* If this is an isolated network, set the default route option * (3) to be empty to avoid setting a default route that's @@ -626,16 +632,21 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, * to build a connection to the outside). */ if (network->def->forwardType == VIR_NETWORK_FORWARD_NONE) { - virCommandAddArgList(cmd, "--dhcp-option=3", - "--no-resolv", NULL); + virBufferAsprintf(&configbuf, "dhcp-option=3\n" + "no-resolv\n"); }
+ /* + * Needed to ensure dnsmasq uses same algorithm for processing + * multiple namedriver entries in /etc/resolv.conf as GLibC. + */ + if (network->def->dns != NULL) { virNetworkDNSDefPtr dns = network->def->dns; int i;
for (i = 0; i < dns->ntxtrecords; i++) { - virCommandAddArgFormat(cmd, "--txt-record=%s,%s", + virBufferAsprintf(&configbuf, "txt-record=%s,%s\n", dns->txtrecords[i].name, dns->txtrecords[i].value); } @@ -673,7 +684,7 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, goto cleanup; }
- virCommandAddArgPair(cmd, "--srv-host", record); + virBufferAsprintf(&configbuf, "srv-host=%s\n", record); VIR_FREE(record); VIR_FREE(recordPort); VIR_FREE(recordWeight); @@ -682,21 +693,14 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, } }
- /* - * --interface does not actually work with dnsmasq < 2.47, - * due to DAD for ipv6 addresses on the interface. - * - * virCommandAddArgList(cmd, "--interface", ipdef->bridge, NULL); - * - * So listen on all defined IPv[46] addresses - */
This comment shouldn't be removed until --interface is added in (and even then, it should be left, somewhat modified, for historical/informational purposes).
for (ii = 0; (tmpipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii)); - ii++) { + ii++) + { char *ipaddr = virSocketAddrFormat(&tmpipdef->address); if (!ipaddr) goto cleanup; - virCommandAddArgList(cmd, "--listen-address", ipaddr, NULL); + virBufferAsprintf(&configbuf, "listen-address=%s\n", ipaddr); VIR_FREE(ipaddr); }
@@ -710,8 +714,8 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, VIR_FREE(saddr); goto cleanup; } - virCommandAddArg(cmd, "--dhcp-range"); - virCommandAddArgFormat(cmd, "%s,%s", saddr, eaddr); + virBufferAsprintf(&configbuf, "dhcp-range=%s,%s\n", + saddr, eaddr); VIR_FREE(saddr); VIR_FREE(eaddr); nbleases += virSocketAddrGetRange(&ipdef->ranges[r].start, @@ -727,8 +731,7 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, char *bridgeaddr = virSocketAddrFormat(&ipdef->address); if (!bridgeaddr) goto cleanup; - virCommandAddArg(cmd, "--dhcp-range"); - virCommandAddArgFormat(cmd, "%s,static", bridgeaddr); + virBufferAsprintf(&configbuf, "dhcp-range=%s,static\n", bridgeaddr); VIR_FREE(bridgeaddr); }
@@ -736,17 +739,13 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, char *leasefile = networkDnsmasqLeaseFileName(network->def->name); if (!leasefile) goto cleanup; - virCommandAddArgFormat(cmd, "--dhcp-leasefile=%s", leasefile); + virBufferAsprintf(&configbuf, "dhcp-leasefile=%s\n", leasefile); VIR_FREE(leasefile); - virCommandAddArgFormat(cmd, "--dhcp-lease-max=%d", nbleases); + virBufferAsprintf(&configbuf, "dhcp-lease-max=%d\n", nbleases); }
if (ipdef->nranges || ipdef->nhosts) - virCommandAddArg(cmd, "--dhcp-no-override"); - - /* add domain to any non-qualified hostnames in /etc/hosts or addn-hosts */ - if (network->def->domain) - virCommandAddArg(cmd, "--expand-hosts"); + virBufferAsprintf(&configbuf, "dhcp-no-override\n");
if (networkBuildDnsmasqHostsfile(dctx, ipdef, network->def->dns) < 0) goto cleanup; @@ -756,38 +755,42 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, * file to allow for runtime additions. */ if (ipdef->nranges || ipdef->nhosts) - virCommandAddArgPair(cmd, "--dhcp-hostsfile", + virBufferAsprintf(&configbuf, "dhcp-hostsfile=%s\n", dctx->hostsfile->path);
/* Likewise, always create this file and put it on the commandline, to allow for * for runtime additions. */ - virCommandAddArgPair(cmd, "--addn-hosts", + virBufferAsprintf(&configbuf, "addn-hosts=%s\n", dctx->addnhostsfile->path);
if (ipdef->tftproot) { - virCommandAddArgList(cmd, "--enable-tftp", - "--tftp-root", ipdef->tftproot, - NULL); + virBufferAsprintf(&configbuf, "enable-tftp\n"); + virBufferAsprintf(&configbuf, "tftp-root=%s\n", ipdef->tftproot); } if (ipdef->bootfile) { - virCommandAddArg(cmd, "--dhcp-boot"); if (VIR_SOCKET_ADDR_VALID(&ipdef->bootserver)) { char *bootserver = virSocketAddrFormat(&ipdef->bootserver);
if (!bootserver) goto cleanup; - virCommandAddArgFormat(cmd, "%s%s%s", + virBufferAsprintf(&configbuf, "dhcp-boot=%s%s%s\n", ipdef->bootfile, ",,", bootserver); VIR_FREE(bootserver); } else { - virCommandAddArg(cmd, ipdef->bootfile); + virBufferAsprintf(&configbuf, "dhcp-boot=%s\n", ipdef->bootfile); } } } + if (!(*configstr = virBufferContentAndReset(&configbuf))) { + virReportOOMError(); + goto cleanup; + }
ret = 0; + cleanup: + virBufferFreeAndReset(&configbuf); VIR_FREE(record); VIR_FREE(recordPort); VIR_FREE(recordWeight); @@ -795,13 +798,18 @@ cleanup: return ret; }
+ /* build the dnsmasq command line */ int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, - char *pidfile, dnsmasqContext *dctx) + char *pidfile, dnsmasqContext *dctx, + char *configdir, + int testOnly, char **testConfigStr) { virCommandPtr cmd = NULL; int ret = -1, ii; virNetworkIpDefPtr ipdef; + char *configfile = NULL; + char *configstr = NULL;
network->dnsmasqPid = -1;
@@ -825,15 +833,41 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdou if (!virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, 0)) return 0;
- cmd = virCommandNew(DNSMASQ); - if (networkBuildDnsmasqArgv(network, ipdef, pidfile, cmd, dctx) < 0) { + if (networkDnsmasqConfContents(network, ipdef, pidfile, &configstr, dctx) < 0) + goto cleanup; + if (!configstr) + goto cleanup; + + if (testOnly) { + *testConfigStr = configstr; + return 0; + } + + /* construct the filename */ + if (!(configfile = networkDnsmasqConfigFileName(network->def->name))) { + virReportOOMError(); + goto cleanup; + } + + /* Write the file */ + if (virFileWriteStr(configfile, configstr, 0600) < 0) { + virReportSystemError(errno, + _("couldn't write dnsmasq config file '%s'"), + configfile); goto cleanup; } + VIR_INFO("dnsmasq conf file %s written", configfile); + + cmd = virCommandNew(DNSMASQ); + virCommandAddArgFormat(cmd, "--conf-file=%s", configfile); + virCommandAddArgFormat(cmd, "--conf-dir=%s", configdir);
if (cmdout) *cmdout = cmd; ret = 0; cleanup: + VIR_FREE(configstr); + VIR_FREE(configfile); if (ret < 0) virCommandFree(cmd); return ret; @@ -844,9 +878,12 @@ networkStartDhcpDaemon(virNetworkObjPtr network) { virCommandPtr cmd = NULL; char *pidfile = NULL; + char *testconfigstr = NULL; + char *configdir = NULL; int ret = -1; dnsmasqContext *dctx = NULL;
+ VIR_INFO("starting dhcp daemon (dnsmasq)"); if (!virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, 0)) { /* no IPv6 addresses, so we don't need to run radvd */ ret = 0; @@ -882,7 +919,18 @@ networkStartDhcpDaemon(virNetworkObjPtr network) if (dctx == NULL) goto cleanup;
- ret = networkBuildDhcpDaemonCommandLine(network, &cmd, pidfile, dctx); + ignore_value(virAsprintf(&configdir, DNSMASQ_STATE_DIR "/%s.d", + network->def->name)); + if (virFileMakePath(configdir) < 0) { + virReportSystemError(errno, + _("cannot create directory %s"), + configdir); + goto cleanup; + } + + + ret = networkBuildDhcpDaemonCommandLine(network, &cmd, pidfile, dctx, + configdir, 0, &testconfigstr); if (ret < 0) goto cleanup;
@@ -911,6 +959,7 @@ networkStartDhcpDaemon(virNetworkObjPtr network) ret = 0; cleanup: VIR_FREE(pidfile); + VIR_FREE(configdir); virCommandFree(cmd); dnsmasqContextFree(dctx); return ret; @@ -2841,6 +2890,14 @@ static int networkUndefine(virNetworkPtr net) { } }
+ { + char *configfile = networkDnsmasqConfigFileName(network->def->name); + if (!configfile) + goto cleanup; + unlink(configfile); + VIR_FREE(configfile); + } + if (dhcp_present) { char *leasefile; dnsmasqContext *dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR); diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index 0fae275..00675c4 100644 --- a/src/network/bridge_driver.h +++ b/src/network/bridge_driver.h @@ -48,15 +48,17 @@ int networkGetNetworkAddress(const char *netname, char **netaddr)
int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, char *pidfile, - dnsmasqContext *dctx) - ; + dnsmasqContext *dctx, + char *configdir, + int testOnly, char **testConfigStr); # else /* Define no-op replacements that don't drag in any link dependencies. */ # define networkAllocateActualDevice(iface) 0 # define networkNotifyActualDevice(iface) 0 # define networkReleaseActualDevice(iface) 0 # define networkGetNetworkAddress(netname, netaddr) (-2) -# define networkBuildDhcpDaemonCommandLine(network, cmdout, pidfile, dctx) 0 +# define networkBuildDhcpDaemonCommandLine(network, cmdout, pidfile, \ + dctx, configdir, testOnly, testConfigStr) 0 # endif
typedef char *(*networkDnsmasqLeaseFileNameFunc)(const char *netname); diff --git a/tests/networkxml2argvdata/isolated-network.argv b/tests/networkxml2argvdata/isolated-network.argv index 13e77b2..042158b 100644 --- a/tests/networkxml2argvdata/isolated-network.argv +++ b/tests/networkxml2argvdata/isolated-network.argv @@ -1,9 +1,15 @@ -@DNSMASQ@ --strict-order --bind-interfaces \ ---local=// --domain-needed --conf-file= \ ---except-interface lo --dhcp-option=3 --no-resolv \ ---listen-address 192.168.152.1 \ ---dhcp-range 192.168.152.2,192.168.152.254 \ ---dhcp-leasefile=/var/lib/libvirt/dnsmasq/private.leases --dhcp-lease-max=253 \ ---dhcp-no-override \ ---dhcp-hostsfile=/var/lib/libvirt/dnsmasq/private.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/private.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=// +dhcp-option=3 +no-resolv +listen-address=192.168.152.1 +dhcp-range=192.168.152.2,192.168.152.254 +dhcp-leasefile=/var/lib/libvirt/dnsmasq/private.leases +dhcp-lease-max=253 +dhcp-no-override +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/private.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/private.addnhosts diff --git a/tests/networkxml2argvdata/nat-network-dns-hosts.argv b/tests/networkxml2argvdata/nat-network-dns-hosts.argv index 03a0676..91eb682 100644 --- a/tests/networkxml2argvdata/nat-network-dns-hosts.argv +++ b/tests/networkxml2argvdata/nat-network-dns-hosts.argv @@ -1,4 +1,10 @@ -@DNSMASQ@ --strict-order --bind-interfaces --domain=example.com \ ---local=/example.com/ --domain-needed \ ---conf-file= --except-interface lo --listen-address 192.168.122.1 \ ---expand-hosts --addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=/example.com/ +domain=example.com +expand-hosts +listen-address=192.168.122.1 +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv b/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv index 210a60c..d92497b 100644 --- a/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv +++ b/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv @@ -1,17 +1,18 @@ -@DNSMASQ@ \ ---strict-order \ ---bind-interfaces \ ---local=// --domain-needed --conf-file= \ ---except-interface lo \ ---srv-host=name.tcp.,,,, \ ---listen-address 192.168.122.1 \ ---listen-address 192.168.123.1 \ ---listen-address 2001:db8:ac10:fe01::1 \ ---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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=// +srv-host=name.tcp.,,,, +listen-address=192.168.122.1 +listen-address=192.168.123.1 +listen-address=2001:db8:ac10:fe01::1 +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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2argvdata/nat-network-dns-srv-record.argv b/tests/networkxml2argvdata/nat-network-dns-srv-record.argv index 833d3cd..d8846c2 100644 --- a/tests/networkxml2argvdata/nat-network-dns-srv-record.argv +++ b/tests/networkxml2argvdata/nat-network-dns-srv-record.argv @@ -1,17 +1,18 @@ -@DNSMASQ@ \ ---strict-order \ ---bind-interfaces \ ---local=// --domain-needed --conf-file= \ ---except-interface lo \ ---srv-host=name.tcp.test-domain-name,.,1024,10,10 \ ---listen-address 192.168.122.1 \ ---listen-address 192.168.123.1 \ ---listen-address 2001:db8:ac10:fe01::1 \ ---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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=// +srv-host=name.tcp.test-domain-name,.,1024,10,10 +listen-address=192.168.122.1 +listen-address=192.168.123.1 +listen-address=2001:db8:ac10:fe01::1 +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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv index 3481507..bf00513 100644 --- a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv +++ b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv @@ -1,11 +1,18 @@ -@DNSMASQ@ --strict-order --bind-interfaces \ ---local=// --domain-needed --conf-file= \ ---except-interface lo '--txt-record=example,example value' \ ---listen-address 192.168.122.1 --listen-address 192.168.123.1 \ ---listen-address 2001:db8:ac10:fe01::1 \ ---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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=// +txt-record=example,example value +listen-address=192.168.122.1 +listen-address=192.168.123.1 +listen-address=2001:db8:ac10:fe01::1 +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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2argvdata/nat-network.argv b/tests/networkxml2argvdata/nat-network.argv index 37fd2fc..d542bbc 100644 --- a/tests/networkxml2argvdata/nat-network.argv +++ b/tests/networkxml2argvdata/nat-network.argv @@ -1,10 +1,17 @@ -@DNSMASQ@ --strict-order --bind-interfaces \ ---local=// --domain-needed --conf-file= \ ---except-interface lo --listen-address 192.168.122.1 \ ---listen-address 192.168.123.1 --listen-address 2001:db8:ac10:fe01::1 \ ---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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=// +listen-address=192.168.122.1 +listen-address=192.168.123.1 +listen-address=2001:db8:ac10:fe01::1 +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-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2argvdata/netboot-network.argv b/tests/networkxml2argvdata/netboot-network.argv index 5408eb7..4f5fedd 100644 --- a/tests/networkxml2argvdata/netboot-network.argv +++ b/tests/networkxml2argvdata/netboot-network.argv @@ -1,10 +1,18 @@ -@DNSMASQ@ --strict-order --bind-interfaces --domain=example.com \ ---local=/example.com/ --domain-needed --conf-file= \ ---except-interface lo --listen-address 192.168.122.1 \ ---dhcp-range 192.168.122.2,192.168.122.254 \ ---dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases \ ---dhcp-lease-max=253 --dhcp-no-override --expand-hosts \ ---dhcp-hostsfile=/var/lib/libvirt/dnsmasq/netboot.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/netboot.addnhosts \ ---enable-tftp \ ---tftp-root /var/lib/tftproot --dhcp-boot pxeboot.img\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=/example.com/ +domain=example.com +expand-hosts +listen-address=192.168.122.1 +dhcp-range=192.168.122.2,192.168.122.254 +dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases +dhcp-lease-max=253 +dhcp-no-override +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/netboot.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/netboot.addnhosts +enable-tftp +tftp-root=/var/lib/tftproot +dhcp-boot=pxeboot.img diff --git a/tests/networkxml2argvdata/netboot-proxy-network.argv b/tests/networkxml2argvdata/netboot-proxy-network.argv index 21e01e3..8b9c03a 100644 --- a/tests/networkxml2argvdata/netboot-proxy-network.argv +++ b/tests/networkxml2argvdata/netboot-proxy-network.argv @@ -1,9 +1,16 @@ -@DNSMASQ@ --strict-order --bind-interfaces --domain=example.com \ ---local=/example.com/ --domain-needed --conf-file= \ ---except-interface lo --listen-address 192.168.122.1 \ ---dhcp-range 192.168.122.2,192.168.122.254 \ ---dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases \ ---dhcp-lease-max=253 --dhcp-no-override --expand-hosts \ ---dhcp-hostsfile=/var/lib/libvirt/dnsmasq/netboot.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/netboot.addnhosts \ ---dhcp-boot pxeboot.img,,10.20.30.40\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=/example.com/ +domain=example.com +expand-hosts +listen-address=192.168.122.1 +dhcp-range=192.168.122.2,192.168.122.254 +dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases +dhcp-lease-max=253 +dhcp-no-override +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/netboot.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/netboot.addnhosts +dhcp-boot=pxeboot.img,,10.20.30.40 diff --git a/tests/networkxml2argvdata/routed-network.argv b/tests/networkxml2argvdata/routed-network.argv index 9fedb2b..ad9e121 100644 --- a/tests/networkxml2argvdata/routed-network.argv +++ b/tests/networkxml2argvdata/routed-network.argv @@ -1,4 +1,8 @@ -@DNSMASQ@ --strict-order --bind-interfaces \ ---local=// --domain-needed --conf-file= \ ---except-interface lo --listen-address 192.168.122.1 \ ---addn-hosts=/var/lib/libvirt/dnsmasq/local.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +bind-interfaces +except-interface=lo +domain-needed +local=// +listen-address=192.168.122.1 +addn-hosts=/var/lib/libvirt/dnsmasq/local.addnhosts diff --git a/tests/networkxml2argvtest.c b/tests/networkxml2argvtest.c index 87519e4..78ac8cf 100644 --- a/tests/networkxml2argvtest.c +++ b/tests/networkxml2argvtest.c @@ -15,37 +15,6 @@ #include "memory.h" #include "network/bridge_driver.h"
-/* Replace all occurrences of @token in @buf by @replacement and adjust size of - * @buf accordingly. Returns 0 on success and -1 on out-of-memory errors. */ -static int replaceTokens(char **buf, const char *token, const char *replacement) { - size_t token_start, token_end; - size_t buf_len, rest_len; - const size_t token_len = strlen(token); - const size_t replacement_len = strlen(replacement); - const int diff = replacement_len - token_len; - - buf_len = rest_len = strlen(*buf) + 1; - token_end = 0; - for (;;) { - char *match = strstr(*buf + token_end, token); - if (match == NULL) - break; - token_start = match - *buf; - rest_len -= token_start + token_len - token_end; - token_end = token_start + token_len; - buf_len += diff; - if (diff > 0) - if (VIR_REALLOC_N(*buf, buf_len) < 0) - return -1; - if (diff != 0) - memmove(*buf + token_end + diff, *buf + token_end, rest_len); - memcpy(*buf + token_start, replacement, replacement_len); - token_end += diff; - } - /* if diff < 0, we could shrink the buffer here... */ - return 0; -} - static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) { char *inXmlData = NULL; char *outArgvData = NULL; @@ -55,6 +24,7 @@ static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) { virNetworkObjPtr obj = NULL; virCommandPtr cmd = NULL; char *pidfile = NULL; + char *configdir = NULL; dnsmasqContext *dctx = NULL;
if (virtTestLoadFile(inxml, &inXmlData) < 0) @@ -62,10 +32,6 @@ static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) {
if (virtTestLoadFile(outargv, &outArgvData) < 0) goto fail; - - if (replaceTokens(&outArgvData, "@DNSMASQ@", DNSMASQ)) - goto fail; - if (!(dev = virNetworkDefParseString(inXmlData))) goto fail;
@@ -78,12 +44,9 @@ static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv) { if (dctx == NULL) goto fail;
- if (networkBuildDhcpDaemonCommandLine(obj, &cmd, pidfile, dctx) < 0) - goto fail; - - if (!(actual = virCommandToString(cmd))) + if (networkBuildDhcpDaemonCommandLine(obj, &cmd, pidfile,
You have an extra trailing space on the line above. Would have been found with "make syntax-check".
+ dctx, configdir, 1, &actual) < 0) goto fail; - if (STRNEQ(outArgvData, actual)) { virtTestDifference(stderr, outArgvData, actual); goto fail; @@ -147,7 +110,6 @@ mymain(void) if (virtTestRun("Network XML-2-Argv " name, \ 1, testCompareXMLToArgvHelper, (name)) < 0) \ ret = -1 - DO_TEST("isolated-network"); DO_TEST("routed-network"); DO_TEST("nat-network");
The problem is that, without interface=, bind-interfaces does not work. Bind-interfaces uses SO_BINDTODEVICE. Unless this in in effect, there is no guarantee that the kernel will route DHCP (v4 or v6) packets to the correct instance of dnsmasq, when there is more than one. **NOTE** This patch assumes that the patch to put dnsmasq parameters into a file instead of the command line, has been applied. --- src/network/bridge_driver.c | 6 ++++++ tests/networkxml2argvdata/isolated-network.argv | 1 + tests/networkxml2argvdata/nat-network-dns-hosts.argv | 1 + tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv | 1 + tests/networkxml2argvdata/nat-network-dns-srv-record.argv | 1 + tests/networkxml2argvdata/nat-network-dns-txt-record.argv | 1 + tests/networkxml2argvdata/nat-network.argv | 1 + tests/networkxml2argvdata/netboot-network.argv | 1 + tests/networkxml2argvdata/netboot-proxy-network.argv | 1 + tests/networkxml2argvdata/routed-network.argv | 1 + 10 files changed, 15 insertions(+) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index ab13df5..6e5d9da 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -693,6 +693,12 @@ networkDnsmasqConfContents(virNetworkObjPtr network, } } + /* This must be defined so that the kernel knows which dnsmasq to route + * packets to when more than one instance if running + */ + if (network->def->bridge != NULL) + virBufferAsprintf(&configbuf, "interface=%s\n", network->def->bridge); + for (ii = 0; (tmpipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii)); ii++) diff --git a/tests/networkxml2argvdata/isolated-network.argv b/tests/networkxml2argvdata/isolated-network.argv index 042158b..abcde93 100644 --- a/tests/networkxml2argvdata/isolated-network.argv +++ b/tests/networkxml2argvdata/isolated-network.argv @@ -6,6 +6,7 @@ domain-needed local=// dhcp-option=3 no-resolv +interface=virbr2 listen-address=192.168.152.1 dhcp-range=192.168.152.2,192.168.152.254 dhcp-leasefile=/var/lib/libvirt/dnsmasq/private.leases diff --git a/tests/networkxml2argvdata/nat-network-dns-hosts.argv b/tests/networkxml2argvdata/nat-network-dns-hosts.argv index 91eb682..7dce6f9 100644 --- a/tests/networkxml2argvdata/nat-network-dns-hosts.argv +++ b/tests/networkxml2argvdata/nat-network-dns-hosts.argv @@ -6,5 +6,6 @@ domain-needed local=/example.com/ domain=example.com expand-hosts +interface=virbr0 listen-address=192.168.122.1 addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv b/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv index d92497b..d87d438 100644 --- a/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv +++ b/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv @@ -5,6 +5,7 @@ except-interface=lo domain-needed local=// srv-host=name.tcp.,,,, +interface=virbr0 listen-address=192.168.122.1 listen-address=192.168.123.1 listen-address=2001:db8:ac10:fe01::1 diff --git a/tests/networkxml2argvdata/nat-network-dns-srv-record.argv b/tests/networkxml2argvdata/nat-network-dns-srv-record.argv index d8846c2..53882fe 100644 --- a/tests/networkxml2argvdata/nat-network-dns-srv-record.argv +++ b/tests/networkxml2argvdata/nat-network-dns-srv-record.argv @@ -5,6 +5,7 @@ except-interface=lo domain-needed local=// srv-host=name.tcp.test-domain-name,.,1024,10,10 +interface=virbr0 listen-address=192.168.122.1 listen-address=192.168.123.1 listen-address=2001:db8:ac10:fe01::1 diff --git a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv index bf00513..cc3ed28 100644 --- a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv +++ b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv @@ -5,6 +5,7 @@ except-interface=lo domain-needed local=// txt-record=example,example value +interface=virbr0 listen-address=192.168.122.1 listen-address=192.168.123.1 listen-address=2001:db8:ac10:fe01::1 diff --git a/tests/networkxml2argvdata/nat-network.argv b/tests/networkxml2argvdata/nat-network.argv index d542bbc..431fffb 100644 --- a/tests/networkxml2argvdata/nat-network.argv +++ b/tests/networkxml2argvdata/nat-network.argv @@ -4,6 +4,7 @@ bind-interfaces except-interface=lo domain-needed local=// +interface=virbr0 listen-address=192.168.122.1 listen-address=192.168.123.1 listen-address=2001:db8:ac10:fe01::1 diff --git a/tests/networkxml2argvdata/netboot-network.argv b/tests/networkxml2argvdata/netboot-network.argv index 4f5fedd..8405095 100644 --- a/tests/networkxml2argvdata/netboot-network.argv +++ b/tests/networkxml2argvdata/netboot-network.argv @@ -6,6 +6,7 @@ domain-needed local=/example.com/ domain=example.com expand-hosts +interface=virbr1 listen-address=192.168.122.1 dhcp-range=192.168.122.2,192.168.122.254 dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases diff --git a/tests/networkxml2argvdata/netboot-proxy-network.argv b/tests/networkxml2argvdata/netboot-proxy-network.argv index 8b9c03a..d7c8966 100644 --- a/tests/networkxml2argvdata/netboot-proxy-network.argv +++ b/tests/networkxml2argvdata/netboot-proxy-network.argv @@ -6,6 +6,7 @@ domain-needed local=/example.com/ domain=example.com expand-hosts +interface=virbr1 listen-address=192.168.122.1 dhcp-range=192.168.122.2,192.168.122.254 dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases diff --git a/tests/networkxml2argvdata/routed-network.argv b/tests/networkxml2argvdata/routed-network.argv index ad9e121..771240f 100644 --- a/tests/networkxml2argvdata/routed-network.argv +++ b/tests/networkxml2argvdata/routed-network.argv @@ -4,5 +4,6 @@ bind-interfaces except-interface=lo domain-needed local=// +interface=virbr1 listen-address=192.168.122.1 addn-hosts=/var/lib/libvirt/dnsmasq/local.addnhosts -- 1.7.11.7
On 10/23/2012 11:07 AM, Gene Czarcinski wrote:
The problem is that, without interface=, bind-interfaces does not work. Bind-interfaces uses SO_BINDTODEVICE. Unless this in in effect, there is no guarantee that the kernel will route DHCP (v4 or v6) packets to the correct instance of dnsmasq, when there is more than one.
Gene - did I understand the last several messages on this topic correctly? I *think* it all said that we don't need --interface, and dnsmasq will behave identically as long as all the ip addresses specified on the commandline (or in the conf file) are on the same interface, right? Although I'm holding off on the "change from long commandline to using a conf file" patch until after 1.0.0 is released (due to the potential for a regression), I had been considering pushing youre earlier version of the --interface patch *before* 1.0.0 (under the assumption that people would experience breakage if we didn't have it). Now I *think* that I don't need to do that, but watned to verify. In addition to the problem that Simon had originally thought we would have, are there any other easily identifiable bugs caused by a lack of --interface=? If so, I still may consider pushing it. I've tested on everything down to RHEL5 (which has dnsmasq-2.45); when discussing this with Dan Berrange earlier in the week he said he recalled some sort of race condition where the interface sometimes wasn't available early enough (or something like that) when using --interface, which was why it was taken out. I haven't seen that, but then I may not be exercising it in the right way (or maybe it's not as problematic when both --interface and --listen-address are specified). At any rate, if there is still a specific problem that it is solving, we can push that change and deal with any fallout found on old distros later (since my initial testing doesn't find any, I'm hoping that's fairly safe). If not, then I think we should leave it, at least until after 1.0.0, and maybe push it then.
participants (2)
-
Gene Czarcinski -
Laine Stump