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

[Michal Privoznik]

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?

> +        <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.

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>


> +        }
> +        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