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

[Laine Stump]

On 12/23/2010 12:41 PM, Eric Blake wrote:
> 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!

Heh. Truthfully I was scared you'd NAK it if I didn't update the docs :-P


>>   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?

I had somehow convinced myself not (because 
networkAddGeneralIp6tablesRules() undoes itself if it fails), but of 
course that logic is wrong - it's the *previous* step that needs 
undoing, so you are absolutely correct. I've squashed in the appropriate 
call.

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

Yup.

>> @@ -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/

Okay.

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

Sure. I almost did that originally, and can't say why I didn't (I guess 
my left brain liked the symmetry of the names or something).

> ACK with those nits addressed.
>