Re: [libvirt] [PATCH 2/3] network: allow disabling dnsmasq's DNS server

On Aug 18, 2016 5:01 AM, "Michal Privoznik" <mprivozn(a)redhat.com&gt; wrote: > > On 12.08.2016 04:41, Laine Stump wrote: >> If you define a libvirt virtual network with one or more IP addresses, >> it starts up an instance of dnsmasq. It's always been possible to >> avoid dnsmasq's dhcp server (simply don't include a <dhcp> element), >> but until now it wasn't possible to avoid having the DNS server >> listening; even if the network has no <dns> element, it is started >> using default settings. >> >> This patch adds a new attribute to <dns>: enable='yes|no'. For >> backward compatibility, it defaults to 'yes', but if you don't want a >> DNS server created for the network, you can simply add: >> >> <dns enable='no'/> >> >> to the network configuration, and next time the network is started >> there will be no dns server created (if there is dhcp configuration, >> dnsmasq will be started with "port=0" which disables the DNS server; >> if there is no dhcp configuration, dnsmasq won't be started at all). >> --- >> docs/formatnetwork.html.in | 12 ++ >> docs/schemas/network.rng | 5 + >> src/conf/network_conf.c | 36 ++++- >> src/conf/network_conf.h | 1 + >> src/network/bridge_driver.c | 146 ++++++++++++--------- >> .../networkxml2confdata/routed-network-no-dns.conf | 11 ++ >> .../networkxml2confdata/routed-network-no-dns.xml | 10 ++ >> tests/networkxml2conftest.c | 1 + >> tests/networkxml2xmlin/routed-network-no-dns.xml | 10 ++ >> tests/networkxml2xmlout/routed-network-no-dns.xml | 12 ++ >> tests/networkxml2xmltest.c | 1 + >> 11 files changed, 179 insertions(+), 66 deletions(-) >> create mode 100644 tests/networkxml2confdata/routed-network-no-dns.conf >> create mode 100644 tests/networkxml2confdata/routed-network-no-dns.xml >> create mode 100644 tests/networkxml2xmlin/routed-network-no-dns.xml >> create mode 100644 tests/networkxml2xmlout/routed-network-no-dns.xml >> >> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in >> index 12d1bed..e103dd7 100644 >> --- a/docs/formatnetwork.html.in >> +++ b/docs/formatnetwork.html.in >> @@ -886,6 +886,18 @@ >> server <span class="since">Since 0.9.3</span>. >> >> <p> >> + The dns element can have an optional <code>enable</code> >> + attribute <span class="since">Since 2.2.0</span>. >> + If <code>enable</code> is "no", then no DNS server will be >> + setup by libvirt for this network (and any other >> + configuration in <code>&lt;dns&gt;</code> will be ignored). >> + If <code>enable</code> is "yes" or unspecified (including >> + the complete absence of any <code>&lt;dns&gt;</code> >> + element) then a DNS server will be setup by libvirt to >> + listen on all IP addresses specified in the network's >> + configuration. >> + </p> > > Le sigh. I wish that we could just disable dns if the tag is not present > in the nework XML. But we can't do that, can we? > Nope :-(. Backward compatibility and all that.

>> + <p> >> The dns element >> can have an optional <code>forwardPlainNames</code> >> attribute <span class="since">Since 1.1.2</span>. > >> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c >> index 6820bde..490574f 100644 >> --- a/src/conf/network_conf.c >> +++ b/src/conf/network_conf.c >> @@ -1335,6 +1335,7 @@ virNetworkDNSDefParseXML(const char *networkName, >> xmlNodePtr *txtNodes = NULL; >> xmlNodePtr *fwdNodes = NULL; >> char *forwardPlainNames = NULL; >> + char *enable = NULL; >> int nhosts, nsrvs, ntxts, nfwds; >> size_t i; >> int ret = -1; >> @@ -1342,6 +1343,18 @@ virNetworkDNSDefParseXML(const char *networkName, >> >> ctxt->node = node; >> >> + enable = virXPathString("string(./@enable)", ctxt); >> + if (enable) { >> + def->enable = virTristateBoolTypeFromString(enable); >> + if (def->enable <= 0) { >> + virReportError(VIR_ERR_XML_ERROR, >> + _("Invalid dns enable setting '%s' " >> + "in network '%s'"), >> + enable, networkName); >> + goto cleanup; >> + } >> + } >> + >> forwardPlainNames = virXPathString("string(./@forwardPlainNames)", ctxt); >> if (forwardPlainNames) { >> def->forwardPlainNames = virTristateBoolTypeFromString(forwardPlainNames); >> @@ -1440,6 +1453,7 @@ virNetworkDNSDefParseXML(const char *networkName, >> >> ret = 0; >> cleanup: >> + VIR_FREE(enable); >> VIR_FREE(forwardPlainNames); >> VIR_FREE(fwdNodes); >> VIR_FREE(hostNodes); >> @@ -2496,12 +2510,22 @@ virNetworkDNSDefFormat(virBufferPtr buf, >> { >> size_t i, j; >> >> - if (!(def->forwardPlainNames || def->nfwds || def->nhosts || >> + if (!(def->enable || def->forwardPlainNames || def->nfwds || def->nhosts || >> def->nsrvs || def->ntxts)) >> return 0; >> >> virBufferAddLit(buf, "<dns"); >> - /* default to "yes", but don't format it in the XML */ >> + if (def->enable) { >> + const char *fwd = virTristateBoolTypeToString(def->enable); >> + >> + if (!fwd) { >> + virReportError(VIR_ERR_INTERNAL_ERROR, >> + _("Unknown enable type %d in network"), >> + def->enable); >> + return -1; > > I don't think check is needed. We've validated the forward mode when > parsing the XML. > Depends on your philosophy. If you trust that nothing could have modified the value from the time it was parsed until the time it's formated, then yes. If you don't trust that (or are uncomfortable that a pointer from a function that could potentially return NULL is used as an argument to an sprintf-like function without checking for NULL), then no.

Libvirt code disagrees about this a lot, even within code written by the same person (e.g. me). These days I tend more towards the "defensive programming" style of checking everything unless something in the direct call chain guarantees that it has been validated.

> Also, I think that we need slightly different approach here. I mean, for > "<dns enable='no'/>" case we just want to put that string into XML and > nothing more. With this code I'm able to get the following which makes > not much sense to me: > > <dns enable='no'> > <txt name='example' value='example value'/> > </dns> I thought about that before making it this way. The idea is to allow temporarily turning off the DNS server without losing all the config. I'm not married to that approach though (especially since we don't do that for other configures AFAIK). So I can modify the behavior.