Re: [libvirt] [PATCHv3 2/3] v8.2 add support for DHCPv6

On 12/04/2012 03:03 PM, Laine Stump wrote: > On 12/03/2012 11:13 AM, Gene Czarcinski wrote: >> The DHCPv6 support includes IPV6 dhcp-range and dhcp-host for one >> IPv6 subnetwork on one interface. This support will only work >> if dnsmasq version >= 2.64; otherwise an error occurs if >> dhcp-range or dhcp-host is specified. >> >> Essentially, this change provides the same DHCP support for IPv6 >> that has been available for IPv4. >> >> With dnsmasq >= 2.64, support for the RA service is now provided >> by dnsmasq (radvd is no longer used/started). >> >> Dnsmasq 2.64 does contain the bugfixes released to DHCPv6 and >> dnsmasq's handling of Router Advertisement. >> >> Documentation has been updated to reflect the new support. >> >> The network schema has been updated to reflect the new support. >> --- >> docs/formatnetwork.html.in | 108 +++++- >> docs/schemas/network.rng | 12 +- >> src/conf/network_conf.c | 100 +++-- >> src/network/bridge_driver.c | 427 >> +++++++++++++++------ >> src/util/dnsmasq.c | 9 +- >> tests/networkxml2argvdata/dhcp6-nat-network.argv | 15 + >> tests/networkxml2argvdata/dhcp6-nat-network.xml | 24 ++ >> tests/networkxml2argvdata/dhcp6-network.argv | 15 + >> tests/networkxml2argvdata/dhcp6-network.xml | 14 + >> .../dhcp6host-routed-network.argv | 13 + >> .../dhcp6host-routed-network.xml | 19 + >> tests/networkxml2argvdata/isolated-network.argv | 16 +- >> .../networkxml2argvdata/nat-network-dns-hosts.argv | 13 +- >> .../nat-network-dns-srv-record-minimal.argv | 9 +- >> .../nat-network-dns-srv-record.argv | 9 +- >> .../nat-network-dns-txt-record.argv | 10 +- >> tests/networkxml2argvdata/nat-network.argv | 17 +- >> tests/networkxml2argvdata/netboot-network.argv | 23 +- >> .../networkxml2argvdata/netboot-proxy-network.argv | 19 +- >> tests/networkxml2argvdata/routed-network.argv | 10 +- >> tests/networkxml2argvtest.c | 7 +- >> 21 files changed, 674 insertions(+), 215 deletions(-) >> create mode 100644 tests/networkxml2argvdata/dhcp6-nat-network.argv >> create mode 100644 tests/networkxml2argvdata/dhcp6-nat-network.xml >> create mode 100644 tests/networkxml2argvdata/dhcp6-network.argv >> create mode 100644 tests/networkxml2argvdata/dhcp6-network.xml >> create mode 100644 >> tests/networkxml2argvdata/dhcp6host-routed-network.argv >> create mode 100644 >> tests/networkxml2argvdata/dhcp6host-routed-network.xml >> >> diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in >> index a3a5ced..a5f0dc7 100644 >> --- a/docs/formatnetwork.html.in >> +++ b/docs/formatnetwork.html.in >> @@ -583,8 +583,10 @@ >> dotted-decimal format, or an IPv6 address in standard >> colon-separated hexadecimal format, that will be >> configured on >> the bridge >> - device associated with the virtual network. To the guests this >> - address will be their default route. For IPv4 addresses, >> the <code>netmask</code> >> + device associated with the virtual network. To the guests >> this IPv4 >> + address will be their IPv4 default route. For IPv6, the >> default route is >> + established via Router Advertisement. >> + For IPv4 addresses, the <code>netmask</code> >> attribute defines the significant bits of the network >> address, >> again specified in dotted-decimal format. For IPv6 >> addresses, >> and as an alternate method for IPv4 addresses, you can >> specify >> @@ -593,10 +595,13 @@ >> could also be given as <code>prefix='24'</code>. The >> <code>family</code> >> attribute is used to specify the type of address - 'ipv4' >> or 'ipv6'; if no >> <code>family</code> is given, 'ipv4' is assumed. A network >> can have more than >> - one of each family of address defined, but only a single >> address can have a >> - <code>dhcp</code> or <code>tftp</code> element. <span >> class="since">Since 0.3.0; >> + one of each family of address defined, but only a single >> IPv4 address can have a >> + <code>dhcp</code> or <code>tftp</code> element. <span >> class="since">Since 0.3.0 </span> >> IPv6, multiple addresses on a single network, >> <code>family</code>, and >> - <code>prefix</code> since 0.8.7</span> >> + <code>prefix</code>. <span class="since">Since >> 0.8.7</span> In addition >> + to the one IPv4 address which has a <code>dhcp</code> >> definition, one IPv6 >> + address can have a <code>dhcp</code> definition. >> + <span class="since"> Since 1.0.1</span> >> <dl> >> <dt><code>tftp</code></dt> >> <dd>Immediately within >> @@ -617,27 +622,46 @@ >> <code>dhcp</code> element is not supported for IPv6, and >> is only supported on a single IP address per network >> for IPv4. >> <span class="since">Since 0.3.0</span> >> + The <code>dhcp</code> element is now supported for IPv6. >> + Again, there is a restriction that only one IPv6 >> address definition >> + is able to have a <code>dhcp</code> element. >> + <span class="since">Since 1.0.1</span> >> <dl> >> <dt><code>range</code></dt> >> <dd>The <code>start</code> and <code>end</code> >> attributes on the >> <code>range</code> element specify the boundaries >> of a pool of >> - IPv4 addresses to be provided to DHCP clients. >> These two addresses >> + addresses to be provided to DHCP clients. These two >> addresses >> must lie within the scope of the network defined >> on the parent >> - <code>ip</code> element. <span class="since">Since >> 0.3.0</span> >> + <code>ip</code> element. There may be zero or more >> + <code>range</code> elements specified. >> + <span class="since">Since 0.3.0</span> >> + <code>Range</code> can be specified for one IPv4 >> address, >> + one IPv6 address, or both. <span >> class="since">Since 1.0.1</span> >> </dd> >> <dt><code>host</code></dt> >> <dd>Within the <code>dhcp</code> element there may >> be zero or more >> - <code>host</code> elements; these specify hosts >> which will be given >> + <code>host</code> elements. These specify hosts >> which will be given >> names and predefined IP addresses by the built-in >> DHCP server. Any >> - such element must specify the MAC address of the >> host to be assigned >> + such IPv4 element must specify the MAC address of >> the host to be assigned >> a given name (via the <code>mac</code> attribute), >> the IP to be >> assigned to that host (via the <code>ip</code> >> attribute), and the >> name to be given that host by the DHCP server (via >> the >> <code>name</code> attribute). <span >> class="since">Since 0.4.5</span> >> + Within the IPv6 <code>dhcp</code> element zero or more >> + <code>host</code> elements are now supported. The >> definition for >> + an IPv6 <code>host</code> element differs from that >> for IPv4: >> + there is no <code>mac</code> attribute since a MAC >> address has no >> + defined meaning in IPv6. Instead, the >> <code>name</code> attribute is >> + used to identify the host to be assigned the IPv6 >> address. For DHCPv6, >> + the name is the plain name of the client host sent >> by the >> + client to the server. Note that this method of >> assigning a >> + specific IP address can be used instead of the >> <code>mac</code> >> + attribute for IPv4. <span class="since">Since >> 1.0.1</span> >> </dd> >> <dt><code>bootp</code></dt> >> <dd>The optional <code>bootp</code> >> - element specifies BOOTP options to be provided by >> the DHCP server. >> + element specifies BOOTP options to be provided by >> the DHCP >> + server for IPv4 only. >> Two attributes are supported: <code>file</code> is >> mandatory and >> gives the file to be used for the boot image; >> <code>server</code> is >> optional and gives the address of the TFTP server >> from which the boot >> @@ -680,6 +704,29 @@ >> &lt;ip family="ipv6" address="2001:db8:ca2:2::1" >> prefix="64" /&gt; >> &lt;/network&gt;</pre> >> + >> + <p> >> + Below is a variation of the above example which adds an IPv6 >> + dhcp range definition. >> + </p> >> + >> + <pre> >> + &lt;network&gt; >> + &lt;name&gt;default6&lt;/name&gt; >> + &lt;bridge name="virbr0" /&gt; >> + &lt;forward mode="nat"/&gt; >> + &lt;ip address="192.168.122.1" netmask="255.255.255.0"&gt; >> + &lt;dhcp&gt; >> + &lt;range start="192.168.122.2" end="192.168.122.254" >> /&gt; >> + &lt;/dhcp&gt; >> + &lt;/ip&gt; >> + &lt;ip family="ipv6" address="2001:db8:ca2:2::1" >> prefix="64" &gt; >> + &lt;dhcp&gt; >> + &lt;range start="2001:db8:ca2:2:1::10" >> end="2001:db8:ca2:2:1::ff" /&gt; >> + &lt;/dhcp&gt; >> + &lt;/ip&gt; >> + &lt;/network&gt;</pre> >> + >> <h3><a name="examplesRoute">Routed network config</a></h3> >> <p> >> @@ -704,6 +751,29 @@ >> &lt;ip family="ipv6" address="2001:db8:ca2:2::1" >> prefix="64" /&gt; >> &lt;/network&gt;</pre> >> + <p> >> + Below is another IPv6 varition. Instead of a dhcp range being >> + specified, this example has a couple of IPv6 host definitions. >> + </p> >> + >> + <pre> >> + &lt;network&gt; >> + &lt;name&gt;local6&lt;/name&gt; >> + &lt;bridge name="virbr1" /&gt; >> + &lt;forward mode="route" dev="eth1"/&gt; >> + &lt;ip address="192.168.122.1" netmask="255.255.255.0"&gt; >> + &lt;dhcp&gt; >> + &lt;range start="192.168.122.2" end="192.168.122.254" >> /&gt; >> + &lt;/dhcp&gt; >> + &lt;/ip&gt; >> + &lt;ip family="ipv6" address="2001:db8:ca2:2::1" >> prefix="64" &gt; >> + &lt;dhcp&gt; >> + &lt;host name="paul" ip="2001:db8:ca2:2:3::1" /&gt; >> + &lt;host name="bob" ip="2001:db8:ca2:2:3::2" /&gt; >> + &lt;/dhcp&gt; >> + &lt;/ip&gt; >> + &lt;/network&gt;</pre> >> + >> <h3><a name="examplesPrivate">Isolated network config</a></h3> >> <p> >> @@ -726,6 +796,24 @@ >> &lt;ip family="ipv6" address="2001:db8:ca2:3::1" >> prefix="64" /&gt; >> &lt;/network&gt;</pre> >> + <h3><a name="examplesPrivate6">Isolated IPv6 network >> config</a></h3> >> + >> + <p> >> + This variation of an isolated network defines only IPv6. >> + </p> >> + >> + <pre> >> + &lt;network&gt; >> + &lt;name&gt;sixnet&lt;/name&gt; >> + &lt;bridge name="virbr6" /&gt; >> + &lt;ip family="ipv6" address="2001:db8:ca2:6::1" >> prefix="64" &gt; >> + &lt;dhcp&gt; >> + &lt;host name="peter" ip="2001:db8:ca2:6:6::1" /&gt; >> + &lt;host name="dariusz" ip="2001:db8:ca2:6:6::2" /&gt; >> + &lt;/dhcp&gt; >> + &lt;/ip&gt; >> + &lt;/network&gt;</pre> >> + >> <h3><a name="examplesBridge">Using an existing host >> bridge</a></h3> >> <p> >> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng >> index 0d67f7f..09d7c73 100644 >> --- a/docs/schemas/network.rng >> +++ b/docs/schemas/network.rng >> @@ -218,7 +218,7 @@ >> </zeroOrMore> >> <zeroOrMore> >> <element name="host"> >> - <attribute name="ip"><ref >> name="ipv4Addr"/></attribute> >> + <attribute name="ip"><ref >> name="ipAddr"/></attribute> >> <oneOrMore> >> <element name="hostname"><ref >> name="dnsName"/></element> >> </oneOrMore> >> @@ -272,15 +272,17 @@ >> <element name="dhcp"> >> <zeroOrMore> >> <element name="range"> >> - <attribute name="start"><ref >> name="ipv4Addr"/></attribute> >> - <attribute name="end"><ref >> name="ipv4Addr"/></attribute> >> + <attribute name="start"><ref >> name="ipAddr"/></attribute> >> + <attribute name="end"><ref >> name="ipAddr"/></attribute> >> </element> >> </zeroOrMore> >> <zeroOrMore> >> <element name="host"> >> - <attribute name="mac"><ref >> name="uniMacAddr"/></attribute> >> + <optional> >> + <attribute name="mac"><ref >> name="uniMacAddr"/></attribute> >> + </optional> >> <attribute name="name"><text/></attribute> >> - <attribute name="ip"><ref >> name="ipv4Addr"/></attribute> >> + <attribute name="ip"><ref >> name="ipAddr"/></attribute> >> </element> >> </zeroOrMore> >> <optional> >> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c >> index 3f9e13c..ad6d0e1 100644 >> --- a/src/conf/network_conf.c >> +++ b/src/conf/network_conf.c >> @@ -633,6 +633,7 @@ cleanup: >> static int >> virNetworkDHCPHostDefParse(const char *networkName, >> + virNetworkIpDefPtr def, > I might have just passed in the family rather than the entire ipdef, > just to make sure that nobody accidentally modified def instead of > host->ip. Going that far isn't necessary, but we at least need to make > it "const virNetworkIpDefPtr def". Excellent point. When you first code something you are a little close to the subject and can miss something like this which is easily caught by another set of eyes.

