On 12/03/2012 11:13 AM, Gene Czarcinski wrote:

This patch adds the capability for virtual guests to do IPv6 communication via a virtual network interface with no IPv6 (gateway) addresses specified. This capability currently exists for IPv4.

This patch allows creation of a completely isolated IPv6 network.

Note that virtual guests cannot communication with the virtualization host via this interface. Also note that: net.ipv6.conf.<interface_name>.disable_ipv6 = 1

Also not that starting libvirtd has set the following: net.bridge.bridge-nf-call-arptables = 1 net.bridge.bridge-nf-call-ip6tables = 1 net.bridge.bridge-nf-call-iptables = 1 although /etc/syslog.conf has them all set to 0.

To control this behavior so that it is not enabled by default, the parameter ipv6='yes' on the <network> statement has been added.

Documentation related to this patch has been updated. The network schema has also been updated. --- docs/formatnetwork.html.in | 28 +++++++++++++++++++++++++++- docs/schemas/network.rng | 10 ++++++++++ src/conf/network_conf.c | 8 ++++++++ src/conf/network_conf.h | 5 +++++ src/network/bridge_driver.c | 25 ++++++++++++++++++++----- 5 files changed, 70 insertions(+), 6 deletions(-)

diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 49206dd..a3a5ced 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -33,7 +33,7 @@ </p>

<pre> - <network> + <network ipv6='yes'> <name>default</name> <uuid>3e3fce45-4f53-4fa7-bb32-11f34168b82b</uuid> ...</pre> @@ -52,6 +52,12 @@ The format must be RFC 4122 compliant, eg <code>3e3fce45-4f53-4fa7-bb32-11f34168b82b</code>. If omitted when defining/creating a new network, a random UUID is generated. <span class="since">Since 0.3.0</span></dd> + <dt><code>ipv6='yes'</code></dt> + <dd>The new, optional parameter <code>ipv6='yes'</code> enables + a network definition with no IPv6 gateway addresses specified + to have guest-to-guest communications. For further information, + see the example below for the example with no gateway addresses. + <span class="since">Since 1.0.1</span></dd> </dl>

<h3><a name="elementsConnect">Connectivity</a></h3> @@ -773,5 +779,25 @@ </forward> </network></pre>

+ <h3><a name="examplesNoGateway">Network config with no gateway addresses</a></h3> + + <p> + A valid network definition can contain no IPv4 or IPv6 addresses. Such a definition + can be used for a "very private" or "very isolated" network since it will not be + possible to communicate with the virtualization host via this network. However, + this virtual network interface can be used for communication between virtual guest + systems. This works for IPv4 and <span class="since">(Since 1.0.1)</span> IPv6. + However, the new ipv6='yes' must be added for guest-to-guest IPv6 + communication. + </p>

I reformatted this paragraph to fit within 80 columns.

+ + <pre> + <network ipv6='yes'> + <name>nogw</name> + <uuid>7a3b7497-1ec7-8aef-6d5c-38dff9109e93</uuid> + <bridge name="virbr2" stp="on" delay="0" /> + <mac address='00:16:3E:5D:C7:9E'/> + </network></pre> + </body> </html> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 4abfd91..0d67f7f 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -17,6 +17,16 @@ <data type="unsignedInt"/> </attribute> </optional> + <!-- Enables IPv6 guest-to-guest communications on a network + with no gateways addresses specified --> + <optional> + <attribute name="ipv6"> + <choice> + <value>yes</value> + <value>no</value> + </choice> + </attribute> + </optional> <interleave>

<!-- The name of the network, used to refer to it through the API diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 6ce2e63..3f9e13c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1264,6 +1264,12 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) def->uuid_specified = true; }

+ /* check if definitions with no IPv6 gateway addresses is to + * allow guest-to-guest communications. + */ + if (virXPathBoolean("boolean(./@ipv6)", ctxt) == 1) + def->ipv6nogw = true;

I don't think virXPathBoolean does what you think it does (although I haven't figured out exactly what it *does* do, I've noticed that 1) none of the rest of libvirt uses it for yes/no attributes, and 2) in the libxml2 source code, when an XPATH_BOOLEAN is formatted into a string, it becomes "True" or "False". I'm changing the above lines to the following: ipv6nogwStr = virXPathString("string(./@ipv6)", ctxt); if (ipv6nogwStr) { if (STREQ(ipv6nogwStr, "yes")) { def->ipv6nogw = true; } else if (STRNEQ(ipv6nogwStr, "no")) { virReportError(VIR_ERR_XML_ERROR, _("Invalid ipv6 setting '%s' in network '%s'"), ipv6nogwStr, def->name); goto error; } VIR_FREE(ipv6nogwStr); } (with the necessary NULL-initialized declaration of ipv6nogwStr at the beginning of the function, and VIR_FREE(ipv6nogwStr) at the error: label). Depending on the amount of changes I make beyond that, I'll either attach an interdiff to this mail, or send an update of your patch with that change.

+ /* Parse network domain information */ def->domain = virXPathString("string(./domain[1]/@name)", ctxt);

@@ -1839,6 +1845,8 @@ char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags) if (!(flags & VIR_NETWORK_XML_INACTIVE) && (def->connections > 0)) { virBufferAsprintf(&buf, " connections='%d'", def->connections); } + if (def->ipv6nogw) + virBufferAddLit(&buf, " ipv6='yes'"); virBufferAddLit(&buf, ">\n"); virBufferAdjustIndent(&buf, 2); virBufferEscapeString(&buf, "<name>%s</name>\n", def->name); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 3e46304..949b3d2 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -184,6 +184,11 @@ struct _virNetworkDef { virMacAddr mac; /* mac address of bridge device */ bool mac_specified;

+ /* specified if ip6tables rules added + * when no ipv6 gateway addresses specified. + */ + bool ipv6nogw;

Heh. Someday we should go through the driver object structures and change all of the ints and bitfields used as booleans into bool...

+ int forwardType; /* One of virNetworkForwardType constants */ int managed; /* managed attribute for hostdev mode */

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 75f3c3a..cb2997d 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1617,13 +1617,18 @@ networkRemoveRoutingIptablesRules(struct network_driver *driver, } }

-/* Add all once/network rules required for IPv6 (if any IPv6 addresses are defined) */ +/* Add all once/network rules required for IPv6. + * If no IPv6 addresses are defined and <network ipv6='yes'> is + * specified, then allow IPv6 commuinications between virtual systems.

I changed "virtual systems" to "guests".

+ * If any IPv6 addresses are defined, then add the rules for regular operation.

Changed to "then add all rules for regular operation (including inter-guest communication)."

+ */ static int networkAddGeneralIp6tablesRules(struct network_driver *driver, virNetworkObjPtr network) {

- if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0)) + if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0) && + !network->def->ipv6nogw) return 0;

Fixed odd indentation of !network->def->ipv6nogw, and put braces around the body (since the expression is multi-line.

/* Catch all rules to block forwarding to/from bridges */ @@ -1653,6 +1658,10 @@ networkAddGeneralIp6tablesRules(struct network_driver *driver, goto err3; }

+ /* if no IPv6 addresses are defined, we are done. */ + if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0)) + return 0; + /* allow DNS over IPv6 */ if (iptablesAddTcpInput(driver->iptables, AF_INET6, network->def->bridge, 53) < 0) { @@ -1689,11 +1698,17 @@ static void networkRemoveGeneralIp6tablesRules(struct network_driver *driver, virNetworkObjPtr network) { - if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0)) + if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0) && + !network->def->ipv6nogw) return; + if (virNetworkDefGetIpByIndex(network->def, AF_INET6, 0)) { + iptablesRemoveUdpInput(driver->iptables, AF_INET6, network->def->bridge, 53); + iptablesRemoveTcpInput(driver->iptables, AF_INET6, network->def->bridge, 53); + }

