[libvirt] [PATCHv3] handle DNS over IPv6

[Eric Blake]

On 01/12/2011 02:24 PM, Paweł Krześniak wrote:
> 2011/1/7 Daniel P. Berrange <berrange at redhat.com>:
>> Practically no apps
>> will be do this and so they typically can't make use of
>> the link-local address.
> 
> I agree.
> v2 of patch attached.

Can you convince your mailer to send patches with mime type text/plain,
rather than base64 encoded as application/octet-stream?  Inline review
is a lot nicer.  That said:

> commit ce44728e232a39ff42bf87f5eb102bc76b71241d
> Author: Pawel Krzesniak <pawel.krzesniak at gmail.com>
> Date:   Tue Jan 11 23:36:19 2011 +0100
> 
>     bridge_driver: handle DNS over IPv6
>     * dnsmasq listens on all defined IPv[46] addresses for network
>     * Add ip6tables rules to allow DNS traffic to host
> ---
>  src/network/bridge_driver.c |   59 ++++++++++++++++++++++++++++++++++--------
>  1 files changed, 47 insertions(+), 12 deletions(-)
> 
> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
> index 4c64a74..d5524db 100644
> --- a/src/network/bridge_driver.c
> +++ b/src/network/bridge_driver.c
> @@ -423,7 +423,7 @@ networkSaveDnsmasqHostsfile(virNetworkIpDefPtr ipdef,
>      return 0;
>  }
>  
> -
> +#define FAMILIES_COUNT 2

Rather than defining this,

>  static int
>  networkBuildDnsmasqArgv(virNetworkObjPtr network,
>                          virNetworkIpDefPtr ipdef,
> @@ -431,7 +431,11 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
>                          virCommandPtr cmd) {
>      int r, ret = -1;
>      int nbleases = 0;
> -    char *bridgeaddr;
> +    char *bridgeaddr = NULL;
> +    char *ipaddr = NULL;
> +    int ii, jj;
> +    virNetworkIpDefPtr tmpipdef;
> +    int families[FAMILIES_COUNT] = { AF_INET, AF_INET6 };

just declare this as int families[] = { ... };

>  
> +    /*
> +     * --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; ii < FAMILIES_COUNT; ii++) {

and iterate while ii < ARRAY_CARDINALITY(families).

One less thing to screw up if we ever add a third family.

> @@ -1027,9 +1040,31 @@ networkAddGeneralIp6tablesRules(struct network_driver *driver,
>          goto err3;
>      }
>  
> +    /* allow DNS over IPv6 */
> +    if (iptablesAddTcpInput(driver->iptables, AF_INET6,
> +                            network->def->bridge, 53) < 0) {
> +        networkReportError(VIR_ERR_SYSTEM_ERROR,
> +                           _("failed to add ip6tables rule to allow DNS requests from '%s'"),
> +                           network->def->bridge);
> +        goto err4;
> +    }
> +
> +    if (iptablesAddUdpInput(driver->iptables, AF_INET6,
> +                            network->def->bridge, 53) < 0) {
> +        networkReportError(VIR_ERR_SYSTEM_ERROR,
> +                           _("failed to add ip6tables rule to allow DNS requests from '%s'"),
> +                           network->def->bridge);
> +        goto err5;
> +    }

Is it worth a (separate) clean-up patch that provides
iptablesAddTcpUdpInput, which adds a rules for both protocol types in
one shot, rather than requiring every call site to duplicate the calls
for tcp and udp?

The patch looks reasonable to me from the coding standpoint, but is
probably worth a v4 for my nits, and you may want feedback from someone
more familiar with the networking standpoint.

-- 
Eric Blake   eblake at redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 619 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20110112/d3a0107e/attachment-0001.sig>