> >> xmlNodePtr node, >> virNetworkDHCPHostDefPtr host, >> bool partialOkay) >> @@ -644,6 +645,13 @@ virNetworkDHCPHostDefParse(const char >> *networkName, >> mac = virXMLPropString(node, "mac"); >> if (mac != NULL) { >> + if (VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET6)) { >> + virReportError(VIR_ERR_XML_ERROR, >> + _("Invalid to specify MAC address '%s' " >> + "in IPv6 network '%s'"), > "in network '%s' IPv6 static host definition." Yup! > >> + mac, networkName); >> + goto cleanup; >> + } >> if (virMacAddrParse(mac, &addr) < 0) { >> virReportError(VIR_ERR_XML_ERROR, >> _("Cannot parse MAC address '%s' in >> network '%s'"), >> @@ -686,10 +694,19 @@ virNetworkDHCPHostDefParse(const char >> *networkName, >> networkName); >> } >> } else { > Out of curiousity: have you tried doing virsh net-update ip-dhcp-host > ... for an IPv6 dhcp entry? (that's one of the callers of this function) I have not yet but I will. > > >> + if (VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET6)) { >> + if (!name) { >> + virReportError(VIR_ERR_XML_ERROR, >> + _("Static host definition in IPv6 >> network '%s' " >> + "must have name attribute"), >> + networkName); >> + goto cleanup; >> + } >> + } >> /* normal usage - you need at least one MAC address or one >> host name */ >> - if (!(mac || name)) { >> + else if (!(mac || name)) { > The else must be on the same line as the closing brace of the preceding > if clause (put the comment up above if "if (VIR_SOCKET_ADDR...)" and > expend it to describe what's needed for IPv6 as well). I did not understand what you were saying at first ... until I looked at you patch where is was very clear .. good! > >> virReportError(VIR_ERR_XML_ERROR, >> - _("Static host definition in network '%s' " >> + _("Static host definition in IPv4 >> network '%s' " >> "must have mac or name attribute"), >> networkName); >> goto cleanup; >> @@ -748,36 +765,39 @@ virNetworkDHCPDefParse(const char *networkName, >> virReportOOMError(); >> return -1; >> } >> - if (virNetworkDHCPHostDefParse(networkName, cur, >> + if (virNetworkDHCPHostDefParse(networkName, def, cur, >> &def->hosts[def->nhosts], >> false) < 0) { >> return -1; >> } >> def->nhosts++; >> - } else if (cur->type == XML_ELEMENT_NODE && >> - xmlStrEqual(cur->name, BAD_CAST "bootp")) { >> - char *file; >> - char *server; >> - virSocketAddr inaddr; >> - memset(&inaddr, 0, sizeof(inaddr)); >> - >> - if (!(file = virXMLPropString(cur, "file"))) { >> - cur = cur->next; >> - continue; >> - } >> - server = virXMLPropString(cur, "server"); >> + } else if (VIR_SOCKET_ADDR_IS_FAMILY(&def->address, >> AF_INET)) { >> + /* the following only applies to IPv4 */ >> + if (cur->type == XML_ELEMENT_NODE && >> + xmlStrEqual(cur->name, BAD_CAST "bootp")) { > You don't need to add the extra nesting - just add the > VIR_SOCKET_ADDR... as another term of the else if expression (i.e. > combine the two): > > } else if (VIR_SOCKET_ADDR_IS_FAMILY(&def->address, > AF_INET) && > cur->type == XML_ELEMENT_NODE && > xmlStrEqual(cur->name, BAD_CAST "bootp")) { > > > Not only is the code simpler, but then the body of the else isn't > re-indented, so it doesn't create a strange diff that needs to be > hand-examined :-) simpler is always better! > >> + char *file; >> + char *server; >> + virSocketAddr inaddr; >> + memset(&inaddr, 0, sizeof(inaddr)); >> + >> + if (!(file = virXMLPropString(cur, "file"))) { >> + cur = cur->next; >> + continue; >> + } >> + server = virXMLPropString(cur, "server"); >> + >> + if (server && >> + virSocketAddrParse(&inaddr, server, AF_UNSPEC) >> < 0) { >> + VIR_FREE(file); >> + VIR_FREE(server); >> + return -1; >> + } >> - if (server && >> - virSocketAddrParse(&inaddr, server, AF_UNSPEC) < 0) { >> - VIR_FREE(file); >> + def->bootfile = file; >> + def->bootserver = inaddr; >> VIR_FREE(server); >> - return -1; >> } >> - >> - def->bootfile = file; >> - def->bootserver = inaddr; >> - VIR_FREE(server); >> } >> cur = cur->next; >> @@ -1139,6 +1159,20 @@ virNetworkIPParseXML(const char *networkName, >> } >> } >> + if (VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET6)) { >> + /* parse IPv6-related info */ >> + cur = node->children; >> + while (cur != NULL) { >> + if (cur->type == XML_ELEMENT_NODE && >> + xmlStrEqual(cur->name, BAD_CAST "dhcp")) { >> + result = virNetworkDHCPDefParse(networkName, def, >> cur); >> + if (result) >> + goto error; >> + } >> + cur = cur->next; >> + } >> + } >> + > Rather than adding an entirely new loop for IPv6 (which is doing a > perfect subset of what's done for IPv4), just move the "if IPv4" > qualifier into the bit of the existing loop that is no IPv4-only. For > that matter, it's probably worth checking that somebody doesn't try to > specify a <tftp> node for an IPv6 network (hmm, I assume PXE boot can be > done with dhcp6 and IPv6. What would it take to make that work too?) OK, I had to do a little research here while I was writing the code. The bottom line is that there is currently no IPv6 PXE. Apparently PXE is "owned" by Intel and IETF does not want to go there. Personally, I believe that remote boot is important. There are some high security environments were the only way to do things is with disk-less workstations. I believe there are still some efforts to get remote boot into IPv6.

I see the change in your patch does just what I thought was needed when I read your comment above. > >> result = 0; >> error: >> @@ -2361,11 +2395,9 @@ virNetworkIpDefByIndex(virNetworkDefPtr def, >> int parentIndex) >> /* first find which ip element's dhcp host list to work on */ >> if (parentIndex >= 0) { >> ipdef = virNetworkDefGetIpByIndex(def, AF_UNSPEC, >> parentIndex); >> - if (!(ipdef && >> - VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET))) { >> + if (!(ipdef)) { >> virReportError(VIR_ERR_OPERATION_INVALID, >> _("couldn't update dhcp host entry - " >> - "no <ip family='ipv4'> " >> "element found at index %d in network >> '%s'"), >> parentIndex, def->name); > You accidentally took out the "no <ip>". That was not an accident. There is no longer a test for just IPv4.

> > >> } >> @@ -2378,17 +2410,17 @@ virNetworkIpDefByIndex(virNetworkDefPtr def, >> int parentIndex) >> for (ii = 0; >> (ipdef = virNetworkDefGetIpByIndex(def, AF_UNSPEC, ii)); >> ii++) { >> - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET) && >> - (ipdef->nranges || ipdef->nhosts)) { >> + if (ipdef->nranges || ipdef->nhosts) >> break; >> - } >> } >> - if (!ipdef) >> + if (!ipdef) { >> ipdef = virNetworkDefGetIpByIndex(def, AF_INET, 0); >> + if (!ipdef) >> + ipdef = virNetworkDefGetIpByIndex(def, AF_INET6, 0); >> + } >> if (!ipdef) { >> virReportError(VIR_ERR_OPERATION_INVALID, >> _("couldn't update dhcp host entry - " >> - "no <ip family='ipv4'> " >> "element found in network '%s'"), >> def->name); > And again. The same answer as above ... not an accident. Or it could be expanded to say IPv4 or IPv6 but I thought just removing the IPv4 part was simpler.

> >> } >> return ipdef; >> @@ -2418,7 +2450,7 @@ virNetworkDefUpdateIPDHCPHost(virNetworkDefPtr >> def, >> /* parse the xml into a virNetworkDHCPHostDef */ >> if (command == VIR_NETWORK_UPDATE_COMMAND_MODIFY) { >> - if (virNetworkDHCPHostDefParse(def->name, ctxt->node, >> &host, false) < 0) >> + if (virNetworkDHCPHostDefParse(def->name, ipdef, >> ctxt->node, &host, false) < 0) >> goto cleanup; >> /* search for the entry with this (mac|name), >> @@ -2451,7 +2483,7 @@ virNetworkDefUpdateIPDHCPHost(virNetworkDefPtr >> def, >> } else if ((command == VIR_NETWORK_UPDATE_COMMAND_ADD_FIRST) || >> (command == VIR_NETWORK_UPDATE_COMMAND_ADD_LAST)) { >> - if (virNetworkDHCPHostDefParse(def->name, ctxt->node, >> &host, true) < 0) >> + if (virNetworkDHCPHostDefParse(def->name, ipdef, >> ctxt->node, &host, true) < 0) >> goto cleanup; >> /* log error if an entry with same name/address/ip >> already exists */ >> @@ -2497,7 +2529,7 @@ virNetworkDefUpdateIPDHCPHost(virNetworkDefPtr >> def, >> } else if (command == VIR_NETWORK_UPDATE_COMMAND_DELETE) { >> - if (virNetworkDHCPHostDefParse(def->name, ctxt->node, >> &host, false) < 0) >> + if (virNetworkDHCPHostDefParse(def->name, ipdef, >> ctxt->node, &host, false) < 0) >> goto cleanup; >> /* find matching entry - all specified attributes must >> match */ >> diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c >> index cb2997d..c07d61a 100644 >> --- a/src/network/bridge_driver.c >> +++ b/src/network/bridge_driver.c >> @@ -75,6 +75,11 @@ >> #define VIR_FROM_THIS VIR_FROM_NETWORK >> +#define CHECK_VERSION_DHCP(CAPS) \ >> + (dnsmasqCapsGetVersion(CAPS) >= 2064000) >> +#define CHECK_VERSION_RA(CAPS) \ >> + (dnsmasqCapsGetVersion(CAPS) >= 2064000) > (I originally thought both of these version numbers should be > encapsulated as flags in dnsmasqCaps, rather than exposing the version > number here (there are, several examples of this in > qemu_capabilities.c), but fully removing mention of the version number > from bridge_driver.c would also require getting the version number for > the error log messages from somewhere too, so I decided against that. > However, these macros *do* need to get their version info from the same > source as the log messsages. So I suggest something like this: > > #define DNSMASQ_DHCPv6_MAJOR_REQD 2 > #define DNSMASQ_DHCPv6_MINOR_REQD 64 > #define DNSMQASQ_RA_MAJOR_REQD 2 > #define DNSMSAQ_RA_MINOR_REQD 64 > > #define DNSMASQ_DHCPv6_SUPPORT(CAPS) \ > (dnsmasqCapsGetVersion(CAPS) >= > (DNSMASQ_DHCPv6_MAJOR_REQD * 1000000) + \ > > (DNSMASQ_DHCPv6_MINOR_REQD * 1000)) > #define DNSMASQ_RA_SUPPORT(CAPS) \ > (dnsmasqCapsGetVersion(CAPS) >= > (DNSMASQ_RA_MAJOR_REQD * 1000000) + \ > (DNSMASQ_RA_MINOR_REQD * 1000)) > > Then use the XXX_REQD in the error logs. That way if we ever needed > to change it for some reason, it would just need changing in a single > place. > > (actually, now that I think about it, the above #defines should be > moved into dnsmasq.h) This works for me. You have a much better idea of the big picture and where stuff like this should go. I just wish there was a better way than using the version number.

I am just overjoyed that dnsmasq 2.64 should be out tonight and that the problem with RA was found and fixed.

I wonder if the dnsmasq maintainer who so quickly updated 2.59 to 2.63 will be willing to do another update to 2.64?

> >> + >> /* Main driver state */ >> struct network_driver { >> virMutex lock; >> @@ -588,20 +593,32 @@ cleanup: >> return ret; >> } >> + /* the following does not build a file, it builds a list >> + * which is later saved into a file >> + */ >> + >> static int >> -networkBuildDnsmasqHostsfile(dnsmasqContext *dctx, >> - virNetworkIpDefPtr ipdef, >> - virNetworkDNSDefPtr dnsdef) >> +networkBuildDnsmasqDhcpHostsList(dnsmasqContext *dctx, >> + virNetworkIpDefPtr ipdef) >> { >> - unsigned int i, j; >> + unsigned int i; >> for (i = 0; i < ipdef->nhosts; i++) { >> virNetworkDHCPHostDefPtr host = &(ipdef->hosts[i]); >> - if ((host->mac) && VIR_SOCKET_ADDR_VALID(&host->ip)) >> + if (VIR_SOCKET_ADDR_VALID(&host->ip)) >> if (dnsmasqAddDhcpHost(dctx, host->mac, &host->ip, >> host->name) < 0) >> return -1; >> } >> + return 0; >> +} >> + >> +static int >> +networkBuildDnsmasqHostsList(dnsmasqContext *dctx, >> + virNetworkDNSDefPtr dnsdef) >> +{ >> + unsigned int i, j; >> + >> if (dnsdef) { >> for (i = 0; i < dnsdef->nhosts; i++) { >> virNetworkDNSHostsDefPtr host = &(dnsdef->hosts[i]); >> @@ -619,7 +636,6 @@ networkBuildDnsmasqHostsfile(dnsmasqContext *dctx, >> static int >> networkBuildDnsmasqArgv(virNetworkObjPtr network, >> - virNetworkIpDefPtr ipdef, >> const char *pidfile, >> virCommandPtr cmd, >> dnsmasqContext *dctx, >> @@ -632,7 +648,8 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, >> char *recordPort = NULL; >> char *recordWeight = NULL; >> char *recordPriority = NULL; >> - virNetworkIpDefPtr tmpipdef; >> + virNetworkIpDefPtr tmpipdef, ipdef, ipv4def, ipv6def; >> + bool dhcp4flag, dhcp6flag, ipv6SLAAC; > It looks to me like dhcp4flag and dhcp6flag are exactly equivalent to > "ipvXdef != NULL", so they are redundant. They should be removed. I seem to remember that there was a difference at one time but the code as it exists today, both dhcp4flag and dhcp6flag are unnecessary and easily replaced by tests for ipv6def and/or ipv4def not being NULL.

> >> /* >> * NB, be careful about syntax for dnsmasq options in long >> format. >> @@ -657,14 +674,17 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, >> * Needed to ensure dnsmasq uses same algorithm for processing >> * multiple namedriver entries in /etc/resolv.conf as GLibC. >> */ >> - virCommandAddArg(cmd, "--strict-order"); >> + virCommandAddArgList(cmd, "--strict-order", >> + "--domain-needed", >> + NULL); >> - if (network->def->domain) >> + if (network->def->domain) { >> virCommandAddArgPair(cmd, "--domain", network->def->domain); >> + virCommandAddArg(cmd, "--expand-hosts"); >> + } >> /* need to specify local even if no domain specified */ >> virCommandAddArgFormat(cmd, "--local=/%s/", >> network->def->domain ? >> network->def->domain : ""); >> - virCommandAddArg(cmd, "--domain-needed"); >> if (pidfile) >> virCommandAddArgPair(cmd, "--pid-file", pidfile); >> @@ -687,7 +707,7 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, >> } else { >> virCommandAddArgList(cmd, >> "--bind-interfaces", >> - "--except-interface", "lo", >> + "--except-interface=lo", >> NULL); > All of the above movement of options seems to be unrelated to adding > support for DHCPv6; as a matter of fact, it all adds up to a NOP (when > combined with the removal of --expand-hosts further down in the file). > Since the next patch is about to replace all of this code anyway, and it > isn't necessary for DHCPv6 support, it should be eliminated from this > diff. Try to keep this patch to only the changes needed for supporting > DHCPv6. You are correct. > >> /* >> * --interface does not actually work with dnsmasq < 2.47, >> @@ -791,14 +811,75 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, >> } >> } >> - if (ipdef) { >> + /* Find the first dhcp for both IPv4 and IPv6 */ >> + for (ii = 0, ipv4def = NULL, ipv6def = NULL, >> + dhcp4flag = false, dhcp6flag = false, ipv6SLAAC = >> false; >> + (ipdef = virNetworkDefGetIpByIndex(network->def, >> AF_UNSPEC, ii)); >> + ii++) { >> + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET)) { >> + if (ipdef->nranges || ipdef->nhosts) { >> + if (ipv4def) { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("For IPv4, multiple DHCP definitions >> cannot " >> + "be specified.")); >> + goto cleanup; >> + } else { >> + ipv4def = ipdef; >> + dhcp4flag = true; >> + } >> + } >> + } >> + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) { >> + if (ipdef->nranges || ipdef->nhosts) { >> + if (!CHECK_VERSION_DHCP(caps)) { >> + unsigned long version = >> dnsmasqCapsGetVersion(caps); >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, >> + _("The version of dnsmasq on this host >> (%d.%d) doesn't " >> + "adequately support dhcp range or >> dhcp host " >> + "specification. Version 2.64 or >> later is required."), > "2.64" should be replaced with %d.%d, and the constants I suggested > above should be added to the args. OK. > >> + (int)version / 1000000, (int)(version % >> 1000000) / 1000); >> + goto cleanup; >> + } >> + if (ipv6def) { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("For IPv6, multiple DHCP definitions >> cannot " >> + "be specified.")); >> + goto cleanup; >> + } else { >> + ipv6def = ipdef; >> + dhcp6flag = true; >> + } >> + } else { >> + ipv6SLAAC = true; >> + } >> + } >> + } >> + >> + if (dhcp6flag && ipv6SLAAC) { >> + VIR_WARN("For IPv6, when DHCP is specified for one address, >> then " >> + "state-full Router Advertising will occur. The >> additional " >> + "IPv6 addresses specified require manually >> configured guest " >> + "network to work properly since both state-full >> (DHCP) " >> + "and state-less (SLAAC) addressing are not >> supported " >> + "on the same network interface."); >> + } >> + >> + if (ipv4def) >> + ipdef = ipv4def; >> + else >> + ipdef = ipv6def; > You could shorten this: > > ipdef = ipv4def ? ipv4def : ipv6def; > >> + >> + while (ipdef) { >> for (r = 0 ; r < ipdef->nranges ; r++) { >> char *saddr = >> virSocketAddrFormat(&ipdef->ranges[r].start); >> - if (!saddr) >> + if (!saddr) { >> + virReportOOMError(); > This error log should be removed - it's already done in > virSocketAddrFormat(). And remove the {} that you added while you're > at it. > >> goto cleanup; >> + } >> char *eaddr = virSocketAddrFormat(&ipdef->ranges[r].end); >> if (!eaddr) { >> VIR_FREE(saddr); >> + virReportOOMError(); > This one too. > >> goto cleanup; >> } >> virCommandAddArg(cmd, "--dhcp-range"); >> @@ -812,72 +893,110 @@ networkBuildDnsmasqArgv(virNetworkObjPtr >> network, >> /* >> * For static-only DHCP, i.e. with no range but at least >> one host element, >> * we have to add a special --dhcp-range option to enable >> the service in >> - * dnsmasq. >> + * dnsmasq. [this is for dhcp-hosts= support] > Really trivial, but () is more common around parenthetical comments than > [] :-) > >> */ >> if (!ipdef->nranges && ipdef->nhosts) { >> char *bridgeaddr = virSocketAddrFormat(&ipdef->address); >> - if (!bridgeaddr) >> + if (!bridgeaddr) { >> + virReportOOMError(); > Again - remove the virReportOOMError() and surrounding {} that you've > added. > >> goto cleanup; >> + } >> virCommandAddArg(cmd, "--dhcp-range"); >> virCommandAddArgFormat(cmd, "%s,static", bridgeaddr); >> VIR_FREE(bridgeaddr); >> } >> - if (ipdef->nranges > 0) { >> - char *leasefile = >> networkDnsmasqLeaseFileName(network->def->name); >> - if (!leasefile) >> - goto cleanup; >> - virCommandAddArgFormat(cmd, "--dhcp-leasefile=%s", >> leasefile); >> - VIR_FREE(leasefile); >> - virCommandAddArgFormat(cmd, "--dhcp-lease-max=%d", >> nbleases); >> - } >> - >> - if (ipdef->nranges || ipdef->nhosts) >> - virCommandAddArg(cmd, "--dhcp-no-override"); >> + if (networkBuildDnsmasqDhcpHostsList(dctx, ipdef) < 0) >> + goto cleanup; >> - /* add domain to any non-qualified hostnames in >> /etc/hosts or addn-hosts */ >> - if (network->def->domain) >> - virCommandAddArg(cmd, "--expand-hosts"); > Here's ^^^ another piece that's part of the NOP I mentioned above. > >> + /* Note: the following is IPv4 only */ >> + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET)) { >> + if (ipdef->nranges || ipdef->nhosts) >> + virCommandAddArg(cmd, "--dhcp-no-override"); >> - if (networkBuildDnsmasqHostsfile(dctx, ipdef, >> network->def->dns) < 0) >> - goto cleanup; >> + if (ipdef->tftproot) { >> + virCommandAddArgList(cmd, "--enable-tftp", >> + "--tftp-root", ipdef->tftproot, >> + NULL); >> + } >> - /* Even if there are currently no static hosts, if we're >> - * listening for DHCP, we should write a 0-length hosts >> - * file to allow for runtime additions. >> - */ >> - if (ipdef->nranges || ipdef->nhosts) >> - virCommandAddArgPair(cmd, "--dhcp-hostsfile", >> - dctx->hostsfile->path); >> + if (ipdef->bootfile) { >> + virCommandAddArg(cmd, "--dhcp-boot"); >> + if (VIR_SOCKET_ADDR_VALID(&ipdef->bootserver)) { >> + char *bootserver = >> virSocketAddrFormat(&ipdef->bootserver); >> - /* Likewise, always create this file and put it on the >> commandline, to allow for >> - * for runtime additions. >> - */ >> - virCommandAddArgPair(cmd, "--addn-hosts", >> - dctx->addnhostsfile->path); >> + if (!bootserver) { >> + virReportOOMError(); >> + goto cleanup; >> + } >> + virCommandAddArgFormat(cmd, "%s%s%s", >> + ipdef->bootfile, ",,", >> bootserver); >> + VIR_FREE(bootserver); >> + } else { >> + virCommandAddArg(cmd, ipdef->bootfile); >> + } >> + } >> + } >> + if (ipdef == ipv6def) >> + ipdef = NULL; >> + else >> + ipdef = ipv6def; > ipdef = (ipdef == ipv6def) ? NULL : ipv6def; > >> + } >> - if (ipdef->tftproot) { >> - virCommandAddArgList(cmd, "--enable-tftp", >> - "--tftp-root", ipdef->tftproot, >> - NULL); >> + if (nbleases > 0) { > Hmm. This reminded me that dnsmasq puts static hosts in the leases file > as well, so we need to also account for that in nbleases. But that's a > separate bug that should have a separate patch. ? > >> + char *leasefile = >> networkDnsmasqLeaseFileName(network->def->name); >> + if (!leasefile) { >> + virReportOOMError(); >> + goto cleanup; >> } > Here's a case where you added in an OOM log that really *was* missing > :-) > >> - if (ipdef->bootfile) { >> - virCommandAddArg(cmd, "--dhcp-boot"); >> - if (VIR_SOCKET_ADDR_VALID(&ipdef->bootserver)) { >> - char *bootserver = >> virSocketAddrFormat(&ipdef->bootserver); >> + virCommandAddArgFormat(cmd, "--dhcp-leasefile=%s", leasefile); >> + VIR_FREE(leasefile); >> + virCommandAddArgFormat(cmd, "--dhcp-lease-max=%d", nbleases); >> + } >> - if (!bootserver) >> - goto cleanup; >> - virCommandAddArgFormat(cmd, "%s%s%s", >> - ipdef->bootfile, ",,", >> bootserver); >> - VIR_FREE(bootserver); >> - } else { >> - virCommandAddArg(cmd, ipdef->bootfile); >> + /* this is done once per interface */ >> + if (networkBuildDnsmasqHostsList(dctx, network->def->dns) < 0) >> + goto cleanup; >> + >> + /* Even if there are currently no static hosts, if we're >> + * listening for DHCP, we should write a 0-length hosts >> + * file to allow for runtime additions. >> + */ >> + if (dhcp4flag || dhcp6flag) > replace this with (iopv4def || ipv6def) as discussed above. > >> + virCommandAddArgPair(cmd, "--dhcp-hostsfile", >> + dctx->hostsfile->path); >> + >> + /* Likewise, always create this file and put it on the >> commandline, to allow for >> + * for runtime additions. > You repeated the word "for" > >> + */ >> + virCommandAddArgPair(cmd, "--addn-hosts", >> + dctx->addnhostsfile->path); >> + >> + /* Are we doing RA instead of radvd? */ >> + if (CHECK_VERSION_RA(caps)) { >> + if (dhcp6flag) > if (ipv6def) > >> + virCommandAddArg(cmd, "--enable-ra"); >> + else { >> + for (ii = 0; >> + (ipdef = virNetworkDefGetIpByIndex(network->def, >> AF_INET6, ii)); >> + ii++) { >> + if (!(ipdef->nranges || ipdef->nhosts)) { >> + char *bridgeaddr = >> virSocketAddrFormat(&ipdef->address); >> + if (bridgeaddr) { >> + virCommandAddArgFormat(cmd, >> + "--dhcp-range=%s,ra-only", bridgeaddr); >> + } else { >> + virReportOOMError(); > This error log is unnecessary - virSocketAddrFormat() already logs the > error. > >> + goto cleanup; >> + } >> + VIR_FREE(bridgeaddr); >> + } >> } >> } >> } >> ret = 0; >> + > spurious extra whitespace added. > >> cleanup: >> VIR_FREE(record); >> VIR_FREE(recordPort); >> @@ -893,32 +1012,20 @@ >> networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, >> virCommandPtr *cmdou >> { >> virCommandPtr cmd = NULL; >> int ret = -1, ii; > You've removed the loop that used ii, so it is now unused, and since I > have -Werror, it fails to build. Remove ii. > > >> - virNetworkIpDefPtr ipdef; >> network->dnsmasqPid = -1; >> - /* Look for first IPv4 address that has dhcp defined. */ >> - /* We support dhcp config on 1 IPv4 interface only. */ >> - for (ii = 0; >> - (ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET, >> ii)); >> - ii++) { >> - if (ipdef->nranges || ipdef->nhosts) >> - break; >> - } >> - /* If no IPv4 addresses had dhcp info, pick the first (if there >> were any). */ >> - if (!ipdef) >> - ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET, 0); >> - >> /* If there are no IP addresses at all (v4 or v6), return now, >> since >> * there won't be any address for dnsmasq to listen on anyway. >> * If there are any addresses, even if no dhcp ranges or >> static entries, >> * we should continue and run dnsmasq, just for the DNS >> capabilities. >> + * This should not happen. This code may not be needed. > What do you mean by this? > > >> */ >> if (!virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, 0)) >> return 0; >> cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps)); >> - if (networkBuildDnsmasqArgv(network, ipdef, pidfile, cmd, dctx, >> caps) < 0) { >> + if (networkBuildDnsmasqArgv(network, pidfile, cmd, dctx, caps) >> < 0) { >> goto cleanup; >> } >> @@ -939,11 +1046,9 @@ networkStartDhcpDaemon(struct network_driver >> *driver, >> char *pidfile = NULL; >> int ret = -1; >> dnsmasqContext *dctx = NULL; >> - virNetworkIpDefPtr ipdef; >> - int i; >> if (!virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, 0)) { >> - /* no IPv6 addresses, so we don't need to run radvd */ >> + /* no IP addresses, so we don't need to run */ >> ret = 0; >> goto cleanup; >> } >> @@ -984,18 +1089,6 @@ networkStartDhcpDaemon(struct network_driver >> *driver, >> if (ret < 0) >> goto cleanup; >> - /* populate dnsmasq hosts file */ >> - for (i = 0; (ipdef = virNetworkDefGetIpByIndex(network->def, >> AF_UNSPEC, i)); i++) { >> - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET) && >> - (ipdef->nranges || ipdef->nhosts)) { >> - if (networkBuildDnsmasqHostsfile(dctx, ipdef, >> - network->def->dns) < 0) >> - goto cleanup; >> - >> - break; >> - } >> - } >> - > Hmm. Yeah, this was just added recently (and even ACKed by me) in commit > 23ae3f, but I now see it was wrong to put it here, because the same > thing is already being done in a subordinate function. > >> ret = dnsmasqSave(dctx); >> if (ret < 0) >> goto cleanup; >> @@ -1028,7 +1121,8 @@ cleanup: >> /* networkRefreshDhcpDaemon: >> * Update dnsmasq config files, then send a SIGHUP so that it >> rereads >> - * them. >> + * them. This only works for the dhcp-hostsfile and the >> + * addn-hosts file. >> * >> * Returns 0 on success, -1 on failure. >> */ >> @@ -1037,34 +1131,57 @@ networkRefreshDhcpDaemon(struct >> network_driver *driver, >> virNetworkObjPtr network) >> { >> int ret = -1, ii; >> - virNetworkIpDefPtr ipdef; >> + virNetworkIpDefPtr ipdef, ipv4def, ipv6def; >> dnsmasqContext *dctx = NULL; >> + /* if no IP addresses specified, nothing to do */ >> + if (virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, 0)) >> + return 0; >> + >> /* if there's no running dnsmasq, just start it */ >> if (network->dnsmasqPid <= 0 || (kill(network->dnsmasqPid, 0) >> < 0)) >> return networkStartDhcpDaemon(driver, network); >> - /* Look for first IPv4 address that has dhcp defined. */ >> - /* We support dhcp config on 1 IPv4 interface only. */ >> + VIR_INFO("REFRESH: DhcpDaemon: for %s", network->def->bridge); > I don't like the all CAPS or the use of "DhcpDaemon". Instead, you > can say > > "Refreshing dnsmasq for network '%s'" > >> + if (!(dctx = dnsmasqContextNew(network->def->name, >> DNSMASQ_STATE_DIR))) >> + goto cleanup; >> + >> + /* Look for first IPv4 address that has dhcp defined. >> + * We only support dhcp-host config on one IPv4 subnetwork >> + * and on one IPv6 subnetwork. >> + */ >> + ipv4def = NULL; >> for (ii = 0; >> (ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET, >> ii)); >> ii++) { >> - if (ipdef->nranges || ipdef->nhosts) >> - break; >> + if (ipdef->nranges || ipdef->nhosts) { >> + if (!ipv4def) >> + ipv4def = ipdef; >> + } > if (!ipv4def && (ipdef->nranges || ipdef->nhosts)) > ipv4def = ipdef; > >> } >> /* If no IPv4 addresses had dhcp info, pick the first (if >> there were any). */ >> if (!ipdef) >> ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET, 0); >> - if (!ipdef) { >> - /* no <ip> elements, so nothing to do */ >> - return 0; >> + ipv6def = NULL; >> + for (ii = 0; >> + (ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET6, >> ii)); >> + ii++) { >> + if (ipdef->nranges || ipdef->nhosts) { >> + if (!ipv6def) >> + ipv6def = ipdef; >> + } > if (!ipv6def && (ipdef->nranges || ipdef->nhosts)) > ipv6def = ipdef; > >> } >> - if (!(dctx = dnsmasqContextNew(network->def->name, >> DNSMASQ_STATE_DIR))) >> - goto cleanup; >> + if (ipv4def) >> + if (networkBuildDnsmasqDhcpHostsList(dctx, ipv4def) < 0) >> + goto cleanup; > if (ipv4def && (networkBuildDnsmasqDhcpHostsList(dctx, > ipv4def) < 0) > goto cleanup; > >> - if (networkBuildDnsmasqHostsfile(dctx, ipdef, >> network->def->dns) < 0) >> + if (ipv6def) >> + if (networkBuildDnsmasqDhcpHostsList(dctx, ipv6def) < 0) >> + goto cleanup; > Same as above - combine the nested ifs. > >> + >> + if (networkBuildDnsmasqHostsList(dctx, network->def->dns) < 0) >> goto cleanup; >> if ((ret = dnsmasqSave(dctx)) < 0) >> @@ -1097,27 +1214,51 @@ networkRestartDhcpDaemon(struct >> network_driver *driver, >> return networkStartDhcpDaemon(driver, network); >> } >> +static char radvd1[] = " AdvOtherConfigFlag off;\n\n"; >> +static char radvd2[] = " AdvAutonomous off;\n"; >> +static char radvd3[] = " AdvOnLink on;\n" >> + " AdvAutonomous on;\n" >> + " AdvRouterAddr off;\n"; >> + >> static int >> networkRadvdConfContents(virNetworkObjPtr network, char **configstr) >> { >> virBuffer configbuf = VIR_BUFFER_INITIALIZER; >> int ret = -1, ii; >> virNetworkIpDefPtr ipdef; >> - bool v6present = false; >> + bool v6present = false, dhcp6 = false; >> *configstr = NULL; >> + /* Check if DHCPv6 is needed */ >> + for (ii = 0; >> + (ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET6, >> ii)); >> + ii++) { >> + v6present = true; >> + if (ipdef->nranges || ipdef->nhosts) { >> + dhcp6 = true; >> + break; >> + } >> + } >> + >> + /* If there are no IPv6 addresses, then we are done */ >> + if (!v6present) { >> + ret = 0; >> + goto cleanup; >> + } >> + >> /* create radvd config file appropriate for this network; >> * IgnoreIfMissing allows radvd to start even when the bridge >> is down >> */ >> virBufferAsprintf(&configbuf, "interface %s\n" >> "{\n" >> " AdvSendAdvert on;\n" >> - " AdvManagedFlag off;\n" >> - " AdvOtherConfigFlag off;\n" >> " IgnoreIfMissing on;\n" >> - "\n", >> - network->def->bridge); >> + " AdvManagedFlag %s;\n" >> + "%s", >> + network->def->bridge, >> + dhcp6 ? "on" : "off", >> + dhcp6 ? "\n" : radvd1); >> /* add a section for each IPv6 address in the config */ >> for (ii = 0; >> @@ -1126,7 +1267,6 @@ networkRadvdConfContents(virNetworkObjPtr >> network, char **configstr) >> int prefix; >> char *netaddr; >> - v6present = true; >> prefix = virNetworkIpDefPrefix(ipdef); >> if (prefix < 0) { >> virReportError(VIR_ERR_INTERNAL_ERROR, >> @@ -1138,12 +1278,9 @@ networkRadvdConfContents(virNetworkObjPtr >> network, char **configstr) >> goto cleanup; >> virBufferAsprintf(&configbuf, >> " prefix %s/%d\n" >> - " {\n" >> - " AdvOnLink on;\n" >> - " AdvAutonomous on;\n" >> - " AdvRouterAddr off;\n" >> - " };\n", >> - netaddr, prefix); >> + " {\n%s };\n", >> + netaddr, prefix, >> + dhcp6 ? radvd2 : radvd3); >> VIR_FREE(netaddr); >> } >> @@ -1209,7 +1346,8 @@ cleanup: >> } >> static int >> -networkStartRadvd(virNetworkObjPtr network) >> +networkStartRadvd(struct network_driver *driver ATTRIBUTE_UNUSED, >> + virNetworkObjPtr network) >> { >> char *pidfile = NULL; >> char *radvdpidbase = NULL; >> @@ -1217,6 +1355,12 @@ networkStartRadvd(virNetworkObjPtr network) >> virCommandPtr cmd = NULL; >> int ret = -1; >> + /* Is dnsmasq handling RA? */ >> + if (CHECK_VERSION_RA(driver->dnsmasqCaps)) { >> + ret = 0; >> + goto cleanup; >> + } >> + >> network->radvdPid = -1; > I think you want to set radvdPid = -1 *before* checking if dnsmasq > supports RA, otherwise you could later end up trying to kill a bogus > pid. > > >> if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0)) { >> @@ -1295,9 +1439,13 @@ static int >> networkRefreshRadvd(struct network_driver *driver ATTRIBUTE_UNUSED, >> virNetworkObjPtr network) >> { >> + /* Is dnsmasq handling RA? */ >> + if (CHECK_VERSION_RA(driver->dnsmasqCaps)) >> + return 0; >> + > I don't think this is what you want here. Instead, you should check if > radvd is running and, if so, kill it so that dnsmasq can take over - you > need to think about the case where you're upgrading from an older > libvirt that didn't support using dnsmasq for RA (and also for the case > where you upgrade dnsmasq from pre-2.64 to post-2.64, then restart > libvirtd). > > >> /* if there's no running radvd, just start it */ >> if (network->radvdPid <= 0 || (kill(network->radvdPid, 0) < 0)) >> - return networkStartRadvd(network); >> + return networkStartRadvd(driver, network); >> if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0)) { >> /* no IPv6 addresses, so we don't need to run radvd */ >> @@ -1679,9 +1827,19 @@ networkAddGeneralIp6tablesRules(struct >> network_driver *driver, >> goto err5; >> } >> + if (iptablesAddUdpInput(driver->iptables, AF_INET6, >> + network->def->bridge, 547) < 0) { >> + virReportError(VIR_ERR_SYSTEM_ERROR, >> + _("failed to add ip6tables rule to allow >> DHCP6 requests from '%s'"), >> + network->def->bridge); >> + goto err6; >> + } >> + >> return 0; >> /* unwind in reverse order from the point of failure */ >> +err6: >> + iptablesRemoveUdpInput(driver->iptables, AF_INET6, >> network->def->bridge, 53); >> err5: >> iptablesRemoveTcpInput(driver->iptables, AF_INET6, >> network->def->bridge, 53); >> err4: >> @@ -1702,6 +1860,7 @@ networkRemoveGeneralIp6tablesRules(struct >> network_driver *driver, >> !network->def->ipv6nogw) >> return; >> if (virNetworkDefGetIpByIndex(network->def, AF_INET6, 0)) { >> + iptablesRemoveUdpInput(driver->iptables, AF_INET6, >> network->def->bridge, 547); >> iptablesRemoveUdpInput(driver->iptables, AF_INET6, >> network->def->bridge, 53); >> iptablesRemoveTcpInput(driver->iptables, AF_INET6, >> network->def->bridge, 53); >> } >> @@ -2293,7 +2452,7 @@ networkStartNetworkVirtual(struct >> network_driver *driver, >> goto err3; >> /* start radvd if there are any ipv6 addresses */ >> - if (v6present && networkStartRadvd(network) < 0) >> + if (v6present && networkStartRadvd(driver, network) < 0) >> goto err4; >> /* DAD has happened (dnsmasq waits for it), dnsmasq is now >> bound to the >> @@ -2754,8 +2913,7 @@ networkValidate(struct network_driver *driver, >> bool vlanUsed, vlanAllowed, badVlanUse = false; >> virPortGroupDefPtr defaultPortGroup = NULL; >> virNetworkIpDefPtr ipdef; >> - bool ipv4def = false; >> - int i; >> + bool ipv4def = false, ipv6def = false; >> /* check for duplicate networks */ >> if (virNetworkObjIsDuplicate(&driver->networks, def, >> check_active) < 0) >> @@ -2774,17 +2932,36 @@ networkValidate(struct network_driver *driver, >> virNetworkSetBridgeMacAddr(def); >> } >> - /* We only support dhcp on one IPv4 address per defined >> network */ >> - for (i = 0; (ipdef = virNetworkDefGetIpByIndex(def, AF_INET, >> i)); i++) { >> - if (ipdef->nranges || ipdef->nhosts) { >> - if (ipv4def) { >> - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> - _("Multiple dhcp sections found. " >> + /* We only support dhcp on one IPv4 address and >> + * on one IPv6 address per defined network >> + */ >> + for (ii = 0; >> + (ipdef = virNetworkDefGetIpByIndex(def, AF_UNSPEC, ii)); >> + ii++) { >> + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET)) { >> + if (ipdef->nranges || ipdef->nhosts) { >> + if (ipv4def) { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("Multiple IPv4 dhcp sections found >> -- " >> "dhcp is supported only for a " >> "single IPv4 address on each >> network")); >> - return -1; >> - } else { >> - ipv4def = true; >> + return -1; >> + } else { >> + ipv4def = true; >> + } >> + } >> + } >> + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) { >> + if (ipdef->nranges || ipdef->nhosts) { >> + if (ipv6def) { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("Multiple IPv6 dhcp sections found >> -- " >> + "dhcp is supported only for a " >> + "single IPv6 address on each >> network")); >> + return -1; >> + } else { >> + ipv6def = true; >> + } >> } >> } >> } >> diff --git a/src/util/dnsmasq.c b/src/util/dnsmasq.c >> index 4f210d2..8f26d42 100644 >> --- a/src/util/dnsmasq.c >> +++ b/src/util/dnsmasq.c >> @@ -306,7 +306,14 @@ hostsfileAdd(dnsmasqHostsfile *hostsfile, >> if (!(ipstr = virSocketAddrFormat(ip))) >> return -1; >> - if (name) { >> + /* the first test determins if it is a dhcpv6 host */ > s/determins/determines/ > > > And is that actually true? I thought you could have ipv4 static hosts > based on name as well. > You should instead check the FAMILY of the address that is passed in. > > >> + if (mac==NULL) { >> + if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, >> "%s,[%s]", >> + name, ipstr) < 0) { >> + goto alloc_error; >> + } >> + } >> + else if (name) { >> if (virAsprintf(&hostsfile->hosts[hostsfile->nhosts].host, >> "%s,%s,%s", > Hmm, but according to this just giving name and IP for IPv4 would blow > up in your face... > > Can you ask about this on dnsmasq list (or verify in the code)? If > name+ip-only is allowed for IPv4, we need to change the hostsfileAdd > function, and if not, we need to change the parse to always require a > mac address for ipv4 (currently it requires either name or ip but not > both). > > >> mac, ipstr, name) < 0) { >> goto alloc_error; > I'll trust that the changes to the tests are correct, since they all > still pass :-) > > > It's taking me *forever to get through this, so I'm splitting the review > here and sending what I've written so far. > > I've attached a diff that includes all the changes I requested for > network_conf.c and formatnetwork.html.in. Once I got into > bridge_driver.c, it got too complicated and too likely to break the next > patch, so I stopped. If you can squash the included patch into your > current patch, then take up making the changes with bridge_driver.c, > then everything should be good. > > (BTW, I'm including diffs because that's often easier to do than try and > explain exactly what I want, and also because we'll be freezing for > release later this week, and I want to get these all in if at all > possible.) > > Oh, and also - a bit later today I'll squash my changes into your 1/3 > and push, so you'll probably want to make a new branch off master and > cherry-pick your 2/3 and 3/3 over into that to continue. (unfortunately > I first have to make a trip to the doctor for a daughter with a fever, > so it may be awhile :-() Been there done that. You always need to keep priorities straight and this stuff does not matter all that much when compared to family. A choice for you: 1. I can sit back and let you continue fixing things OR 2. I can take your comments (and diff) and then make the changes indicated to create either a new version of the patches or patches to go on top of this patch file.