- iptablesRemoveUdpInput(driver->iptables, AF_INET6, network->def->bridge, 53); - iptablesRemoveTcpInput(driver->iptables, AF_INET6, network->def->bridge, 53); + /* the following rules are there if no IPv6 address has been defined + * but network->def->ipv6nogw == true + */ iptablesRemoveForwardAllowCross(driver->iptables, AF_INET6, network->def->bridge); iptablesRemoveForwardRejectIn(driver->iptables, AF_INET6, network->def->bridge); iptablesRemoveForwardRejectOut(driver->iptables, AF_INET6, network->def->bridge);

One other thing that we forgot about - a test case for networkxml2xmltest (xml2argv is only useful for things that affect dhcp). I've added a very simple test case to networkxml2xml(in|out) and added it to the list in networkxml2xmltest.c - it defines a network with no IP addresses, but with "ipv6='yes'". I also added "ipv6='no'" to networkxml2xmlin/isolated-network.xml to make sure that it is accepted by the parser and results in ipv6nogw *not* being set (it of course won't show up in the output). ACK with the changes I've squashed in. I've attached an interdiff of the changes I've made to this message, and will push as soon as someone else ACKs those additional changes. Thanks for the contribution! I know it's sometimes a pain to get things all the way to pushing, but it really does help keep the expansion manageable and prevent unwanted surprises for our diverse set of users.

On 12/03/2012 11:13 AM, Gene Czarcinski wrote:

...

This patch adds the capability for virtual guests to do IPv6 communication via a virtual network interface with no IPv6 (gateway) addresses specified. This capability currently exists for IPv4.

This patch allows creation of a completely isolated IPv6 network.

Note that virtual guests cannot communication with the virtualization host via this interface. Also note that: net.ipv6.conf.<interface_name>.disable_ipv6 = 1

Also not that starting libvirtd has set the following: net.bridge.bridge-nf-call-arptables = 1 net.bridge.bridge-nf-call-ip6tables = 1 net.bridge.bridge-nf-call-iptables = 1 although /etc/syslog.conf has them all set to 0.

To control this behavior so that it is not enabled by default, the parameter ipv6='yes' on the <network> statement has been added.

Documentation related to this patch has been updated. The network schema has also been updated. --- docs/formatnetwork.html.in | 28 +++++++++++++++++++++++++++- docs/schemas/network.rng | 10 ++++++++++ src/conf/network_conf.c | 8 ++++++++ src/conf/network_conf.h | 5 +++++ src/network/bridge_driver.c | 25 ++++++++++++++++++++----- 5 files changed, 70 insertions(+), 6 deletions(-)

diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 49206dd..a3a5ced 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -33,7 +33,7 @@ </p>

<pre> - <network> + <network ipv6='yes'> <name>default</name> <uuid>3e3fce45-4f53-4fa7-bb32-11f34168b82b</uuid> ...</pre> @@ -52,6 +52,12 @@ The format must be RFC 4122 compliant, eg <code>3e3fce45-4f53-4fa7-bb32-11f34168b82b</code>. If omitted when defining/creating a new network, a random UUID is generated. <span class="since">Since 0.3.0</span></dd> + <dt><code>ipv6='yes'</code></dt> + <dd>The new, optional parameter <code>ipv6='yes'</code> enables + a network definition with no IPv6 gateway addresses specified + to have guest-to-guest communications. For further information, + see the example below for the example with no gateway addresses. + <span class="since">Since 1.0.1</span></dd> </dl>

<h3><a name="elementsConnect">Connectivity</a></h3> @@ -773,5 +779,25 @@ </forward> </network></pre>

+ <h3><a name="examplesNoGateway">Network config with no gateway addresses</a></h3> + + <p> + A valid network definition can contain no IPv4 or IPv6 addresses. Such a definition + can be used for a "very private" or "very isolated" network since it will not be + possible to communicate with the virtualization host via this network. However, + this virtual network interface can be used for communication between virtual guest + systems. This works for IPv4 and <span class="since">(Since 1.0.1)</span> IPv6. + However, the new ipv6='yes' must be added for guest-to-guest IPv6 + communication. + </p> I reformatted this paragraph to fit within 80 columns.

...

+ + <pre> + <network ipv6='yes'> + <name>nogw</name> + <uuid>7a3b7497-1ec7-8aef-6d5c-38dff9109e93</uuid> + <bridge name="virbr2" stp="on" delay="0" /> + <mac address='00:16:3E:5D:C7:9E'/> + </network></pre> + </body> </html> diff --git a/docs/schemas/network.rng b/docs/schemas/network.rng index 4abfd91..0d67f7f 100644 --- a/docs/schemas/network.rng +++ b/docs/schemas/network.rng @@ -17,6 +17,16 @@ <data type="unsignedInt"/> </attribute> </optional> + <!-- Enables IPv6 guest-to-guest communications on a network + with no gateways addresses specified --> + <optional> + <attribute name="ipv6"> + <choice> + <value>yes</value> + <value>no</value> + </choice> + </attribute> + </optional> <interleave>

<!-- The name of the network, used to refer to it through the API diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 6ce2e63..3f9e13c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1264,6 +1264,12 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) def->uuid_specified = true; }

+ /* check if definitions with no IPv6 gateway addresses is to + * allow guest-to-guest communications. + */ + if (virXPathBoolean("boolean(./@ipv6)", ctxt) == 1) + def->ipv6nogw = true; I don't think virXPathBoolean does what you think it does (although I haven't figured out exactly what it *does* do, I've noticed that 1) none of the rest of libvirt uses it for yes/no attributes, and 2) in the libxml2 source code, when an XPATH_BOOLEAN is formatted into a string, it becomes "True" or "False". I'm changing the above lines to the following:

ipv6nogwStr = virXPathString("string(./@ipv6)", ctxt); if (ipv6nogwStr) { if (STREQ(ipv6nogwStr, "yes")) { def->ipv6nogw = true; } else if (STRNEQ(ipv6nogwStr, "no")) { virReportError(VIR_ERR_XML_ERROR, _("Invalid ipv6 setting '%s' in network '%s'"), ipv6nogwStr, def->name); goto error; } VIR_FREE(ipv6nogwStr); }

(with the necessary NULL-initialized declaration of ipv6nogwStr at the beginning of the function, and VIR_FREE(ipv6nogwStr) at the error: label).

Depending on the amount of changes I make beyond that, I'll either attach an interdiff to this mail, or send an update of your patch with that change. I had previously tried virXPathBoolean() and once you realize that it returns an int which can have values of -1, 0, and 1 with -1 meaning it was missing it seems to work OK. It also seems to with on/off or

On 12/03/2012 04:03 PM, Laine Stump wrote: true/false the same. Since I was setting ipv6='on' in <network>, it recognized that. But, this works also.

...

