[libvirt] [PATCH 2/3] network: allow disabling dnsmasq's DNS server
Laine Stump
laine at laine.org
Thu Aug 18 18:42:51 UTC 2016
On Aug 18, 2016 5:01 AM, "Michal Privoznik" <mprivozn at redhat.com> 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><dns></code> will be ignored).
> > + If <code>enable</code> is "yes" or unspecified (including
> > + the complete absence of any <code><dns></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 mor 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
>
>
> > + }
> > + virBufferAsprintf(buf, " enable='%s'", fwd);
> > + }
> > if (def->forwardPlainNames) {
> > const char *fwd =
virTristateBoolTypeToString(def->forwardPlainNames);
> >
>
> The rest of the patch looks okay. ACK if you fix the XML formatting issue.
>
> Michal
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20160818/bc64adfa/attachment-0001.htm>
More information about the libvir-list
mailing list