[libvirt] [PATCHv2 12/13] Turn on IPv6 support in the bridge_driver.c virtual network driver

[Eric Blake]

On 12/22/2010 11:58 AM, Laine Stump wrote:
> At this point everything is already in place to make IPv6 happen, we just
> need to add a few rules, remove some checks for IPv4-only, and document
> the changes to the XML on the website.
> ---
> No changes from V1.
> 
>  docs/formatnetwork.html.in  |   35 +++++++--

Yeah - a patch series with documentation updates!

>  static int
>  networkAddGeneralIptablesRules(struct network_driver *driver,
>                                 virNetworkObjPtr network)
> @@ -926,6 +985,11 @@ networkAddGeneralIptablesRules(struct network_driver *driver,
>          goto err8;
>      }
>  
> +    /* add IPv6 general rules, if needed */
> +    if (networkAddGeneralIp6tablesRules(driver, network) < 0) {
> +        goto err8;

Should this be err9, with a step that undoes the previous action when
you get past the err8 failure point?

> +        if (virAsprintf(&field, SYSCTL_PATH "/net/ipv6/conf/%s/disable_ipv6",
> +                        network->def->bridge) < 0) {

...
> +        if (virFileWriteStr(field, "1", 0) < 0) {
> +            virReportSystemError(errno,
> +                                 _("cannot enable %s"), field);

Misleading message; maybe "cannot write to %s to disable IPv6".

> @@ -1755,7 +1845,8 @@ cleanup:
>  static int networkUndefine(virNetworkPtr net) {
>      struct network_driver *driver = net->conn->networkPrivateData;
>      virNetworkObjPtr network;
> -    virNetworkIpDefPtr ipv4def;
> +    virNetworkIpDefPtr ipdef;
> +    int v4present = 0, v6present = 0;

s/int/bool/

> @@ -1780,12 +1871,17 @@ static int networkUndefine(virNetworkPtr net) {
>  
>      /* we only support dhcp on one IPv4 address per defined network */
>      for (ii = 0;
> -         (ipv4def = virNetworkDefGetIpByIndex(network->def, AF_INET, ii));
> +         (ipdef = virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, ii));
>           ii++) {
> -        if (ipv4def->nranges || ipv4def->nhosts)
> -            break;
> +        if (VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET)) {
> +            if (ipdef->nranges || ipdef->nhosts)
> +                v4present = 1;

At first glance, this logic didn't sound right.  You only set v4present
if you found a dhcp interface, ignoring other ipv4 interfaces.  Then
again,...

> +        } else if (VIR_SOCKET_IS_FAMILY(&ipdef->address, AF_INET6)) {
> +            v6present = 1;
> +        }
>      }
> -    if (ipv4def) {
> +
> +    if (v4present) {
>          dnsmasqContext *dctx = dnsmasqContextNew(network->def->name, DNSMASQ_STATE_DIR);

you only use it to disable dnsmasq rather than all things related to
IPv4, so maybe it would be better to rename the variable to dhcp_present
instead of v4present.

ACK with those nits addressed.

-- 
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/20101223/cc8eebd9/attachment-0001.sig>