+ /* Parse network domain information */ def->domain = virXPathString("string(./domain[1]/@name)", ctxt);

@@ -1839,6 +1845,8 @@ char *virNetworkDefFormat(const virNetworkDefPtr def, unsigned int flags) if (!(flags & VIR_NETWORK_XML_INACTIVE) && (def->connections > 0)) { virBufferAsprintf(&buf, " connections='%d'", def->connections); } + if (def->ipv6nogw) + virBufferAddLit(&buf, " ipv6='yes'"); virBufferAddLit(&buf, ">\n"); virBufferAdjustIndent(&buf, 2); virBufferEscapeString(&buf, "<name>%s</name>\n", def->name); diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 3e46304..949b3d2 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -184,6 +184,11 @@ struct _virNetworkDef { virMacAddr mac; /* mac address of bridge device */ bool mac_specified;

+ /* specified if ip6tables rules added + * when no ipv6 gateway addresses specified. + */ + bool ipv6nogw; Heh. Someday we should go through the driver object structures and change all of the ints and bitfields used as booleans into bool...

... but doing it carefully. Sometimes an int is used to true/false/something-else ... not every answer can be binary witness what is returned by virXPathBoolean().

...

+ int forwardType; /* One of virNetworkForwardType constants */ int managed; /* managed attribute for hostdev mode */

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 75f3c3a..cb2997d 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1617,13 +1617,18 @@ networkRemoveRoutingIptablesRules(struct network_driver *driver, } }

-/* Add all once/network rules required for IPv6 (if any IPv6 addresses are defined) */ +/* Add all once/network rules required for IPv6. + * If no IPv6 addresses are defined and <network ipv6='yes'> is + * specified, then allow IPv6 commuinications between virtual systems. I changed "virtual systems" to "guests".

I tried to be consistent but sometimes I missed.

...

+ * If any IPv6 addresses are defined, then add the rules for regular operation. Changed to "then add all rules for regular operation (including inter-guest communication)."

...

+ */ static int networkAddGeneralIp6tablesRules(struct network_driver *driver, virNetworkObjPtr network) {

- if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0)) + if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0) && + !network->def->ipv6nogw) return 0; Fixed odd indentation of !network->def->ipv6nogw, and put braces around the body (since the expression is multi-line.

...

/* Catch all rules to block forwarding to/from bridges */ @@ -1653,6 +1658,10 @@ networkAddGeneralIp6tablesRules(struct network_driver *driver, goto err3; }

+ /* if no IPv6 addresses are defined, we are done. */ + if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0)) + return 0; + /* allow DNS over IPv6 */ if (iptablesAddTcpInput(driver->iptables, AF_INET6, network->def->bridge, 53) < 0) { @@ -1689,11 +1698,17 @@ static void networkRemoveGeneralIp6tablesRules(struct network_driver *driver, virNetworkObjPtr network) { - if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0)) + if (!virNetworkDefGetIpByIndex(network->def, AF_INET6, 0) && + !network->def->ipv6nogw) return; + if (virNetworkDefGetIpByIndex(network->def, AF_INET6, 0)) { + iptablesRemoveUdpInput(driver->iptables, AF_INET6, network->def->bridge, 53); + iptablesRemoveTcpInput(driver->iptables, AF_INET6, network->def->bridge, 53); + }

- iptablesRemoveUdpInput(driver->iptables, AF_INET6, network->def->bridge, 53); - iptablesRemoveTcpInput(driver->iptables, AF_INET6, network->def->bridge, 53); + /* the following rules are there if no IPv6 address has been defined + * but network->def->ipv6nogw == true + */ iptablesRemoveForwardAllowCross(driver->iptables, AF_INET6, network->def->bridge); iptablesRemoveForwardRejectIn(driver->iptables, AF_INET6, network->def->bridge); iptablesRemoveForwardRejectOut(driver->iptables, AF_INET6, network->def->bridge);

One other thing that we forgot about - a test case for networkxml2xmltest (xml2argv is only useful for things that affect dhcp). I've added a very simple test case to networkxml2xml(in|out) and added it to the list in networkxml2xmltest.c - it defines a network with no IP addresses, but with "ipv6='yes'". I also added "ipv6='no'" to networkxml2xmlin/isolated-network.xml to make sure that it is accepted by the parser and results in ipv6nogw *not* being set (it of course won't show up in the output). I thought I was getting away without a new test ;)

ACK with the changes I've squashed in. I've attached an interdiff of the changes I've made to this message, and will push as soon as someone else ACKs those additional changes.

Thanks for the contribution! I know it's sometimes a pain to get things all the way to pushing, but it really does help keep the expansion manageable and prevent unwanted surprises for our diverse set of users. ACK ... this is good! I appreciate your help on this.

This was a small change (I almost forgot I needed to change the schema). I can hardly wait for your review of the DHCPv6 and conf-file patches. BTW. While I appreciate you fixing things up, I am perfectly willing to make corrections when (not if) you find stuff has not been done "right." Yes, some of it might be a matter of style difference but consistency counts when you are trying to understand what some code is doing. Gene

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

On 12/05/2012 10:00 AM, Laine Stump wrote:

...

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. I usually feel completely incompetent after reading any review of my

On 12/04/2012 04:56 PM, Gene Czarcinski wrote: patches :-)

...

...

...

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")) {

...

+ 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

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! 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. Okay, so for now there's no point in allowing those.

...

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. What you *really* wanted to remove was "family='ipv4'. Instead, you've arrived at a log message that says "couldn't update dhcp host entry - element found at index %d in network '%s'". It should say "no <ip> element found at ...". Anyway, I fixed that in the patch I included with my review.

(I did forget to do one thing though - both of the messages in this function talk about "host entry", but this is a helper function that's used for both <host> and <range>, so both of the log messages should have the word "host" removed - just "couldn't update dhcp entry ..." is enough, and is correct in both cases.)

...

...

...

} @@ -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. And the same reply to answer as above :-)

(This reminds me of when I worked for a dictionary publisher back in the 1980s. I was surpsised to learn that the proofreaders would read the entire dictionary *backwards* (in addition to reading it forwards) in order to catch more errors that they would have skipped right over if they were reading forwards.

...

...

...

} 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. Me too. Maybe we'll think of something better later on, and can replace this.

...

I am just overjoyed that dnsmasq 2.64 should be out tonight and that the problem with RA was found and fixed. Yes, I'm subscribed to dnsmasq-discuss and followed the entire saga. It sounds like your testing was essential to getting the code working correctly. It's good you had the time/motivation to investigate it.

...

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? As the saying goes ... "faster than a speeding bullet" ... https://bugzilla.redhat.com/show_bug.cgi?id=883819

Now if I could just figure out how to get F18 installed from the DVD. Fortunately, "fedup" to the rescue ... it worked! I suspect there are going to be some anaconda developers who will be "burning the midnight oil" between now and the release. Based on my recent experience with dnsmasq and the RA problem, I empathize with them.

It's up to him of course, but my guess is there won't be any reluctance to do that for Fedora 18 (although it will probably be an update after the release). For F17, that all depends on the temperament of the maintainer. Some packages (e.g. libvirt) are *never* rebased after the beta freeze, and some are a bit more relaxed.

...

...

...

