[libvirt] [PATCHv3 2/3] v8.2 add support for DHCPv6
Gene Czarcinski
gene at czarc.net
Tue Dec 4 21:56:44 UTC 2012
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 @@
>> <ip family="ipv6" address="2001:db8:ca2:2::1" prefix="64" />
>> </network></pre>
>>
>> +
>> + <p>
>> + Below is a variation of the above example which adds an IPv6
>> + dhcp range definition.
>> + </p>
>> +
>> + <pre>
>> + <network>
>> + <name>default6</name>
>> + <bridge name="virbr0" />
>> + <forward mode="nat"/>
>> + <ip address="192.168.122.1" netmask="255.255.255.0">
>> + <dhcp>
>> + <range start="192.168.122.2" end="192.168.122.254" />
>> + </dhcp>
>> + </ip>
>> + <ip family="ipv6" address="2001:db8:ca2:2::1" prefix="64" >
>> + <dhcp>
>> + <range start="2001:db8:ca2:2:1::10" end="2001:db8:ca2:2:1::ff" />
>> + </dhcp>
>> + </ip>
>> + </network></pre>
>> +
>> <h3><a name="examplesRoute">Routed network config</a></h3>
>>
>> <p>
>> @@ -704,6 +751,29 @@
>> <ip family="ipv6" address="2001:db8:ca2:2::1" prefix="64" />
>> </network></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>
>> + <network>
>> + <name>local6</name>
>> + <bridge name="virbr1" />
>> + <forward mode="route" dev="eth1"/>
>> + <ip address="192.168.122.1" netmask="255.255.255.0">
>> + <dhcp>
>> + <range start="192.168.122.2" end="192.168.122.254" />
>> + </dhcp>
>> + </ip>
>> + <ip family="ipv6" address="2001:db8:ca2:2::1" prefix="64" >
>> + <dhcp>
>> + <host name="paul" ip="2001:db8:ca2:2:3::1" />
>> + <host name="bob" ip="2001:db8:ca2:2:3::2" />
>> + </dhcp>
>> + </ip>
>> + </network></pre>
>> +
>> <h3><a name="examplesPrivate">Isolated network config</a></h3>
>>
>> <p>
>> @@ -726,6 +796,24 @@
>> <ip family="ipv6" address="2001:db8:ca2:3::1" prefix="64" />
>> </network></pre>
>>
>> + <h3><a name="examplesPrivate6">Isolated IPv6 network config</a></h3>
>> +
>> + <p>
>> + This variation of an isolated network defines only IPv6.
>> + </p>
>> +
>> + <pre>
>> + <network>
>> + <name>sixnet</name>
>> + <bridge name="virbr6" />
>> + <ip family="ipv6" address="2001:db8:ca2:6::1" prefix="64" >
>> + <dhcp>
>> + <host name="peter" ip="2001:db8:ca2:6:6::1" />
>> + <host name="dariusz" ip="2001:db8:ca2:6:6::2" />
>> + </dhcp>
>> + </ip>
>> + </network></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.
Gene
More information about the libvir-list
mailing list