+ /* 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. Yeah, I figured that was the case. That happens a lot.

...

...

...

/* * 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. I'm pretty pressed for time, so I think it would be best if you squashed in the patch that I sent with the initial review to this patch (2/3) (this will take care of all the problems I found with network_conf.c), then make the modifications I suggested for bridge_driver.c yourself.

I still haven't pushed 1/3, but will try to do that this morning.

I have been a bit distracted myself. Between trying to get F18-beta installed on my test hardware (something I thought would be easy and not take too long) and having some minor back surgery this morning (for pain and it is minor although my back is a bit sore), I am just getting back to this. You have written a few more missives so I will wait to do anything until I read them all. Gene

On 12/04/2012 03:03 PM, Laine Stump wrote:

...

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

You caught a good one here. The code in src/conf/network.c is correct and supports only name and Address for IPv6 as well as name+mac+ip or name+ip or mac+ip for IPv4. The code in dnsmasq.c did not but does now. There are a whole bunch of variations supported by dnsmasq but I am not sure how many other are worth any effort. One thing (I hope this is not a show stopper). In the spirit of "what's done is done," I "ignored" your comments about --domain-needed moving and "--except-interface", "lo" changing to "--except-interface=lo" because it would mean changing many test files as well as many changes to the conf-file patch file. v8.3 of DHCPv6 done ... moving on to conf-file. Gene

On 12/03/2012 11:13 AM, Gene Czarcinski wrote:

This patch changes how parameters are passed to dnsmasq. Instead of being on the command line, the parameters are put into a file (one parameter per line) and a commandline --conf-file= specifies the location of the file. The file is located in the same directory as the leases file.

This also adds the dnsmasq parameter interface=<net-name>

Since my earlier patch for the CVE did that (when appropriate), this comment is obsolete.

Putting the dnsmasq parameters into a configuration file allows them to be examined and more easily understood than examining the command lines displayed by "ps ax". This is especially true when a number of networks have been started.

I suspect that when the use of dnsmasq was originally done, the command line was simple but has gotten more complicated over time and will likely become even more complicated in the future.

I believe that if use of dnsmasq was done today with the current requirements for dnsmasq, a configuration file would have been used. Many (most?) daemons use configuration files as oppose to large number of command line parameters.

One potential addition to dnsmasq parameters is to specify the reverse-lookup queries which are not to be passed on up the chain. For IPv4, such queries are rather simple and take the form: <ipv4_address>.in-addr.arpa but ipv6 queries involve a lot longer specification. The IPv6 query will be of the form: <ipv6_address>.ip6.arpa where the above query expands to a 44 character string which is specified by --local=/<ipv6_address>.ip6.arpa/ which is certainly a lot of clutter to add to the command line. --- src/network/bridge_driver.c | 190 +++++++++++++-------- src/network/bridge_driver.h | 7 +- tests/networkxml2argvdata/dhcp6-nat-network.argv | 29 ++-- tests/networkxml2argvdata/dhcp6-network.argv | 29 ++-- .../dhcp6host-routed-network.argv | 25 ++- tests/networkxml2argvdata/isolated-network.argv | 31 ++-- .../networkxml2argvdata/nat-network-dns-hosts.argv | 19 +-- .../nat-network-dns-srv-record-minimal.argv | 37 ++-- .../nat-network-dns-srv-record.argv | 27 ++- .../nat-network-dns-txt-record.argv | 26 +-- tests/networkxml2argvdata/nat-network.argv | 29 ++-- tests/networkxml2argvdata/netboot-network.argv | 37 ++-- .../networkxml2argvdata/netboot-proxy-network.argv | 33 ++-- tests/networkxml2argvdata/routed-network.argv | 15 +- tests/networkxml2argvtest.c | 40 +---- 15 files changed, 286 insertions(+), 288 deletions(-)

diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index c07d61a..2ccc695 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -142,6 +142,16 @@ networkDnsmasqLeaseFileNameFunc networkDnsmasqLeaseFileName = networkDnsmasqLeaseFileNameDefault;

static char * +networkDnsmasqConfigFileName(const char *netname) +{ + char *conffile; + + ignore_value(virAsprintf(&conffile, DNSMASQ_STATE_DIR "/%s.conf", + netname)); + return conffile; +} + +static char * networkRadvdPidfileBasename(const char *netname) { /* this is simple but we want to be sure it's consistently done */ @@ -168,6 +178,7 @@ networkRemoveInactive(struct network_driver *driver, { char *leasefile = NULL; char *radvdconfigfile = NULL; + char *configfile = NULL; char *radvdpidbase = NULL; dnsmasqContext *dctx = NULL; virNetworkDefPtr def = virNetworkObjGetPersistentDef(net); @@ -187,9 +198,13 @@ networkRemoveInactive(struct network_driver *driver, if (!(radvdpidbase = networkRadvdPidfileBasename(def->name))) goto no_memory;

+ if (!(configfile = networkDnsmasqConfigFileName(def->name))) + goto no_memory; + /* dnsmasq */ dnsmasqDelete(dctx); unlink(leasefile); + unlink(configfile);

/* radvd */ unlink(radvdconfigfile); @@ -202,6 +217,7 @@ networkRemoveInactive(struct network_driver *driver,

cleanup: VIR_FREE(leasefile); + VIR_FREE(configfile); VIR_FREE(radvdconfigfile); VIR_FREE(radvdpidbase); dnsmasqContextFree(dctx); @@ -635,12 +651,13 @@ networkBuildDnsmasqHostsList(dnsmasqContext *dctx,

static int -networkBuildDnsmasqArgv(virNetworkObjPtr network, +networkDnsmasqConfContents(virNetworkObjPtr network, const char *pidfile, - virCommandPtr cmd, + char **configstr, dnsmasqContext *dctx, dnsmasqCapsPtr caps ATTRIBUTE_UNUSED) { + virBuffer configbuf = VIR_BUFFER_INITIALIZER; int r, ret = -1; int nbleases = 0; int ii; @@ -651,46 +668,43 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, virNetworkIpDefPtr tmpipdef, ipdef, ipv4def, ipv6def; bool dhcp4flag, dhcp6flag, ipv6SLAAC;

+ *configstr = NULL; + /* - * NB, be careful about syntax for dnsmasq options in long format. - * - * If the flag has a mandatory argument, it can be given using - * either syntax: + * All dnsmasq parameters are now put into a configuration + * file except, of course, the command line --conf-file= + * which specified the name and where the configuration + * file is located.

"All dnsmasq parameters are put into a configuration file, except the --conf-file= parameter, which specifies the location of the configuration file."

* - * --foo bar - * --foo=bar + * Therefore, all parameters must now be specified as: * - * If the flag has a optional argument, it *must* be given using - * the syntax: - * - * --foo=bar - * - * It is hard to determine whether a flag is optional or not, - * without reading the dnsmasq source :-( The manpage is not - * very explicit on this. + * foo=bar

"All conf file parameters must be specified as "foo=bar" (as opposed to the "foo bar" form which is acceptable for dnsmasq commandline parameters".

*/

/* * Needed to ensure dnsmasq uses same algorithm for processing * multiple namedriver entries in /etc/resolv.conf as GLibC. */ - virCommandAddArgList(cmd, "--strict-order", - "--domain-needed", - NULL);

- 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 : ""); + /* create dnsmasq config file appropriate for this network */ + virBufferAddLit(&configbuf, + "# dnsmasq conf file created by libvirt\n"

We should be more verbose here, similar to what's done with the network and domain xml files: "WARNING: THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE\n" "OVERWRITTEN AND LOST. Changes to this configuration should be made using:\n" " virsh net-edit %s\n" "or other application using the libvirt API.\n", network->def->name

+ "strict-order\n" + "domain-needed\n");

- if (pidfile) - virCommandAddArgPair(cmd, "--pid-file", pidfile); - - /* *no* conf file */ - virCommandAddArg(cmd, "--conf-file="); + if (network->def->domain) { + virBufferAsprintf(&configbuf, + "domain=%s\n" + "expand-hosts\n", + network->def->domain); + } + /* need to specify local even if no domain specified */ + virBufferAsprintf(&configbuf, + "local=/%s/\n", + network->def->domain ? network->def->domain : ""); + + if (pidfile) + virBufferAsprintf(&configbuf, "pid-file=%s\n", pidfile);

if (dnsmasqCapsGet(caps, DNSMASQ_CAPS_BIND_DYNAMIC)) { /* using --bind-dynamic with only --interface (no @@ -700,15 +714,14 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, * other than one of the virtual guests connected directly to * this network). This was added in response to CVE 2012-3411. */ - virCommandAddArgList(cmd, - "--bind-dynamic", - "--interface", network->def->bridge, - NULL); + virBufferAsprintf(&configbuf, + "bind-dynamic\n" + "interface=%s\n", + network->def->bridge); } else { - virCommandAddArgList(cmd, - "--bind-interfaces", - "--except-interface=lo", - NULL); + virBufferAddLit(&configbuf, + "bind-interfaces\n" + "except-interface=lo\n"); /* * --interface does not actually work with dnsmasq < 2.47, * due to DAD for ipv6 addresses on the interface. @@ -725,7 +738,7 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, if (!ipaddr) goto cleanup; /* also part of CVE 2012-3411 - if the host's version of - * dnsmasq doesn't have --bind-dynamic, only allow listening on + * dnsmasq doesn't have bind-dynamic, only allow listening on * private/local IP addresses (see RFC1918/RFC3484/RFC4193) */ if (!virSocketAddrIsPrivate(&tmpipdef->address)) { @@ -734,7 +747,7 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Publicly routable address %s is prohibited. " "The version of dnsmasq on this host (%d.%d) doesn't " - "support the --bind-dynamic option, which is required " + "support the bind-dynamic option, which is required " "for safe operation on a publicly routable subnet " "(see CVE-2012-3411). You must either upgrade dnsmasq, " "or use a private/local subnet range for this network " @@ -742,21 +755,21 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, (int)version / 1000000, (int)(version % 1000000) / 1000); goto cleanup; } - virCommandAddArgList(cmd, "--listen-address", ipaddr, NULL); + virBufferAsprintf(&configbuf, "listen-address=%s\n", ipaddr); VIR_FREE(ipaddr); } }

/* If this is an isolated network, set the default route option * (3) to be empty to avoid setting a default route that's - * guaranteed to not work, and set --no-resolv so that no dns + * guaranteed to not work, and set no-resolv so that no dns * requests are forwarded on to the dns server listed in the * host's /etc/resolv.conf (since this could be used as a channel * to build a connection to the outside). */ if (network->def->forwardType == VIR_NETWORK_FORWARD_NONE) { - virCommandAddArgList(cmd, "--dhcp-option=3", - "--no-resolv", NULL); + virBufferAddLit(&configbuf, "dhcp-option=3\n" + "no-resolv\n"); }

if (network->def->dns != NULL) { @@ -764,7 +777,7 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, int i;

for (i = 0; i < dns->ntxtrecords; i++) { - virCommandAddArgFormat(cmd, "--txt-record=%s,%s", + virBufferAsprintf(&configbuf, "txt-record=%s,%s\n",

Does dnsmasq's conf file require quotes around this when the value has embedded spaces? (I'm assuming not, but that is one thing that virCommandAddArgFormat does that is no longer happening - if there are embedded spaces in the value, the *entire arg* is single-quoted).

dns->txtrecords[i].name, dns->txtrecords[i].value); } @@ -802,7 +815,7 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, goto cleanup; }

- virCommandAddArgPair(cmd, "--srv-host", record); + virBufferAsprintf(&configbuf, "srv-host=%s\n", record); VIR_FREE(record); VIR_FREE(recordPort); VIR_FREE(recordWeight); @@ -882,8 +895,8 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, virReportOOMError(); goto cleanup; } - virCommandAddArg(cmd, "--dhcp-range"); - virCommandAddArgFormat(cmd, "%s,%s", saddr, eaddr); + virBufferAsprintf(&configbuf, "dhcp-range=%s,%s\n", + saddr, eaddr); VIR_FREE(saddr); VIR_FREE(eaddr); nbleases += virSocketAddrGetRange(&ipdef->ranges[r].start, @@ -901,8 +914,7 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, virReportOOMError(); goto cleanup; } - virCommandAddArg(cmd, "--dhcp-range"); - virCommandAddArgFormat(cmd, "%s,static", bridgeaddr); + virBufferAsprintf(&configbuf, "dhcp-range=%s,static\n", bridgeaddr); VIR_FREE(bridgeaddr); }

@@ -912,16 +924,14 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, /* 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"); + virBufferAddLit(&configbuf, "dhcp-no-override\n");

if (ipdef->tftproot) { - virCommandAddArgList(cmd, "--enable-tftp", - "--tftp-root", ipdef->tftproot, - NULL); + virBufferAddLit(&configbuf, "enable-tftp\n"); + virBufferAsprintf(&configbuf, "tftp-root=%s\n", ipdef->tftproot); }

if (ipdef->bootfile) { - virCommandAddArg(cmd, "--dhcp-boot"); if (VIR_SOCKET_ADDR_VALID(&ipdef->bootserver)) { char *bootserver = virSocketAddrFormat(&ipdef->bootserver);

@@ -929,11 +939,11 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, virReportOOMError(); goto cleanup; } - virCommandAddArgFormat(cmd, "%s%s%s", + virBufferAsprintf(&configbuf, "dhcp-boot=%s%s%s\n", ipdef->bootfile, ",,", bootserver); VIR_FREE(bootserver); } else { - virCommandAddArg(cmd, ipdef->bootfile); + virBufferAsprintf(&configbuf, "dhcp-boot=%s\n", ipdef->bootfile); } } } @@ -949,9 +959,9 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, virReportOOMError(); goto cleanup; } - virCommandAddArgFormat(cmd, "--dhcp-leasefile=%s", leasefile); + virBufferAsprintf(&configbuf, "dhcp-leasefile=%s\n", leasefile); VIR_FREE(leasefile); - virCommandAddArgFormat(cmd, "--dhcp-lease-max=%d", nbleases); + virBufferAsprintf(&configbuf, "dhcp-lease-max=%d\n", nbleases); }

/* this is done once per interface */ @@ -963,19 +973,19 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, * file to allow for runtime additions. */ if (dhcp4flag || dhcp6flag) - virCommandAddArgPair(cmd, "--dhcp-hostsfile", - dctx->hostsfile->path); + virBufferAsprintf(&configbuf, "dhcp-hostsfile=%s\n", + dctx->hostsfile->path);

/* Likewise, always create this file and put it on the commandline, to allow for * for runtime additions. */ - virCommandAddArgPair(cmd, "--addn-hosts", - dctx->addnhostsfile->path); + virBufferAsprintf(&configbuf, "addn-hosts=%s\n", + dctx->addnhostsfile->path);

/* Are we doing RA instead of radvd? */ if (CHECK_VERSION_RA(caps)) { if (dhcp6flag) - virCommandAddArg(cmd, "--enable-ra"); + virBufferAddLit(&configbuf, "enable-ra\n"); else { for (ii = 0; (ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET6, ii)); @@ -983,8 +993,8 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, if (!(ipdef->nranges || ipdef->nhosts)) { char *bridgeaddr = virSocketAddrFormat(&ipdef->address); if (bridgeaddr) { - virCommandAddArgFormat(cmd, - "--dhcp-range=%s,ra-only", bridgeaddr); + virBufferAsprintf(&configbuf, + "dhcp-range=%s,ra-only\n", bridgeaddr); } else { virReportOOMError(); goto cleanup; @@ -995,9 +1005,15 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network, } }

+ if (!(*configstr = virBufferContentAndReset(&configbuf))) { + virReportOOMError();

This is incorrect - virBufferContentAndReset() doesn't attempt to allocate any memory; it only returns a pointer to memory that was already allocated. It will return an error only if some earlier virBuffer*() function had a problem, and in that case the error will already be set/logged. So, if it virBufferContentAndReset returns NULL, simply goto cleanup.

+ goto cleanup; + } + ret = 0;

cleanup: + virBufferFreeAndReset(&configbuf); VIR_FREE(record); VIR_FREE(recordPort); VIR_FREE(recordWeight); @@ -1005,13 +1021,17 @@ cleanup: return ret; }

+ /* build the dnsmasq command line */

strange indentation.

int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, char *pidfile, dnsmasqContext *dctx, - dnsmasqCapsPtr caps) + dnsmasqCapsPtr caps, + int testOnly, char **testConfigStr)

Instead of tricking out networkBuildDhcpDaemonCommandLine() to return after just calling networkDnsmasqConfContents() when you're calling from the test code, you should change the test code to directly call networkDnsmasqConfContents(). You'll need to add it to bridge_driver.h and src/libvirt_private.syms.

{ virCommandPtr cmd = NULL; - int ret = -1, ii; + int ret = -1; + char *configfile = NULL; + char *configstr = NULL;

network->dnsmasqPid = -1;

@@ -1024,11 +1044,33 @@ networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdou if (!virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, 0)) return 0;

- cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps)); - if (networkBuildDnsmasqArgv(network, pidfile, cmd, dctx, caps) < 0) { + if (networkDnsmasqConfContents(network, pidfile, &configstr, dctx, caps) < 0) + goto cleanup; + if (!configstr) goto cleanup; + + if (testOnly) { + *testConfigStr = configstr; + return 0; }

+ /* construct the filename */ + if (!(configfile = networkDnsmasqConfigFileName(network->def->name))) { + virReportOOMError(); + goto cleanup; + } + + /* Write the file */ + if (virFileWriteStr(configfile, configstr, 0600) < 0) { + virReportSystemError(errno, + _("couldn't write dnsmasq config file '%s'"), + configfile); + goto cleanup; + } + + cmd = virCommandNew(dnsmasqCapsGetBinaryPath(caps)); + virCommandAddArgFormat(cmd, "--conf-file=%s", configfile); + if (cmdout) *cmdout = cmd; ret = 0; @@ -1044,6 +1086,7 @@ networkStartDhcpDaemon(struct network_driver *driver, { virCommandPtr cmd = NULL; char *pidfile = NULL; + char *testconfigstr = NULL; int ret = -1; dnsmasqContext *dctx = NULL;

@@ -1084,8 +1127,9 @@ networkStartDhcpDaemon(struct network_driver *driver,

dnsmasqCapsRefresh(&driver->dnsmasqCaps, false);

- ret = networkBuildDhcpDaemonCommandLine(network, &cmd, pidfile, - dctx, driver->dnsmasqCaps); + ret = networkBuildDhcpDaemonCommandLine(network, &cmd, pidfile, dctx, + driver->dnsmasqCaps, + 0, &testconfigstr); if (ret < 0) goto cleanup;

diff --git a/src/network/bridge_driver.h b/src/network/bridge_driver.h index 8c16bdd..f727f6a 100644 --- a/src/network/bridge_driver.h +++ b/src/network/bridge_driver.h @@ -49,15 +49,16 @@ int networkGetNetworkAddress(const char *netname, char **netaddr) int networkBuildDhcpDaemonCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, char *pidfile, dnsmasqContext *dctx, - dnsmasqCapsPtr caps) - ; + dnsmasqCapsPtr caps, + int testOnly, char **testConfigStr); # else /* Define no-op replacements that don't drag in any link dependencies. */ # define networkAllocateActualDevice(iface) 0 # define networkNotifyActualDevice(iface) (iface=iface, 0) # define networkReleaseActualDevice(iface) (iface=iface, 0) # define networkGetNetworkAddress(netname, netaddr) (-2) -# define networkBuildDhcpDaemonCommandLine(network, cmdout, pidfile, dctx, caps) 0 +# define networkBuildDhcpDaemonCommandLine(network, cmdout, pidfile, \ + dctx, caps, testonly, testConfigStr) 0

Don't forget to change this back with you remove testonly and testConfigStr (you won't notice it when you try to build, but other platforms with no network driver will get a build error).

# endif

Again, I'm assuming that a successful pass of the test cases means that the changes to the test data is correct. You should rename all the .argv files to .conf though.

typedef char *(*networkDnsmasqLeaseFileNameFunc)(const char *netname); diff --git a/tests/networkxml2argvdata/dhcp6-nat-network.argv b/tests/networkxml2argvdata/dhcp6-nat-network.argv index df8c507..bdb64d6 100644 --- a/tests/networkxml2argvdata/dhcp6-nat-network.argv +++ b/tests/networkxml2argvdata/dhcp6-nat-network.argv @@ -1,15 +1,14 @@ -@DNSMASQ@ \ ---strict-order \ ---domain-needed \ ---local=// \ ---conf-file= \ ---bind-dynamic \ ---interface virbr0 \ ---dhcp-range 192.168.122.2,192.168.122.254 \ ---dhcp-no-override \ ---dhcp-range 2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff \ ---dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases \ ---dhcp-lease-max=493 \ ---dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts \ ---enable-ra\ +# dnsmasq conf file created by libvirt +strict-order +domain-needed +local=// +bind-dynamic +interface=virbr0 +dhcp-range=192.168.122.2,192.168.122.254 +dhcp-no-override +dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff +dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases +dhcp-lease-max=493 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts +enable-ra diff --git a/tests/networkxml2argvdata/dhcp6-network.argv b/tests/networkxml2argvdata/dhcp6-network.argv index 059c418..42c7b35 100644 --- a/tests/networkxml2argvdata/dhcp6-network.argv +++ b/tests/networkxml2argvdata/dhcp6-network.argv @@ -1,15 +1,14 @@ -@DNSMASQ@ \ ---strict-order \ ---domain-needed \ ---domain=mynet \ ---expand-hosts \ ---local=/mynet/ \ ---conf-file= \ ---bind-dynamic \ ---interface virbr0 \ ---dhcp-range 2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff \ ---dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases \ ---dhcp-lease-max=240 \ ---dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts \ ---enable-ra\ +# dnsmasq conf file created by libvirt +strict-order +domain-needed +domain=mynet +expand-hosts +local=/mynet/ +bind-dynamic +interface=virbr0 +dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff +dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases +dhcp-lease-max=240 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts +enable-ra diff --git a/tests/networkxml2argvdata/dhcp6host-routed-network.argv b/tests/networkxml2argvdata/dhcp6host-routed-network.argv index 8f6d7d3..9ee3c28 100644 --- a/tests/networkxml2argvdata/dhcp6host-routed-network.argv +++ b/tests/networkxml2argvdata/dhcp6host-routed-network.argv @@ -1,13 +1,12 @@ -@DNSMASQ@ \ ---strict-order \ ---domain-needed \ ---local=// \ ---conf-file= \ ---bind-dynamic \ ---interface virbr1 \ ---dhcp-range 192.168.122.1,static \ ---dhcp-no-override \ ---dhcp-range 2001:db8:ac10:fd01::1,static \ ---dhcp-hostsfile=/var/lib/libvirt/dnsmasq/local.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/local.addnhosts \ ---enable-ra\ +# dnsmasq conf file created by libvirt +strict-order +domain-needed +local=// +bind-dynamic +interface=virbr1 +dhcp-range=192.168.122.1,static +dhcp-no-override +dhcp-range=2001:db8:ac10:fd01::1,static +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/local.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/local.addnhosts +enable-ra diff --git a/tests/networkxml2argvdata/isolated-network.argv b/tests/networkxml2argvdata/isolated-network.argv index b0783bd..740e423 100644 --- a/tests/networkxml2argvdata/isolated-network.argv +++ b/tests/networkxml2argvdata/isolated-network.argv @@ -1,16 +1,15 @@ -@DNSMASQ@ \ ---strict-order \ ---domain-needed \ ---local=// \ ---conf-file= \ ---bind-interfaces \ ---except-interface=lo \ ---listen-address 192.168.152.1 \ ---dhcp-option=3 \ ---no-resolv \ ---dhcp-range 192.168.152.2,192.168.152.254 \ ---dhcp-no-override \ ---dhcp-leasefile=/var/lib/libvirt/dnsmasq/private.leases \ ---dhcp-lease-max=253 \ ---dhcp-hostsfile=/var/lib/libvirt/dnsmasq/private.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/private.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +domain-needed +local=// +bind-interfaces +except-interface=lo +listen-address=192.168.152.1 +dhcp-option=3 +no-resolv +dhcp-range=192.168.152.2,192.168.152.254 +dhcp-no-override +dhcp-leasefile=/var/lib/libvirt/dnsmasq/private.leases +dhcp-lease-max=253 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/private.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/private.addnhosts diff --git a/tests/networkxml2argvdata/nat-network-dns-hosts.argv b/tests/networkxml2argvdata/nat-network-dns-hosts.argv index fee137f..1a3df37 100644 --- a/tests/networkxml2argvdata/nat-network-dns-hosts.argv +++ b/tests/networkxml2argvdata/nat-network-dns-hosts.argv @@ -1,10 +1,9 @@ -@DNSMASQ@ \ ---strict-order \ ---domain-needed \ ---domain=example.com \ ---expand-hosts \ ---local=/example.com/ \ ---conf-file= \ ---bind-dynamic \ ---interface virbr0 \ ---addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +domain-needed +domain=example.com +expand-hosts +local=/example.com/ +bind-dynamic +interface=virbr0 +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv b/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv index 1de6637..67600f4 100644 --- a/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv +++ b/tests/networkxml2argvdata/nat-network-dns-srv-record-minimal.argv @@ -1,19 +1,18 @@ -@DNSMASQ@ \ ---strict-order \ ---domain-needed \ ---local=// \ ---conf-file= \ ---bind-interfaces \ ---except-interface=lo \ ---listen-address 192.168.122.1 \ ---listen-address 192.168.123.1 \ ---listen-address fc00:db8:ac10:fe01::1 \ ---listen-address fc00:db8:ac10:fd01::1 \ ---listen-address 10.24.10.1 \ ---srv-host=name.tcp.,,,, \ ---dhcp-range 192.168.122.2,192.168.122.254 \ ---dhcp-no-override \ ---dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases \ ---dhcp-lease-max=253 \ ---dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +domain-needed +local=// +bind-interfaces +except-interface=lo +listen-address=192.168.122.1 +listen-address=192.168.123.1 +listen-address=fc00:db8:ac10:fe01::1 +listen-address=fc00:db8:ac10:fd01::1 +listen-address=10.24.10.1 +srv-host=name.tcp.,,,, +dhcp-range=192.168.122.2,192.168.122.254 +dhcp-no-override +dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases +dhcp-lease-max=253 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2argvdata/nat-network-dns-srv-record.argv b/tests/networkxml2argvdata/nat-network-dns-srv-record.argv index e7ecaa5..c03e6d9 100644 --- a/tests/networkxml2argvdata/nat-network-dns-srv-record.argv +++ b/tests/networkxml2argvdata/nat-network-dns-srv-record.argv @@ -1,14 +1,13 @@ -@DNSMASQ@ \ ---strict-order \ ---domain-needed \ ---local=// \ ---conf-file= \ ---bind-dynamic \ ---interface virbr0 \ ---srv-host=name.tcp.test-domain-name,.,1024,10,10 \ ---dhcp-range 192.168.122.2,192.168.122.254 \ ---dhcp-no-override \ ---dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases \ ---dhcp-lease-max=253 \ ---dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +domain-needed +local=// +bind-dynamic +interface=virbr0 +srv-host=name.tcp.test-domain-name,.,1024,10,10 +dhcp-range=192.168.122.2,192.168.122.254 +dhcp-no-override +dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases +dhcp-lease-max=253 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv index 8ea0004..7dd9f3f 100644 --- a/tests/networkxml2argvdata/nat-network-dns-txt-record.argv +++ b/tests/networkxml2argvdata/nat-network-dns-txt-record.argv @@ -1,13 +1,13 @@ -@DNSMASQ@ \ ---strict-order \ ---domain-needed \ ---local=// \ ---conf-file= \ ---bind-dynamic --interface virbr0 \ -'--txt-record=example,example value' \ ---dhcp-range 192.168.122.2,192.168.122.254 \ ---dhcp-no-override \ ---dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases \ ---dhcp-lease-max=253 \ ---dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +domain-needed +local=// +bind-dynamic +interface=virbr0 +txt-record=example,example value +dhcp-range=192.168.122.2,192.168.122.254 +dhcp-no-override +dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases +dhcp-lease-max=253 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts diff --git a/tests/networkxml2argvdata/nat-network.argv b/tests/networkxml2argvdata/nat-network.argv index 578a5ff..e71f54b 100644 --- a/tests/networkxml2argvdata/nat-network.argv +++ b/tests/networkxml2argvdata/nat-network.argv @@ -1,15 +1,14 @@ -@DNSMASQ@ \ ---strict-order \ ---domain-needed \ ---local=// \ ---conf-file= \ ---bind-dynamic \ ---interface virbr0 \ ---dhcp-range 192.168.122.2,192.168.122.254 \ ---dhcp-no-override \ ---dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases \ ---dhcp-lease-max=253 \ ---dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts \ ---dhcp-range=2001:db8:ac10:fe01::1,ra-only \ ---dhcp-range=2001:db8:ac10:fd01::1,ra-only\ +# dnsmasq conf file created by libvirt +strict-order +domain-needed +local=// +bind-dynamic +interface=virbr0 +dhcp-range=192.168.122.2,192.168.122.254 +dhcp-no-override +dhcp-leasefile=/var/lib/libvirt/dnsmasq/default.leases +dhcp-lease-max=253 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/default.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/default.addnhosts +dhcp-range=2001:db8:ac10:fe01::1,ra-only +dhcp-range=2001:db8:ac10:fd01::1,ra-only diff --git a/tests/networkxml2argvdata/netboot-network.argv b/tests/networkxml2argvdata/netboot-network.argv index c5b75a3..df8df31 100644 --- a/tests/networkxml2argvdata/netboot-network.argv +++ b/tests/networkxml2argvdata/netboot-network.argv @@ -1,19 +1,18 @@ -@DNSMASQ@ \ ---strict-order \ ---domain-needed \ ---domain=example.com \ ---expand-hosts \ ---local=/example.com/ \ ---conf-file= \ ---bind-interfaces \ ---except-interface=lo \ ---listen-address 192.168.122.1 \ ---dhcp-range 192.168.122.2,192.168.122.254 \ ---dhcp-no-override \ ---enable-tftp \ ---tftp-root /var/lib/tftproot \ ---dhcp-boot pxeboot.img \ ---dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases \ ---dhcp-lease-max=253 \ ---dhcp-hostsfile=/var/lib/libvirt/dnsmasq/netboot.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/netboot.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +domain-needed +domain=example.com +expand-hosts +local=/example.com/ +bind-interfaces +except-interface=lo +listen-address=192.168.122.1 +dhcp-range=192.168.122.2,192.168.122.254 +dhcp-no-override +enable-tftp +tftp-root=/var/lib/tftproot +dhcp-boot=pxeboot.img +dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases +dhcp-lease-max=253 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/netboot.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/netboot.addnhosts diff --git a/tests/networkxml2argvdata/netboot-proxy-network.argv b/tests/networkxml2argvdata/netboot-proxy-network.argv index 16e1c85..a84d7a1 100644 --- a/tests/networkxml2argvdata/netboot-proxy-network.argv +++ b/tests/networkxml2argvdata/netboot-proxy-network.argv @@ -1,17 +1,16 @@ -@DNSMASQ@ \ ---strict-order \ ---domain-needed \ ---domain=example.com \ ---expand-hosts \ ---local=/example.com/ \ ---conf-file= \ ---bind-interfaces \ ---except-interface=lo \ ---listen-address 192.168.122.1 \ ---dhcp-range 192.168.122.2,192.168.122.254 \ ---dhcp-no-override \ ---dhcp-boot pxeboot.img,,10.20.30.40 \ ---dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases \ ---dhcp-lease-max=253 \ ---dhcp-hostsfile=/var/lib/libvirt/dnsmasq/netboot.hostsfile \ ---addn-hosts=/var/lib/libvirt/dnsmasq/netboot.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +domain-needed +domain=example.com +expand-hosts +local=/example.com/ +bind-interfaces +except-interface=lo +listen-address=192.168.122.1 +dhcp-range=192.168.122.2,192.168.122.254 +dhcp-no-override +dhcp-boot=pxeboot.img,,10.20.30.40 +dhcp-leasefile=/var/lib/libvirt/dnsmasq/netboot.leases +dhcp-lease-max=253 +dhcp-hostsfile=/var/lib/libvirt/dnsmasq/netboot.hostsfile +addn-hosts=/var/lib/libvirt/dnsmasq/netboot.addnhosts diff --git a/tests/networkxml2argvdata/routed-network.argv b/tests/networkxml2argvdata/routed-network.argv index b3fbf49..eaf4bd3 100644 --- a/tests/networkxml2argvdata/routed-network.argv +++ b/tests/networkxml2argvdata/routed-network.argv @@ -1,8 +1,7 @@ -@DNSMASQ@ \ ---strict-order \ ---domain-needed \ ---local=// \ ---conf-file= \ ---bind-dynamic \ ---interface virbr1 \ ---addn-hosts=/var/lib/libvirt/dnsmasq/local.addnhosts\ +# dnsmasq conf file created by libvirt +strict-order +domain-needed +local=// +bind-dynamic +interface=virbr1 +addn-hosts=/var/lib/libvirt/dnsmasq/local.addnhosts diff --git a/tests/networkxml2argvtest.c b/tests/networkxml2argvtest.c index a020db9..cc6be1c 100644 --- a/tests/networkxml2argvtest.c +++ b/tests/networkxml2argvtest.c @@ -15,37 +15,6 @@ #include "memory.h" #include "network/bridge_driver.h"

-/* Replace all occurrences of @token in @buf by @replacement and adjust size of - * @buf accordingly. Returns 0 on success and -1 on out-of-memory errors. */ -static int replaceTokens(char **buf, const char *token, const char *replacement) { - size_t token_start, token_end; - size_t buf_len, rest_len; - const size_t token_len = strlen(token); - const size_t replacement_len = strlen(replacement); - const int diff = replacement_len - token_len; - - buf_len = rest_len = strlen(*buf) + 1; - token_end = 0; - for (;;) { - char *match = strstr(*buf + token_end, token); - if (match == NULL) - break; - token_start = match - *buf; - rest_len -= token_start + token_len - token_end; - token_end = token_start + token_len; - buf_len += diff; - if (diff > 0) - if (VIR_REALLOC_N(*buf, buf_len) < 0) - return -1; - if (diff != 0) - memmove(*buf + token_end + diff, *buf + token_end, rest_len); - memcpy(*buf + token_start, replacement, replacement_len); - token_end += diff; - } - /* if diff < 0, we could shrink the buffer here... */ - return 0; -} - static int testCompareXMLToArgvFiles(const char *inxml, const char *outargv, dnsmasqCapsPtr caps) { @@ -65,9 +34,6 @@ testCompareXMLToArgvFiles(const char *inxml, const char *outargv, dnsmasqCapsPtr if (virtTestLoadFile(outargv, &outArgvData) < 0) goto fail;

- if (replaceTokens(&outArgvData, "@DNSMASQ@", DNSMASQ)) - goto fail; - if (!(dev = virNetworkDefParseString(inXmlData))) goto fail;

@@ -80,10 +46,8 @@ testCompareXMLToArgvFiles(const char *inxml, const char *outargv, dnsmasqCapsPtr if (dctx == NULL) goto fail;

- if (networkBuildDhcpDaemonCommandLine(obj, &cmd, pidfile, dctx, caps) < 0) - goto fail; - - if (!(actual = virCommandToString(cmd))) + if (networkBuildDhcpDaemonCommandLine(obj, &cmd, pidfile, + dctx, caps, 1, &actual) < 0) goto fail;

Again - this should directly call networkDnsmasqConfContents instead of networkBuildDhcpDaemonCommandLine.

if (STRNEQ(outArgvData, actual)) {

I'll ACK this once you've made the changes I've pointed out above (and rebased it on top of the updated versions of the other two patches).