[libvirt] [RFC] DHCP Relay agent functionality
Recently I needed to implement functionality to operate a DHCP relay instead of dnsmasq's DHCP server on the virtual networks managed by libvirt. The primary benefit of operating a relay is that the central DHCP and DNS servers for the physical network can manage IP address allocation and hostname resolution for the entire network and, potentially, expose VMs via hostname and IP address on the public network too, as well as being able to monitor VM instance creation for resource planning. I originally implemented this functionality on libvirt 0.9.8 included with Ubuntu 12.04 as a hack to simply disable dnsmasq entirely and use dhcp-helper[1] (the DHCP relay written by Simon Kelley, author of dnsmaq) although I've constructed it so that another relay-agent could be used. After proving it works well I've reworked the code on the current libvirt master branch. If it's thought to be of use to the project please let me know. [1] http://www.thekelleys.org.uk/dhcp-helper/ The following changes since commit 34f1a618a5c4507f27f3f467b723c9119c1db3c7: conf: Avoid leaking of RNG device definition (2013-02-25 22:31:11 +0100) are available in the git repository at: git://github.com/iam-TJ/libvirt.git dhcp_relay_network for you to fetch changes up to dfc0609403106712b205e60b53b29dc850cad68d: docs: Describe the <dhcp> 'enable' and 'relay' attributes (2013-02-27 20:35:06 +0000) ---------------------------------------------------------------- TJ (10): conf: DHCP - add state for DHCP Relay and On/Off switch conf: Network - add ability to read/write XML DHCP state conf: Network - add pointers to enabled virNetworkIpDef DHCP settings conf: Network - keep track of active DHCP stanza in virNetworkDef network: Bridge - use IPv4 and IPv6 active DHCP stanza pointers network: Bridge - Add support for a DHCP Relay Agent network: Bridge - don't offer dnsmasq DHCP services when DHCP relay is enabled configure: Add DHCPRELAY to the set of external program definitions Add copyright attribution for DHCP relay functionality docs: Describe the <dhcp> 'enable' and 'relay' attributes configure.ac | 4 +++ docs/formatnetwork.html.in | 22 +++++++++++++- src/conf/network_conf.c | 36 +++++++++++++++++++--- src/conf/network_conf.h | 7 +++++ src/network/bridge_driver.c | 215 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------------- 5 files changed, 229 insertions(+), 55 deletions(-)
On 02/27/2013 01:53 PM, TJ wrote:
Recently I needed to implement functionality to operate a DHCP relay instead of dnsmasq's DHCP server on the virtual networks managed by libvirt.
The primary benefit of operating a relay is that the central DHCP and DNS servers for the physical network can manage IP address allocation and hostname resolution for the entire network and, potentially, expose VMs via hostname and IP address on the public network too, as well as being able to monitor VM instance creation for resource planning.
I originally implemented this functionality on libvirt 0.9.8 included with Ubuntu 12.04 as a hack to simply disable dnsmasq entirely and use dhcp-helper[1] (the DHCP relay written by Simon Kelley, author of dnsmaq) although I've constructed it so that another relay-agent could be used. After proving it works well I've reworked the code on the current libvirt master branch. If it's thought to be of use to the project please let me know.
[1] http://www.thekelleys.org.uk/dhcp-helper/
The following changes since commit 34f1a618a5c4507f27f3f467b723c9119c1db3c7:
conf: Avoid leaking of RNG device definition (2013-02-25 22:31:11 +0100)
are available in the git repository at:
git://github.com/iam-TJ/libvirt.git dhcp_relay_network
for you to fetch changes up to dfc0609403106712b205e60b53b29dc850cad68d:
docs: Describe the <dhcp> 'enable' and 'relay' attributes (2013-02-27 20:35:06 +0000)
Sounds interesting. It would also help if you posted this series of patches directly to the list. While there might be people willing to download from your repository, it is hard for them to provide comments if your patches have not also appeared on the list. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Signed-off-by: TJ <linux@iam.tj> --- src/conf/network_conf.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index c509915..8400eab 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -144,6 +144,10 @@ struct _virNetworkIpDef { char *bootfile; virSocketAddr bootserver; + /* when false no DHCP server is started */ + bool dhcp_enabled; + /* when true ranges are ignored and a DHCP relay-agent started */ + bool dhcp_relay; }; typedef struct _virNetworkForwardIfDef virNetworkForwardIfDef; @@ -234,6 +238,7 @@ typedef virNetworkObj *virNetworkObjPtr; struct _virNetworkObj { virMutex lock; + pid_t dhcprelayPid; pid_t dnsmasqPid; pid_t radvdPid; unsigned int active : 1; -- 1.8.1.2.433.g9808ce0.dirty
On 02/27/2013 07:18 PM, TJ wrote:
Signed-off-by: TJ <linux@iam.tj>
Can you use a full legal name, instead of a two-letter pseudonym? Your commit message is missing some substance, such as a summary of what is being added.
--- src/conf/network_conf.h | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index c509915..8400eab 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -144,6 +144,10 @@ struct _virNetworkIpDef { char *bootfile; virSocketAddr bootserver; + /* when false no DHCP server is started */ + bool dhcp_enabled;
Your patch is whitespace-damaged. Did you sent it with 'git send-email'?
+ /* when true ranges are ignored and a DHCP relay-agent started */ + bool dhcp_relay; }; typedef struct _virNetworkForwardIfDef virNetworkForwardIfDef; @@ -234,6 +238,7 @@ typedef virNetworkObj *virNetworkObjPtr; struct _virNetworkObj { virMutex lock; + pid_t dhcprelayPid; pid_t dnsmasqPid;
Does your series ever allow dnsmasq and dhcprelay to run at the same time, or can we use a single pid_t field that covers the mutually exclusive choice of which helper is running based on the rest of the config?
pid_t radvdPid; unsigned int active : 1;
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 28/02/13 03:15, Eric Blake wrote:
On 02/27/2013 07:18 PM, TJ wrote:
Signed-off-by: TJ <linux@iam.tj>
Can you use a full legal name, instead of a two-letter pseudonym?
TJ is my full name.
Your commit message is missing some substance, such as a summary of what is being added.
The first line of the commit message is in the email subject, and describes the commit. "conf: DHCP - add state for DHCP Relay and On/Off switch"
--- src/conf/network_conf.h | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index c509915..8400eab 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -144,6 +144,10 @@ struct _virNetworkIpDef { char *bootfile; virSocketAddr bootserver; + /* when false no DHCP server is started */ + bool dhcp_enabled;
Your patch is whitespace-damaged. Did you sent it with 'git send-email'?
Yes. It barfed first time and added 9 emails into one post! Second time it seemed to be OK.
+ /* when true ranges are ignored and a DHCP relay-agent started */ + bool dhcp_relay; }; typedef struct _virNetworkForwardIfDef virNetworkForwardIfDef; @@ -234,6 +238,7 @@ typedef virNetworkObj *virNetworkObjPtr; struct _virNetworkObj { virMutex lock; + pid_t dhcprelayPid; pid_t dnsmasqPid;
Does your series ever allow dnsmasq and dhcprelay to run at the same time, or can we use a single pid_t field that covers the mutually exclusive choice of which helper is running based on the rest of the config?
When local DHCP server services are disabled dnsmasq is still launched since there are several non-DHCP settings in the generated config. In my test-bed for these patches dnsmasq and dhcp-helper will be started alongside each other. If this series is accepted I was intending to propose adding <dns enable='(yes|no)'/> in the same style used for <dhcp> so that dnsmasq can be totally disabled if un-needed.
On 02/27/2013 08:26 PM, TJ wrote:
On 28/02/13 03:15, Eric Blake wrote:
On 02/27/2013 07:18 PM, TJ wrote:
Signed-off-by: TJ <linux@iam.tj>
Can you use a full legal name, instead of a two-letter pseudonym?
TJ is my full name.
Whatever. It might be the pseudonym you prefer in open source, but I seriously doubt you come from a culture where your parents did not grant you a surname.
Your commit message is missing some substance, such as a summary of what is being added.
The first line of the commit message is in the email subject, and describes the commit.
"conf: DHCP - add state for DHCP Relay and On/Off switch"
The summary is nice, but we also want to see a sample of the XML that you are adding. See, for example, commit 1716e7a6.
Your patch is whitespace-damaged. Did you sent it with 'git send-email'?
Yes. It barfed first time and added 9 emails into one post! Second time it seemed to be OK.
I only see 2-10 on the second attempt. I didn't see a resend of 1. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 28/02/13 03:49, Eric Blake wrote:
On 02/27/2013 08:26 PM, TJ wrote:
On 28/02/13 03:15, Eric Blake wrote:
On 02/27/2013 07:18 PM, TJ wrote:
Signed-off-by: TJ <linux@iam.tj>
Can you use a full legal name, instead of a two-letter pseudonym?
TJ is my full name.
Whatever. It might be the pseudonym you prefer in open source, but I seriously doubt you come from a culture where your parents did not grant you a surname.
That's a problem with your perception. It is my full legal name as per passport, driving license, etc.
On 02/27/2013 08:49 PM, Eric Blake wrote:
Your commit message is missing some substance, such as a summary of what is being added.
The first line of the commit message is in the email subject, and describes the commit.
"conf: DHCP - add state for DHCP Relay and On/Off switch"
The summary is nice, but we also want to see a sample of the XML that you are adding. See, for example, commit 1716e7a6.
The summary in 6/10 is more what I was looking for; maybe it's worth squashing that commit and this one into a single patch? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/27/2013 10:26 PM, TJ wrote:
On 28/02/13 03:15, Eric Blake wrote:
Does your series ever allow dnsmasq and dhcprelay to run at the same time, or can we use a single pid_t field that covers the mutually exclusive choice of which helper is running based on the rest of the config?
When local DHCP server services are disabled dnsmasq is still launched since there are several non-DHCP settings in the generated config.
Right. a DNS server is run by default on any network that has an IP address defined.
In my test-bed for these patches dnsmasq and dhcp-helper will be started alongside each other.
If this series is accepted I was intending to propose adding <dns enable='(yes|no)'/>
You can consider that one pre-approved, as we've already discussed it to death and decided it was a good idea; just nobody has gotten around to doing it that way yet :-) (in other words, go ahead and write it; you'll be getting it off the todo lists of at least two of us.)
in the same style used for <dhcp> so that dnsmasq can be
Actually the enable attribute is totally unnecessary for <dhcp>, since an absence of <dhcp> implies that it is disabled (and indeed that is what already happens), and in the case of <dhcp relay='yes'/>, it is also implied (since a local dhcp server and dhcp relay are mutually exclusive). So, if there is no <dhcp> (or if it's <dhcp relay='yes'/>) and there is <dns enable='no'/>, then dnsmasq won't be run. The only reason we need an enable attribute for DNS is that the original default (when there is no <dns> element) is to enable the dns server, and we must preserve backward compatibility.
On 02/27/2013 09:18 PM, TJ wrote:
Signed-off-by: TJ <linux@iam.tj> --- src/conf/network_conf.h | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index c509915..8400eab 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -144,6 +144,10 @@ struct _virNetworkIpDef { char *bootfile; virSocketAddr bootserver; + /* when false no DHCP server is started */ + bool dhcp_enabled; + /* when true ranges are ignored and a DHCP relay-agent started */ + bool dhcp_relay;
I think Eric mentioned elsewhere that your patches are *extremely* small. At least both the data structure, parser, and formatter changes should go into a single patch. For that matter, this functionality is small enough that you could put the struct change, parser change, implementation of the feature in bridge_driver, makefile and configure changes, AND the documentation addition into a single patch.
}; typedef struct _virNetworkForwardIfDef virNetworkForwardIfDef; @@ -234,6 +238,7 @@ typedef virNetworkObj *virNetworkObjPtr; struct _virNetworkObj { virMutex lock; + pid_t dhcprelayPid; pid_t dnsmasqPid; pid_t radvdPid; unsigned int active : 1;
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 28/02/13 19:45, Laine Stump wrote: [...sni[...]
I think Eric mentioned elsewhere that your patches are *extremely* small. At least both the data structure, parser, and formatter changes should go into a single patch. For that matter, this functionality is small enough that you could put the struct change, parser change, implementation of the feature in bridge_driver, makefile and configure changes, AND the documentation addition into a single patch.
Yes. Remember this an RFC so to some extend I'm making it easier to document (in commit messages as well as logical separation of the patches), the way I thought about the problem for myself as much as anyone else. When I'm developing a feature for some package that is foreign to me I prefer to break each focus of change into the actual hacking style I follow, which allows me to go back and change patches that have a single focus without changing related code (which may also be in a related branch where I'm testing alternate functionality) using, f.e: git rebase -i HEAD^^^^^^ s/^(pick) (49fe52|3f98634)/edit \1/ while $SOME_FILE; do vim $SOME_FILE git commit --amend git rebase --continue done Once the patch-set works and has positive feedback I can squash cherry-picked commits into a new branch for the final: git request-pull branch_x
From: "TJ" <libvirt@iam.tj> Maintain backwards XML compatibility by assuming existing default values and only adding the additional XML properties if settings are not default. Signed-off-by: TJ <linux@iam.tj> --- src/conf/network_conf.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 3fc01cf..259de0a 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -828,6 +828,19 @@ virNetworkDHCPDefParseXML(const char *networkName, { xmlNodePtr cur; + char *tmp = NULL; + + def->dhcp_enabled = true; + if ((tmp = virXMLPropString(node, "enabled"))) { + def->dhcp_enabled = strncmp("no", tmp, 2) == 0 ? false : def->dhcp_enabled; + VIR_FREE(tmp); + } + + def->dhcp_relay = false; + if ((tmp = virXMLPropString(node, "relay"))) { + def->dhcp_relay = strncmp("yes", tmp, 3) == 0 ? true : def->dhcp_relay; + VIR_FREE(tmp); + } cur = node->children; while (cur != NULL) { @@ -2180,12 +2193,19 @@ virNetworkIpDefFormat(virBufferPtr buf, virBufferEscapeString(buf, "<tftp root='%s' />\n", def->tftproot); } - if ((def->nranges || def->nhosts)) { + if ((def->nranges || def->nhosts) || + !def->dhcp_enabled || def->dhcp_relay) { int ii; - virBufferAddLit(buf, "<dhcp>\n"); + virBufferAddLit(buf, "<dhcp"); + if (!def->dhcp_enabled) + virBufferAddLit(buf, " enabled='no'"); + if (def->dhcp_relay) + virBufferAddLit(buf, " relay='yes'"); + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); - for (ii = 0 ; ii < def->nranges ; ii++) { + for (ii = 0 ; def->nranges && ii < def->nranges ; ii++) { char *saddr = virSocketAddrFormat(&def->ranges[ii].start); if (!saddr) goto error; @@ -2199,7 +2219,7 @@ virNetworkIpDefFormat(virBufferPtr buf, VIR_FREE(saddr); VIR_FREE(eaddr); } - for (ii = 0 ; ii < def->nhosts ; ii++) { + for (ii = 0 ; def->nhosts && ii < def->nhosts ; ii++) { virBufferAddLit(buf, "<host "); if (def->hosts[ii].mac) virBufferAsprintf(buf, "mac='%s' ", def->hosts[ii].mac); -- 1.8.1.2.433.g9808ce0.dirty
From 79a655340f1febc7c35ea4e0a7e0f30ec03e4795 Mon Sep 17 00:00:00 2001 In-Reply-To: <512E724A.10805@iam.tj> References: <512E724A.10805@iam.tj> From: "TJ" <libvirt@iam.tj> Date: Tue, 26 Feb 2013 17:11:52 +0000 Subject: [PATCH 03/10] conf: Network - add pointers to enabled virNetworkIpDef DHCP settings To: libvir-list@redhat.com
Having previously introduced DHCP enabled and relay state within the virNetworkIpDef structure - which can be one of many on each network - these pointers allow us to track and easily access the DHCP state for IPv4 and IPv6 when setting up the network without having to iterate every virNetworkIpDef to find the DHCP state. Signed-off-by: TJ <linux@iam.tj> --- src/conf/network_conf.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 8400eab..1889c45 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -231,6 +231,8 @@ struct _virNetworkDef { virPortGroupDefPtr portGroups; virNetDevBandwidthPtr bandwidth; virNetDevVlan vlan; + virNetworkIpDefPtr ipv4_dhcp; + virNetworkIpDefPtr ipv6_dhcp; }; typedef struct _virNetworkObj virNetworkObj; -- 1.8.1.2.433.g9808ce0.dirty
From 5ce7f49d7e0ebf0eeb9595d305c2b70895f4e4db Mon Sep 17 00:00:00 2001 In-Reply-To: <512E724A.10805@iam.tj> References: <512E724A.10805@iam.tj> From: "TJ" <libvirt@iam.tj> Date: Tue, 26 Feb 2013 17:14:36 +0000 Subject: [PATCH 04/10] conf: Network - keep track of active DHCP stanza in virNetworkDef To: libvir-list@redhat.com
From ed60402970098706b49e3d71c008520168968277 Mon Sep 17 00:00:00 2001 In-Reply-To: <512E724A.10805@iam.tj> References: <512E724A.10805@iam.tj> From: "TJ" <libvirt@iam.tj> Date: Tue, 26 Feb 2013 17:43:46 +0000 Subject: [PATCH 05/10] network: Bridge - use IPv4 and IPv6 active DHCP stanza
To avoid iterating all virNetworkIpDef entries when determining DHCP state keep track of the first enabled DHCP stanza in the network definition itself, for both IPv4 and IPv6. A by-product of this change is it allows the XML to contain more than one IP->DHCP stanza. The active DHCP stanza is the first enabled DHCP stanza. All other stanzas are retained which adds flexibility when multiple interfaces and routes might come and go since an alternative DHCP stanza can be selected and a refresh operation performed without needing to destroy/edit/start the network. Signed-off-by: TJ <linux@iam.tj> --- src/conf/network_conf.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 259de0a..c5eab01 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1882,6 +1882,13 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) if (ret < 0) goto error; def->nips++; + /* use only the first enabled DHCP definition */ + if (!def->ipv4_dhcp && def->ips[ii].dhcp_enabled && + VIR_SOCKET_ADDR_IS_FAMILY(&def->ips[ii].address, AF_INET)) + def->ipv4_dhcp = &def->ips[ii]; + if (!def->ipv6_dhcp && def->ips[ii].dhcp_enabled && + VIR_SOCKET_ADDR_IS_FAMILY(&def->ips[ii].address, AF_INET6)) + def->ipv6_dhcp = &def->ips[ii]; } } VIR_FREE(ipNodes); -- 1.8.1.2.433.g9808ce0.dirty pointers To: libvir-list@redhat.com Rather than iterate through virNetworkIPDef arrays multiple times use the new virNetworkDef ipv4_dhcp and ipv6_dhcp active stanza pointers. Signed-off-by: TJ <linux@iam.tj> --- src/network/bridge_driver.c | 63 +++++++++++---------------------------------- 1 file changed, 15 insertions(+), 48 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0932cf8..8410c93 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -810,24 +810,16 @@ networkDnsmasqConfContents(virNetworkObjPtr network, } } - /* Find the first dhcp for both IPv4 and IPv6 */ - for (ii = 0, ipv4def = NULL, ipv6def = NULL, 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; - } - } - } - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) { - if (ipdef->nranges || ipdef->nhosts) { + ipv4def = ipv6def = NULL; + ipdef = network->def->ipv4_dhcp; + if (ipdef && (ipdef->nranges || ipdef->nhosts)) + ipv4def = ipdef; + + ipdef = network->def->ipv6_dhcp; + if (ipdef) { + if (ipdef->nranges || ipdef->nhosts) { + ipv6def = ipdef; + if (!DNSMASQ_DHCPv6_SUPPORT(caps)) { unsigned long version = dnsmasqCapsGetVersion(caps); virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -842,18 +834,9 @@ networkDnsmasqConfContents(virNetworkObjPtr network, DNSMASQ_DHCPv6_MINOR_REQD); goto cleanup; } - if (ipv6def) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("For IPv6, multiple DHCP definitions " - "cannot be specified.")); - goto cleanup; - } else { - ipv6def = ipdef; - } - } else { - ipv6SLAAC = true; - } - } + } + } else { + ipv6SLAAC = true; } if (ipv6def && ipv6SLAAC) { @@ -1160,25 +1143,9 @@ networkRefreshDhcpDaemon(struct network_driver *driver, 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 (!ipv4def && (ipdef->nranges || ipdef->nhosts)) - ipv4def = ipdef; - } + ipv4def = network->def->ipv4_dhcp; - ipv6def = NULL; - for (ii = 0; - (ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET6, ii)); - ii++) { - if (!ipv6def && (ipdef->nranges || ipdef->nhosts)) - ipv6def = ipdef; - } + ipv6def = network->def->ipv6_dhcp; if (ipv4def && (networkBuildDnsmasqDhcpHostsList(dctx, ipv4def) < 0)) goto cleanup; -- 1.8.1.2.433.g9808ce0.dirty
From bc6995ebc1ea078b10c574df3f9264d0053627d2 Mon Sep 17 00:00:00 2001 In-Reply-To: <512E724A.10805@iam.tj> References: <512E724A.10805@iam.tj> From: "TJ" <libvirt@iam.tj> Date: Tue, 26 Feb 2013 20:26:58 +0000 Subject: [PATCH 06/10] network: Bridge - Add support for a DHCP Relay Agent To: libvir-list@redhat.com
A DHCP relay daemon will be started that will forward all DHCP/BOOTP requests on the bridge network via the first declared forward interface. The relay is broadcast rather than routed so no IP address is needed on the bridge. The XML <relay> element's "relay" property of the active DHCP stanza defaults to 'no'. Set it to 'yes' to enable the relay: <ip ...> <dhcp relay='yes'/> </ip> The relay will not be started if the "enable" property is 'no': <ip ...> <dhcp enable='no' relay='yes'/> </ip> Signed-off-by: TJ <linux@iam.tj> --- src/network/bridge_driver.c | 146 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 146 insertions(+) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8410c93..c02d3de 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -587,6 +587,145 @@ cleanup: * which is later saved into a file */ +static virNetworkIpDefPtr +networkGetActiveDhcp(virNetworkObjPtr network) +{ + virNetworkIpDefPtr dhcp = NULL; + + if (network->def && network->def->ipv4_dhcp) + dhcp = network->def->ipv4_dhcp; + + if (!dhcp && + network->def && network->def->ipv6_dhcp) + dhcp = network->def->ipv6_dhcp; + + return dhcp; +} + +static int +networkBuildDhcpRelayArgv(virNetworkObjPtr network, + const char *pidfile, + virCommandPtr cmd) +{ + int ret = -1; + + /* PID file */ + virCommandAddArgList(cmd, "-r", pidfile, NULL); + + /* Listen for DHCP requests on the bridge interface */ + virCommandAddArgList(cmd, "-i", network->def->bridge, NULL); + + /* Use the first forwarding device to broadcast to the upstream DHCP server */ + if (network->def->forward.nifs > 0) { + virCommandAddArgList(cmd, "-b", network->def->forward.ifs[0].device.dev, NULL); + ret = 0; + } else + virReportSystemError(VIR_ERR_INVALID_INTERFACE, + _("DHCP relay requires at least one network %s\n"), + "<forward ... dev='eth?'/> or <interface dev='eth?'/>"); + + return ret; +} + +static int +networkBuildDhcpRelayCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, + char *pidfile) +{ + virCommandPtr cmd = NULL; + int ret = -1; + + cmd = virCommandNew(DHCPRELAY); + if (networkBuildDhcpRelayArgv(network, pidfile, cmd) < 0) { + goto cleanup; + } + + if (cmdout) + *cmdout = cmd; + ret = 0; +cleanup: + if (ret < 0) + virCommandFree(cmd); + return ret; +} + +static int +networkStartDhcpRelayDaemon(struct network_driver *driver ATTRIBUTE_UNUSED, + virNetworkObjPtr network) +{ + virCommandPtr cmd = NULL; + virNetworkIpDefPtr ipdef = NULL; + char *pidfile = NULL; + char *tmp = NULL; + int pid_len; + int ret = 0; + const char *dhcprelay = "dhcprelay_"; + + ipdef = networkGetActiveDhcp(network); + /* Prepare for DHCP relay agent */ + if (ipdef && ipdef->dhcp_enabled && ipdef->dhcp_relay) { + ret = -1; + + if (virFileMakePath(NETWORK_PID_DIR) < 0) { + virReportSystemError(errno, + _("cannot create directory %s"), + NETWORK_PID_DIR); + goto cleanup; + } + + pid_len = strlen(dhcprelay) + strlen(network->def->name); + if ( VIR_ALLOC_N(tmp, pid_len+1) >= 0) { + tmp = strcpy(tmp, dhcprelay); + tmp = strncat(tmp, network->def->name, pid_len); + if (!(pidfile = virPidFileBuildPath(NETWORK_PID_DIR, tmp))) { + virReportOOMError(); + goto cleanup; + } + } else { + virReportOOMError(); + goto cleanup; + } + + ret = networkBuildDhcpRelayCommandLine(network, &cmd, pidfile); + if (ret < 0) + goto cleanup; + + ret = virCommandRun(cmd, NULL); + if (ret < 0) + goto cleanup; + + ret = virPidFileRead(NETWORK_PID_DIR, pidfile, &network->dhcprelayPid); + if (ret < 0) + virReportSystemError(errno, _("%s is not running"), DHCPRELAY); + +cleanup: + VIR_FREE(tmp); + VIR_FREE(pidfile); + virCommandFree(cmd); + } + return ret; +} + +static int +networkRestartDhcpRelayDaemon(struct network_driver *driver, + virNetworkObjPtr network) +{ + /* if there is a running DHCP relay agent, kill it */ + if (network->dhcprelayPid > 0) { + networkKillDaemon(network->dhcprelayPid, DHCPRELAY, + network->def->name); + network->dhcprelayPid = -1; + } + /* now start the daemon if it should be started */ + return networkStartDhcpRelayDaemon(driver, network); +} + +static int +networkRefreshDhcpRelayDaemon(struct network_driver *driver, + virNetworkObjPtr network) +{ + return networkRestartDhcpRelayDaemon(driver, network); +} + static int networkBuildDnsmasqDhcpHostsList(dnsmasqContext *dctx, virNetworkIpDefPtr ipdef) @@ -1496,6 +1635,7 @@ networkRefreshDaemons(struct network_driver *driver) * dnsmasq and/or radvd, or restart them if they've * disappeared. */ + networkRefreshDhcpRelayDaemon(driver, network); networkRefreshDhcpDaemon(driver, network); networkRefreshRadvd(driver, network); } @@ -2462,6 +2602,10 @@ networkStartNetworkVirtual(struct network_driver *driver, networkStartDhcpDaemon(driver, network) < 0) goto err3; + /* start DHCP relay-agent (doesn't need IP address(es) to function) */ + if (networkStartDhcpRelayDaemon(driver, network) < 0) + goto err3; + /* start radvd if there are any ipv6 addresses */ if (v6present && networkStartRadvd(driver, network) < 0) goto err4; @@ -3276,6 +3420,8 @@ networkUpdate(virNetworkPtr net, */ if (networkRestartDhcpDaemon(driver, network) < 0) goto cleanup; + if (networkRestartDhcpRelayDaemon(driver, network) < 0) + goto cleanup; } else if (section == VIR_NETWORK_SECTION_IP_DHCP_HOST) { /* if we previously weren't listening for dhcp and now we -- 1.8.1.2.433.g9808ce0.dirty
From 94c8cf891f83fd26b1ba608ed6be4315058493dd Mon Sep 17 00:00:00 2001 In-Reply-To: <512E724A.10805@iam.tj> References: <512E724A.10805@iam.tj> From: "TJ" <libvirt@iam.tj> Date: Tue, 26 Feb 2013 21:22:47 +0000 Subject: [PATCH 07/10] network: Bridge - don't offer dnsmasq DHCP services when DHCP relay is enabled To: libvir-list@redhat.com
When dnsmasq's DNS services are required but the network is configured to use a DHCP relay agent (other than dnsmasq's proxy services) the configuration generated for dnsmasq should not enable DHCP services. Signed-off-by: TJ <linux@iam.tj> --- src/network/bridge_driver.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index c02d3de..a4cd727 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -951,11 +951,12 @@ networkDnsmasqConfContents(virNetworkObjPtr network, ipv4def = ipv6def = NULL; ipdef = network->def->ipv4_dhcp; - if (ipdef && (ipdef->nranges || ipdef->nhosts)) + if (ipdef && ipdef->dhcp_enabled && !ipdef->dhcp_relay && + (ipdef->nranges || ipdef->nhosts)) ipv4def = ipdef; ipdef = network->def->ipv6_dhcp; - if (ipdef) { + if (ipdef && ipdef->dhcp_enabled && !ipdef->dhcp_relay) { if (ipdef->nranges || ipdef->nhosts) { ipv6def = ipdef; @@ -1266,8 +1267,8 @@ static int networkRefreshDhcpDaemon(struct network_driver *driver, virNetworkObjPtr network) { - int ret = -1, ii; - virNetworkIpDefPtr ipdef, ipv4def, ipv6def; + int ret = -1; + virNetworkIpDefPtr ipv4def, ipv6def; dnsmasqContext *dctx = NULL; /* if no IP addresses specified, nothing to do */ -- 1.8.1.2.433.g9808ce0.dirty
From e8dfe2a0b5945dbd71e3a7d6fdbbf9457fae9063 Mon Sep 17 00:00:00 2001 In-Reply-To: <512E724A.10805@iam.tj> References: <512E724A.10805@iam.tj> From: "TJ" <libvirt@iam.tj> Date: Tue, 26 Feb 2013 21:35:05 +0000 Subject: [PATCH 08/10] configure: Add DHCPRELAY to the set of external program definitions To: libvir-list@redhat.com
This variable should name the path to the system's DHCP relay daemon. At this time the expected daemon is "dhcp-helper", a DHCP relay agent from Simon Kelly, author of dnsmasq. The supporting code, however, has been designed to work with any suitable DHCP relay agent. Later patches could allow configuration of the agent's command-line arguments. Currently the chosen agent must support: -b <bridge_interface> (the virtual network to listen on) -i <physical_interface> (the interface to broadcast on) -r </path/to/pid.file> (the PID file of the daemon) Signed-off-by: TJ <linux@iam.tj> --- configure.ac | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/configure.ac b/configure.ac index e3a749a..b62170e 100644 --- a/configure.ac +++ b/configure.ac @@ -295,6 +295,8 @@ dnl External programs that we can use if they are available. dnl We will hard-code paths to these programs unless we cannot dnl detect them, in which case we'll search for the program dnl along the $PATH at runtime and fail if it's not there. +AC_PATH_PROG([DHCPRELAY], [dhcp-helper], [dhcp-helper], + [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_PATH_PROG([DNSMASQ], [dnsmasq], [dnsmasq], [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_PATH_PROG([RADVD], [radvd], [radvd], @@ -314,6 +316,8 @@ AC_PATH_PROG([OVSVSCTL], [ovs-vsctl], [ovs-vsctl], AC_PATH_PROG([SCRUB], [scrub], [scrub], [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) +AC_DEFINE_UNQUOTED([DHCPRELAY], ["$DHCPRELAY"], + [Location or name of the dhcp relay-agent program]) AC_DEFINE_UNQUOTED([DNSMASQ],["$DNSMASQ"], [Location or name of the dnsmasq program]) AC_DEFINE_UNQUOTED([RADVD],["$RADVD"], -- 1.8.1.2.433.g9808ce0.dirty
From 69128632689e26cc281288fb556cf76e77f441fb Mon Sep 17 00:00:00 2001 In-Reply-To: <512E724A.10805@iam.tj> References: <512E724A.10805@iam.tj> From: "TJ" <libvirt@iam.tj> Date: Wed, 27 Feb 2013 03:51:28 +0000 Subject: [PATCH 09/10] Add copyright attribution for DHCP relay functionality To: libvir-list@redhat.com
Signed-off-by: TJ <linux@iam.tj> --- src/conf/network_conf.c | 1 + src/network/bridge_driver.c | 1 + 2 files changed, 2 insertions(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index c5eab01..43c277c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3,6 +3,7 @@ * * Copyright (C) 2006-2013 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange + * Copyright (C) 2013 TJ <linux@iam.tj> * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index a4cd727..1667ae6 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4,6 +4,7 @@ * * Copyright (C) 2006-2013 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange + * Copyright (C) 2013 TJ <linux@iam.tj> * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public -- 1.8.1.2.433.g9808ce0.dirty
From c68b5a4aa5d28929e86a3e9227dba6c64df90e71 Mon Sep 17 00:00:00 2001 In-Reply-To: <512E724A.10805@iam.tj> References: <512E724A.10805@iam.tj> From: "TJ" <libvirt@iam.tj> Date: Wed, 27 Feb 2013 20:35:06 +0000 Subject: [PATCH 10/10] docs: Describe the <dhcp> 'enable' and 'relay' attributes To: libvir-list@redhat.com
Signed-off-by: TJ <linux@iam.tj> --- docs/formatnetwork.html.in | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 41a83fa..c4c4def 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -650,12 +650,20 @@ <dt><code>dhcp</code></dt> <dd>Also within the <code>ip</code> element there is an optional <code>dhcp</code> element. The presence of this element - enables DHCP services on the virtual network. It will further + enables DHCP services on the virtual network. It can further contain one or more <code>range</code> elements. The <code>dhcp</code> element supported for both IPv4 <span class="since">Since 0.3.0</span> and IPv6 <span class="since">Since 1.0.1</span>, but only for one IP address of each type per network. + Since $TODO.$FIXME it can optionally contain a boolean <code>enable</code> attribute + ('yes' or 'no') where the value defaults to 'yes', and a boolean + <code>relay</code> attribute where the value defaults to 'no'. + When <code>relay='yes'</code> any settings within the <code>dhcp</code> block + are ignored and a DHCP relay agent is started instead of a local DHCP server. + The DHCP relay daemon will listen on the network's bridge interface for + DHCP/BOOTP traffic and relay it via broadcast from the first interface declared + in the <code>forward</code> element. <dl> <dt><code>range</code></dt> <dd>The <code>start</code> and <code>end</code> attributes on the @@ -779,6 +787,18 @@ <ip family="ipv6" address="2001:db8:ca2:2::1" prefix="64" /> </network></pre> + <p>Here is the same configuration using a DHCP relay agent instead of a local DHCP server.</p> + <pre> + <network> + <name>local</name> + <bridge name="virbr1" /> + <forward mode="route" dev="eth1"/> + <ip address="192.168.122.1" netmask="255.255.255.0"> + <dhcp relay='yes'/> + </ip> + <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. -- 1.8.1.2.433.g9808ce0.dirty
From: TJ <linux@iam.tj> Maintain backwards XML compatibility by assuming existing default values and only adding the additional XML properties if settings are not default. Signed-off-by: TJ <linux@iam.tj> --- src/conf/network_conf.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 3fc01cf..259de0a 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -828,6 +828,19 @@ virNetworkDHCPDefParseXML(const char *networkName, { xmlNodePtr cur; + char *tmp = NULL; + + def->dhcp_enabled = true; + if ((tmp = virXMLPropString(node, "enabled"))) { + def->dhcp_enabled = strncmp("no", tmp, 2) == 0 ? false : def->dhcp_enabled; + VIR_FREE(tmp); + } + + def->dhcp_relay = false; + if ((tmp = virXMLPropString(node, "relay"))) { + def->dhcp_relay = strncmp("yes", tmp, 3) == 0 ? true : def->dhcp_relay; + VIR_FREE(tmp); + } cur = node->children; while (cur != NULL) { @@ -2180,12 +2193,19 @@ virNetworkIpDefFormat(virBufferPtr buf, virBufferEscapeString(buf, "<tftp root='%s' />\n", def->tftproot); } - if ((def->nranges || def->nhosts)) { + if ((def->nranges || def->nhosts) || + !def->dhcp_enabled || def->dhcp_relay) { int ii; - virBufferAddLit(buf, "<dhcp>\n"); + virBufferAddLit(buf, "<dhcp"); + if (!def->dhcp_enabled) + virBufferAddLit(buf, " enabled='no'"); + if (def->dhcp_relay) + virBufferAddLit(buf, " relay='yes'"); + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2); - for (ii = 0 ; ii < def->nranges ; ii++) { + for (ii = 0 ; def->nranges && ii < def->nranges ; ii++) { char *saddr = virSocketAddrFormat(&def->ranges[ii].start); if (!saddr) goto error; @@ -2199,7 +2219,7 @@ virNetworkIpDefFormat(virBufferPtr buf, VIR_FREE(saddr); VIR_FREE(eaddr); } - for (ii = 0 ; ii < def->nhosts ; ii++) { + for (ii = 0 ; def->nhosts && ii < def->nhosts ; ii++) { virBufferAddLit(buf, "<host "); if (def->hosts[ii].mac) virBufferAsprintf(buf, "mac='%s' ", def->hosts[ii].mac); -- 1.8.1.2.433.g9808ce0.dirty
From: TJ <linux@iam.tj> Having previously introduced DHCP enabled and relay state within the virNetworkIpDef structure - which can be one of many on each network - these pointers allow us to track and easily access the DHCP state for IPv4 and IPv6 when setting up the network without having to iterate every virNetworkIpDef to find the DHCP state. Signed-off-by: TJ <linux@iam.tj> --- src/conf/network_conf.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 8400eab..1889c45 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -231,6 +231,8 @@ struct _virNetworkDef { virPortGroupDefPtr portGroups; virNetDevBandwidthPtr bandwidth; virNetDevVlan vlan; + virNetworkIpDefPtr ipv4_dhcp; + virNetworkIpDefPtr ipv6_dhcp; }; typedef struct _virNetworkObj virNetworkObj; -- 1.8.1.2.433.g9808ce0.dirty
On 02/27/2013 07:57 PM, TJ wrote:
From: TJ <linux@iam.tj>
Having previously introduced DHCP enabled and relay state within the virNetworkIpDef structure - which can be one of many on each network - these pointers allow us to track and easily access the DHCP state for IPv4 and IPv6 when setting up the network without having to iterate every virNetworkIpDef to find the DHCP state.
This patch is useless in isolation; it's small enough that it is probably worth squashing in with the first patch to actual use these members.
Signed-off-by: TJ <linux@iam.tj> --- src/conf/network_conf.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 8400eab..1889c45 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -231,6 +231,8 @@ struct _virNetworkDef { virPortGroupDefPtr portGroups; virNetDevBandwidthPtr bandwidth; virNetDevVlan vlan; + virNetworkIpDefPtr ipv4_dhcp; + virNetworkIpDefPtr ipv6_dhcp; };
typedef struct _virNetworkObj virNetworkObj;
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/27/2013 09:57 PM, TJ wrote:
From: TJ <linux@iam.tj>
Having previously introduced DHCP enabled and relay state within the virNetworkIpDef structure - which can be one of many on each network - these pointers allow us to track and easily access the DHCP state for IPv4 and IPv6 when setting up the network without having to iterate every virNetworkIpDef to find the DHCP state.
I'm not sure I like this. Having these convenience pointers is, er, convenient, but it also means that you must maintain them, for example during virNetworkUpdate* (a series of calls to this could potentially remove all dhcp info from one IP address, and add it into another IP address). That means more potential for getting it out of synce due to missing a change in some obscure place.
Signed-off-by: TJ <linux@iam.tj> --- src/conf/network_conf.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index 8400eab..1889c45 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -231,6 +231,8 @@ struct _virNetworkDef { virPortGroupDefPtr portGroups; virNetDevBandwidthPtr bandwidth; virNetDevVlan vlan; + virNetworkIpDefPtr ipv4_dhcp; + virNetworkIpDefPtr ipv6_dhcp; };
typedef struct _virNetworkObj virNetworkObj;
From: TJ <linux@iam.tj> To avoid iterating all virNetworkIpDef entries when determining DHCP state keep track of the first enabled DHCP stanza in the network definition itself, for both IPv4 and IPv6. A by-product of this change is it allows the XML to contain more than one IP->DHCP stanza. The active DHCP stanza is the first enabled DHCP stanza. All other stanzas are retained which adds flexibility when multiple interfaces and routes might come and go since an alternative DHCP stanza can be selected and a refresh operation performed without needing to destroy/edit/start the network. Signed-off-by: TJ <linux@iam.tj> --- src/conf/network_conf.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 259de0a..c5eab01 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1882,6 +1882,13 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) if (ret < 0) goto error; def->nips++; + /* use only the first enabled DHCP definition */ + if (!def->ipv4_dhcp && def->ips[ii].dhcp_enabled && + VIR_SOCKET_ADDR_IS_FAMILY(&def->ips[ii].address, AF_INET)) + def->ipv4_dhcp = &def->ips[ii]; + if (!def->ipv6_dhcp && def->ips[ii].dhcp_enabled && + VIR_SOCKET_ADDR_IS_FAMILY(&def->ips[ii].address, AF_INET6)) + def->ipv6_dhcp = &def->ips[ii]; } } VIR_FREE(ipNodes); -- 1.8.1.2.433.g9808ce0.dirty
On 02/27/2013 09:57 PM, TJ wrote:
From: TJ <linux@iam.tj>
To avoid iterating all virNetworkIpDef entries when determining DHCP state keep track of the first enabled DHCP stanza in the network definition itself, for both IPv4 and IPv6.
A by-product of this change is it allows the XML to contain more than one IP->DHCP stanza. The active DHCP stanza is the first enabled DHCP stanza.
Hmm. This is an interesting idea, but I can't think of any other place in libvirt config where we allow putting the config in place and setting it disabled, and wouldn't want to set that precedent if it didn't already exist. Any other opinions about that?
All other stanzas are retained which adds flexibility when multiple interfaces and routes might come and go since an alternative DHCP stanza can be selected and a refresh operation performed without needing to destroy/edit/start the network.
Well, the network would still need to be updated (virNetworkUpdateFlags), and that operation.
Signed-off-by: TJ <linux@iam.tj> --- src/conf/network_conf.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 259de0a..c5eab01 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -1882,6 +1882,13 @@ virNetworkDefParseXML(xmlXPathContextPtr ctxt) if (ret < 0) goto error; def->nips++; + /* use only the first enabled DHCP definition */ + if (!def->ipv4_dhcp && def->ips[ii].dhcp_enabled && + VIR_SOCKET_ADDR_IS_FAMILY(&def->ips[ii].address, AF_INET)) + def->ipv4_dhcp = &def->ips[ii]; + if (!def->ipv6_dhcp && def->ips[ii].dhcp_enabled && + VIR_SOCKET_ADDR_IS_FAMILY(&def->ips[ii].address, AF_INET6)) + def->ipv6_dhcp = &def->ips[ii]; } } VIR_FREE(ipNodes);
From: TJ <linux@iam.tj> Rather than iterate through virNetworkIPDef arrays multiple times use the new virNetworkDef ipv4_dhcp and ipv6_dhcp active stanza pointers. Signed-off-by: TJ <linux@iam.tj> --- src/network/bridge_driver.c | 63 +++++++++++---------------------------------- 1 file changed, 15 insertions(+), 48 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0932cf8..8410c93 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -810,24 +810,16 @@ networkDnsmasqConfContents(virNetworkObjPtr network, } } - /* Find the first dhcp for both IPv4 and IPv6 */ - for (ii = 0, ipv4def = NULL, ipv6def = NULL, 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; - } - } - } - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) { - if (ipdef->nranges || ipdef->nhosts) { + ipv4def = ipv6def = NULL; + ipdef = network->def->ipv4_dhcp; + if (ipdef && (ipdef->nranges || ipdef->nhosts)) + ipv4def = ipdef; + + ipdef = network->def->ipv6_dhcp; + if (ipdef) { + if (ipdef->nranges || ipdef->nhosts) { + ipv6def = ipdef; + if (!DNSMASQ_DHCPv6_SUPPORT(caps)) { unsigned long version = dnsmasqCapsGetVersion(caps); virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -842,18 +834,9 @@ networkDnsmasqConfContents(virNetworkObjPtr network, DNSMASQ_DHCPv6_MINOR_REQD); goto cleanup; } - if (ipv6def) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("For IPv6, multiple DHCP definitions " - "cannot be specified.")); - goto cleanup; - } else { - ipv6def = ipdef; - } - } else { - ipv6SLAAC = true; - } - } + } + } else { + ipv6SLAAC = true; } if (ipv6def && ipv6SLAAC) { @@ -1160,25 +1143,9 @@ networkRefreshDhcpDaemon(struct network_driver *driver, 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 (!ipv4def && (ipdef->nranges || ipdef->nhosts)) - ipv4def = ipdef; - } + ipv4def = network->def->ipv4_dhcp; - ipv6def = NULL; - for (ii = 0; - (ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET6, ii)); - ii++) { - if (!ipv6def && (ipdef->nranges || ipdef->nhosts)) - ipv6def = ipdef; - } + ipv6def = network->def->ipv6_dhcp; if (ipv4def && (networkBuildDnsmasqDhcpHostsList(dctx, ipv4def) < 0)) goto cleanup; -- 1.8.1.2.433.g9808ce0.dirty
On 02/27/2013 09:57 PM, TJ wrote:
From: TJ <linux@iam.tj>
Rather than iterate through virNetworkIPDef arrays multiple times use the new virNetworkDef ipv4_dhcp and ipv6_dhcp active stanza pointers.
Signed-off-by: TJ <linux@iam.tj> --- src/network/bridge_driver.c | 63 +++++++++++---------------------------------- 1 file changed, 15 insertions(+), 48 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0932cf8..8410c93 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -810,24 +810,16 @@ networkDnsmasqConfContents(virNetworkObjPtr network, } }
- /* Find the first dhcp for both IPv4 and IPv6 */ - for (ii = 0, ipv4def = NULL, ipv6def = NULL, ipv6SLAAC = false;
Your refactoring has eliminated the initialization of ipv6SLAAC to 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; - } - } - } - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) { - if (ipdef->nranges || ipdef->nhosts) {
I'm starting to warm to the idea of a convenience pointer, but still dislike the fact that it could get out of sync due to careless programming. How about a convenience *function* instead. It would be just as inefficient, but the code would look cleaner (and the inefficiency isn't a big problem anyway, since it's very rarely done, and takes a miniscule amount of time anyway).
+ ipv4def = ipv6def = NULL; + ipdef = network->def->ipv4_dhcp; + if (ipdef && (ipdef->nranges || ipdef->nhosts)) + ipv4def = ipdef; + + ipdef = network->def->ipv6_dhcp; + if (ipdef) { + if (ipdef->nranges || ipdef->nhosts) { + ipv6def = ipdef; + if (!DNSMASQ_DHCPv6_SUPPORT(caps)) { unsigned long version = dnsmasqCapsGetVersion(caps); virReportError(VIR_ERR_CONFIG_UNSUPPORTED, @@ -842,18 +834,9 @@ networkDnsmasqConfContents(virNetworkObjPtr network, DNSMASQ_DHCPv6_MINOR_REQD); goto cleanup; } - if (ipv6def) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("For IPv6, multiple DHCP definitions " - "cannot be specified.")); - goto cleanup; - } else { - ipv6def = ipdef; - } - } else { - ipv6SLAAC = true; - } - } + } + } else { + ipv6SLAAC = true; }
if (ipv6def && ipv6SLAAC) { @@ -1160,25 +1143,9 @@ networkRefreshDhcpDaemon(struct network_driver *driver, 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 (!ipv4def && (ipdef->nranges || ipdef->nhosts)) - ipv4def = ipdef; - } + ipv4def = network->def->ipv4_dhcp;
- ipv6def = NULL; - for (ii = 0; - (ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET6, ii)); - ii++) { - if (!ipv6def && (ipdef->nranges || ipdef->nhosts)) - ipv6def = ipdef; - } + ipv6def = network->def->ipv6_dhcp;
if (ipv4def && (networkBuildDnsmasqDhcpHostsList(dctx, ipv4def) < 0)) goto cleanup;
On 28/02/13 19:51, Laine Stump wrote:
On 02/27/2013 09:57 PM, TJ wrote:
From: TJ <linux@iam.tj>
Rather than iterate through virNetworkIPDef arrays multiple times use the new virNetworkDef ipv4_dhcp and ipv6_dhcp active stanza pointers.
Signed-off-by: TJ <linux@iam.tj> --- src/network/bridge_driver.c | 63 +++++++++++---------------------------------- 1 file changed, 15 insertions(+), 48 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0932cf8..8410c93 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -810,24 +810,16 @@ networkDnsmasqConfContents(virNetworkObjPtr network, } }
- /* Find the first dhcp for both IPv4 and IPv6 */ - for (ii = 0, ipv4def = NULL, ipv6def = NULL, ipv6SLAAC = false;
Your refactoring has eliminated the initialization of ipv6SLAAC to 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; - } - } - } - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) { - if (ipdef->nranges || ipdef->nhosts) {
I'm starting to warm to the idea of a convenience pointer, but still dislike the fact that it could get out of sync due to careless programming. How about a convenience *function* instead. It would be just as inefficient, but the code would look cleaner (and the inefficiency isn't a big problem anyway, since it's very rarely done, and takes a miniscule amount of time anyway).
Actually that is where I started, see patch 06 "network: Bridge - Add support for DHCP relay agent" and static virNetworkGetActiveDhcp(). Later I wanted to eliminate the iterations over the <ip> structures that I had moved into this function since it was called from several places, and ended up replacing it with code in the XML parser that sets the ->ipv{4,6}_dhcp pointers once.
On 28/02/13 19:51, Laine Stump wrote:
On 02/27/2013 09:57 PM, TJ wrote:
From: TJ <linux@iam.tj>
Rather than iterate through virNetworkIPDef arrays multiple times use the new virNetworkDef ipv4_dhcp and ipv6_dhcp active stanza pointers.
Signed-off-by: TJ <linux@iam.tj> --- src/network/bridge_driver.c | 63 +++++++++++---------------------------------- 1 file changed, 15 insertions(+), 48 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 0932cf8..8410c93 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -810,24 +810,16 @@ networkDnsmasqConfContents(virNetworkObjPtr network, } }
- /* Find the first dhcp for both IPv4 and IPv6 */ - for (ii = 0, ipv4def = NULL, ipv6def = NULL, ipv6SLAAC = false;
Your refactoring has eliminated the initialization of ipv6SLAAC to 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; - } - } - } - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) { - if (ipdef->nranges || ipdef->nhosts) {
I'm starting to warm to the idea of a convenience pointer, but still dislike the fact that it could get out of sync due to careless programming. How about a convenience *function* instead. It would be just as inefficient, but the code would look cleaner (and the inefficiency isn't a big problem anyway, since it's very rarely done, and takes a miniscule amount of time anyway).
Actually that is where I started, see patch 06 "network: Bridge - Add support for DHCP relay agent" and static virNetworkGetActiveDhcp(). Later I wanted to eliminate the iterations over the <ip> structures that I had moved into this function since it was called from several places, and ended up replacing it with code in the XML parser that sets the ->ipv{4,6}_dhcp pointers once.
From: TJ <linux@iam.tj> A DHCP relay daemon will be started that will forward all DHCP/BOOTP requests on the bridge network via the first declared forward interface. The relay is broadcast rather than routed so no IP address is needed on the bridge. The XML <relay> element's "relay" property of the active DHCP stanza defaults to 'no'. Set it to 'yes' to enable the relay: <ip ...> <dhcp relay='yes'/> </ip> The relay will not be started if the "enable" property is 'no': <ip ...> <dhcp enable='no' relay='yes'/> </ip> Signed-off-by: TJ <linux@iam.tj> --- src/network/bridge_driver.c | 146 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 146 insertions(+) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8410c93..c02d3de 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -587,6 +587,145 @@ cleanup: * which is later saved into a file */ +static virNetworkIpDefPtr +networkGetActiveDhcp(virNetworkObjPtr network) +{ + virNetworkIpDefPtr dhcp = NULL; + + if (network->def && network->def->ipv4_dhcp) + dhcp = network->def->ipv4_dhcp; + + if (!dhcp && + network->def && network->def->ipv6_dhcp) + dhcp = network->def->ipv6_dhcp; + + return dhcp; +} + +static int +networkBuildDhcpRelayArgv(virNetworkObjPtr network, + const char *pidfile, + virCommandPtr cmd) +{ + int ret = -1; + + /* PID file */ + virCommandAddArgList(cmd, "-r", pidfile, NULL); + + /* Listen for DHCP requests on the bridge interface */ + virCommandAddArgList(cmd, "-i", network->def->bridge, NULL); + + /* Use the first forwarding device to broadcast to the upstream DHCP server */ + if (network->def->forward.nifs > 0) { + virCommandAddArgList(cmd, "-b", network->def->forward.ifs[0].device.dev, NULL); + ret = 0; + } else + virReportSystemError(VIR_ERR_INVALID_INTERFACE, + _("DHCP relay requires at least one network %s\n"), + "<forward ... dev='eth?'/> or <interface dev='eth?'/>"); + + return ret; +} + +static int +networkBuildDhcpRelayCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, + char *pidfile) +{ + virCommandPtr cmd = NULL; + int ret = -1; + + cmd = virCommandNew(DHCPRELAY); + if (networkBuildDhcpRelayArgv(network, pidfile, cmd) < 0) { + goto cleanup; + } + + if (cmdout) + *cmdout = cmd; + ret = 0; +cleanup: + if (ret < 0) + virCommandFree(cmd); + return ret; +} + +static int +networkStartDhcpRelayDaemon(struct network_driver *driver ATTRIBUTE_UNUSED, + virNetworkObjPtr network) +{ + virCommandPtr cmd = NULL; + virNetworkIpDefPtr ipdef = NULL; + char *pidfile = NULL; + char *tmp = NULL; + int pid_len; + int ret = 0; + const char *dhcprelay = "dhcprelay_"; + + ipdef = networkGetActiveDhcp(network); + /* Prepare for DHCP relay agent */ + if (ipdef && ipdef->dhcp_enabled && ipdef->dhcp_relay) { + ret = -1; + + if (virFileMakePath(NETWORK_PID_DIR) < 0) { + virReportSystemError(errno, + _("cannot create directory %s"), + NETWORK_PID_DIR); + goto cleanup; + } + + pid_len = strlen(dhcprelay) + strlen(network->def->name); + if ( VIR_ALLOC_N(tmp, pid_len+1) >= 0) { + tmp = strcpy(tmp, dhcprelay); + tmp = strncat(tmp, network->def->name, pid_len); + if (!(pidfile = virPidFileBuildPath(NETWORK_PID_DIR, tmp))) { + virReportOOMError(); + goto cleanup; + } + } else { + virReportOOMError(); + goto cleanup; + } + + ret = networkBuildDhcpRelayCommandLine(network, &cmd, pidfile); + if (ret < 0) + goto cleanup; + + ret = virCommandRun(cmd, NULL); + if (ret < 0) + goto cleanup; + + ret = virPidFileRead(NETWORK_PID_DIR, pidfile, &network->dhcprelayPid); + if (ret < 0) + virReportSystemError(errno, _("%s is not running"), DHCPRELAY); + +cleanup: + VIR_FREE(tmp); + VIR_FREE(pidfile); + virCommandFree(cmd); + } + return ret; +} + +static int +networkRestartDhcpRelayDaemon(struct network_driver *driver, + virNetworkObjPtr network) +{ + /* if there is a running DHCP relay agent, kill it */ + if (network->dhcprelayPid > 0) { + networkKillDaemon(network->dhcprelayPid, DHCPRELAY, + network->def->name); + network->dhcprelayPid = -1; + } + /* now start the daemon if it should be started */ + return networkStartDhcpRelayDaemon(driver, network); +} + +static int +networkRefreshDhcpRelayDaemon(struct network_driver *driver, + virNetworkObjPtr network) +{ + return networkRestartDhcpRelayDaemon(driver, network); +} + static int networkBuildDnsmasqDhcpHostsList(dnsmasqContext *dctx, virNetworkIpDefPtr ipdef) @@ -1496,6 +1635,7 @@ networkRefreshDaemons(struct network_driver *driver) * dnsmasq and/or radvd, or restart them if they've * disappeared. */ + networkRefreshDhcpRelayDaemon(driver, network); networkRefreshDhcpDaemon(driver, network); networkRefreshRadvd(driver, network); } @@ -2462,6 +2602,10 @@ networkStartNetworkVirtual(struct network_driver *driver, networkStartDhcpDaemon(driver, network) < 0) goto err3; + /* start DHCP relay-agent (doesn't need IP address(es) to function) */ + if (networkStartDhcpRelayDaemon(driver, network) < 0) + goto err3; + /* start radvd if there are any ipv6 addresses */ if (v6present && networkStartRadvd(driver, network) < 0) goto err4; @@ -3276,6 +3420,8 @@ networkUpdate(virNetworkPtr net, */ if (networkRestartDhcpDaemon(driver, network) < 0) goto cleanup; + if (networkRestartDhcpRelayDaemon(driver, network) < 0) + goto cleanup; } else if (section == VIR_NETWORK_SECTION_IP_DHCP_HOST) { /* if we previously weren't listening for dhcp and now we -- 1.8.1.2.433.g9808ce0.dirty
On 02/27/2013 07:57 PM, TJ wrote:
From: TJ <linux@iam.tj>
A DHCP relay daemon will be started that will forward all DHCP/BOOTP requests on the bridge network via the first declared forward interface. The relay is broadcast rather than routed so no IP address is needed on the bridge.
The XML <relay> element's "relay" property of the active DHCP stanza defaults to 'no'. Set it to 'yes' to enable the relay:
<ip ...> <dhcp relay='yes'/> </ip>
The relay will not be started if the "enable" property is 'no':
<ip ...> <dhcp enable='no' relay='yes'/> </ip>
At this point in the series, I'll defer to Laine for technical review. But I still have some style review:
+ + /* Use the first forwarding device to broadcast to the upstream DHCP server */ + if (network->def->forward.nifs > 0) { + virCommandAddArgList(cmd, "-b", network->def->forward.ifs[0].device.dev, NULL); + ret = 0;
No TABs in .c, ever. Run 'make syntax-check'.
+ } else
HACKING says that if you use {} on one side of 'else', you must use it on both sides.
+ virReportSystemError(VIR_ERR_INVALID_INTERFACE, + _("DHCP relay requires at least one network %s\n"), + "<forward ... dev='eth?'/> or <interface dev='eth?'/>");
Indentation is off, even when you get rid of that TAB. You have a mixed-language sentence - translators will get the first half, but can't translate the 'or' in the second string, and that looks gross. I would have written: virReportSystemError(VIR_ERR_INVALID_INTERFACE, "%s", _("DHCP relay requires at least one network " "<forward dev='eth?'/> or " "<interface dev='eth?'/>");
+static int +networkBuildDhcpRelayCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, + char *pidfile) +{ + virCommandPtr cmd = NULL; + int ret = -1; + + cmd = virCommandNew(DHCPRELAY); + if (networkBuildDhcpRelayArgv(network, pidfile, cmd) < 0) { + goto cleanup; + } + + if (cmdout) + *cmdout = cmd; + ret = 0; +cleanup: + if (ret < 0) + virCommandFree(cmd); + return ret;
Memory leak if ret == 0 but !*cmdout.
+} + +static int +networkStartDhcpRelayDaemon(struct network_driver *driver ATTRIBUTE_UNUSED, + virNetworkObjPtr network)
Indentation is off.
+{ + virCommandPtr cmd = NULL; + virNetworkIpDefPtr ipdef = NULL; + char *pidfile = NULL; + char *tmp = NULL; + int pid_len; + int ret = 0; + const char *dhcprelay = "dhcprelay_"; + + ipdef = networkGetActiveDhcp(network); + /* Prepare for DHCP relay agent */ + if (ipdef && ipdef->dhcp_enabled && ipdef->dhcp_relay) { + ret = -1;
Indentation is off.
+ + if (virFileMakePath(NETWORK_PID_DIR) < 0) { + virReportSystemError(errno, + _("cannot create directory %s"), + NETWORK_PID_DIR); + goto cleanup; + } + + pid_len = strlen(dhcprelay) + strlen(network->def->name); + if ( VIR_ALLOC_N(tmp, pid_len+1) >= 0) {
Spacing is off. Correct would be: if (VIR_ALLOC_N(tmp, pid_len + 1) >= 0) { But you shouldn't need to do VIR_ALLOC_N in the first place, since...
+ tmp = strcpy(tmp, dhcprelay); + tmp = strncat(tmp, network->def->name, pid_len);
...strncat() is generally the wrong interface. Use virAsprintf or virBuffer* instead.
+ if (!(pidfile = virPidFileBuildPath(NETWORK_PID_DIR, tmp))) { + virReportOOMError(); + goto cleanup; + } + } else { + virReportOOMError(); + goto cleanup; + }
This indentation mess with TAB is making this hard to review. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 28/02/13 03:38, Eric Blake wrote:
This indentation mess with TAB is making this hard to review.
It seems vim is the culprit. Default setting seems to be that it's inserting a tab when I hit Enter on a previous line when there's a multiple of 8 spaces of indent on the new line. I had a few arguments with it and thought it was behaving. Ignore the formatting issues for now, deal with the substance. If the substance is acceptable I'll ensure a revised series doesn't choke on hidden formatting.
On 02/27/2013 07:57 PM, TJ wrote:
From: TJ <linux@iam.tj>
A DHCP relay daemon will be started that will forward all DHCP/BOOTP requests on the bridge network via the first declared forward interface. The relay is broadcast rather than routed so no IP address is needed on the bridge.
+static int +networkBuildDhcpRelayCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, + char *pidfile) +{ + virCommandPtr cmd = NULL; + int ret = -1; + + cmd = virCommandNew(DHCPRELAY);
Your patches are out of order. This patch tries to use DHCPRELAY, but that isn't defined until patch 8/10 modifies configure.ac. Squash those two patches together. Each patch needs to be buildable in isolation. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 28/02/13 04:09, Eric Blake wrote:
On 02/27/2013 07:57 PM, TJ wrote:
From: TJ <linux@iam.tj>
A DHCP relay daemon will be started that will forward all DHCP/BOOTP requests on the bridge network via the first declared forward interface. The relay is broadcast rather than routed so no IP address is needed on the bridge.
+static int +networkBuildDhcpRelayCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, + char *pidfile) +{ + virCommandPtr cmd = NULL; + int ret = -1; + + cmd = virCommandNew(DHCPRELAY);
Your patches are out of order. This patch tries to use DHCPRELAY, but that isn't defined until patch 8/10 modifies configure.ac. Squash those two patches together. Each patch needs to be buildable in isolation.
One thing I'm not yet sure how to deal with is the difference between build and deployment systems when one or the other doesn't have access to 'dhcp-helper', since as I understand it, some distributions do not include it in their package archives (it is available in Debian/Ubuntu). With the current implementation it must be available on the build system since otherwise the configure.ac definition of DHCPRELAY won't happen and it won't be declared in config.h.in, which means that compilation of the source-code relying on it will fail. It seems messy but the only realistic way to deal with that is to surround the relevant code block(s) with #ifdef DHCPRELAY ... #endif My preferred option is to change the way DHCPRELAY is handled entirely. I copied its definition style from DNSMASQ which, as far as I can see, *must* exist on the build system and in the same absolute path as it will be found on the deployment system. It might be better to simply declare the expected basename of the executable so instead of: DHCPRELAY=/usr/sbin/dhcp-helper we have DHCPRELAY=dhcp-helper then allow libvirt to find that executable using the PATH at run-time (which also accommodates user builds installed to /usr/local/sbin/). Taking that further to abstract the DHCPRELAY code entirely from dhcp-helper (since unlike dnsmasq it isn't required to be available), it might make sense to add an optional 'agent' attribute to the <dhcp> element; something like <dhcp relay='yes' agent='isc-relay'/> (which would find 'agent' on the PATH). This would allow the default to be taken from DHCPRELAY but over-ridden by the optional 'agent' attribute. That then leaves the issue of different DHCP relay agents requiring different command-line parameters. I was looking at the discussion about the <option> element and wondering if that could have a dual use here, especially in the name= variety, e.g: <ip ...> <dhcp relay='yes' agent='some-agent-we-dont-know'> <option name="bridge"> <value data="-b"/> </option> <option name="interface"> <value data="-i"/> </option> <option name="pidfile"> <value data="-r"/> </option> </dhcp> </ip> For the DHCP relay the <option> names ("bridge", "interface", "pdifile") are constants that libvirt looks for in the XML. It replaces its hard-coded command-line switch defaults with the respective <value data="..."> if found. That approach would give the DHCP relay code the flexibility to evolve based on sysadmin needs without requiring any code modification.
On 02/27/2013 11:32 PM, TJ wrote:
One thing I'm not yet sure how to deal with is the difference between build and deployment systems when one or the other doesn't have access to 'dhcp-helper', since as I understand it, some distributions do not include it in their package archives (it is available in Debian/Ubuntu).
Just copy what we do for other helper executables, such as dnsmasq. If the user pre-defines $DNSMASQ as part of calling configure (that is, ./configure DNSMASQ=/alternate/dnsmasq), that pre-configuration is used regardless of whether it exists at the time. Otherwise, if the executable is present at configure time, then that path is hardcoded into that build. If neither pre-configured nor path search finds anything, then the basename is used, for a runtime PATH lookup. Distro packagers are smart enough to do build requirements and either prepulate the variable or pre-install the packages in their build environment, so that the distro build of libvirt will always have the desired absolute path to the distro executable. Someone building libvirt from a tarball has total control over what to use, and the default of a PATH lookup during configure with a fallback to a PATH lookup at runtime is generally the right thing (except for distros, most people don't build on one machine and install on another). So your patch already did the right thing, by copying and pasting what existing examples we already had for DNSMASQ.
That then leaves the issue of different DHCP relay agents requiring different command-line parameters. I was looking at the discussion about the <option> element and wondering if that could have a dual use here, especially in the name= variety, e.g:
We have to deal with different command line syntax in the case of nc (netcat); but that's normally a problem we don't have to worry about. If someone actually hits the problem, they can provide patches at that time. For now, assuming that you are targetting the one and only dhcp-relay program with known syntax is sufficient. In other words, you're worrying too much. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/28/2013 01:32 AM, TJ wrote:
That then leaves the issue of different DHCP relay agents requiring different command-line parameters. I was looking at the discussion about the <option> element and wondering if that could have a dual use here, especially in the name= variety, e.g:
<ip ...> <dhcp relay='yes' agent='some-agent-we-dont-know'> <option name="bridge"> <value data="-b"/> </option> <option name="interface"> <value data="-i"/> </option> <option name="pidfile"> <value data="-r"/> </option> </dhcp> </ip>
No, definitely not. the <option> element is for options in the dhcp request/response packets, as defined in RFC2132 (and others), other than coincidentally (because the dhcp options can be specified on the dnsmasq commandline) it has nothing to do with commandline parameters of any particular program.
On 02/27/2013 09:57 PM, TJ wrote:
From: TJ <linux@iam.tj>
A DHCP relay daemon will be started that will forward all DHCP/BOOTP requests on the bridge network via the first declared forward interface.
Okay, I think we've got our first candidate for something we might want to configure. Picking the forward interface would probably be correct 95% of the time, but it seems reasonable someone might want to forward it somewhere else. Also there's the fact that most people don't define any forward interface for their networks, just leaving it up to the host's IP routing to determine the appropriate egress for every packet. (I don't really like what's done with the forward interface anyway - it doesn't do anything about routing to force traffic egress via that particular interface, just rejects any traffic that would have gone out a different interface).
The relay is broadcast rather than routed so no IP address is needed on the bridge.
The XML <relay> element's "relay" property of the active DHCP stanza defaults to 'no'. Set it to 'yes' to enable the relay:
<ip ...> <dhcp relay='yes'/> </ip>
The relay will not be started if the "enable" property is 'no':
<ip ...> <dhcp enable='no' relay='yes'/>
I don't see the utility of that - it's the same as simply omitting the <dhcp> element altogether.
</ip>
Signed-off-by: TJ <linux@iam.tj> --- src/network/bridge_driver.c | 146 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 146 insertions(+)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 8410c93..c02d3de 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -587,6 +587,145 @@ cleanup: * which is later saved into a file */
+static virNetworkIpDefPtr +networkGetActiveDhcp(virNetworkObjPtr network) +{ + virNetworkIpDefPtr dhcp = NULL; + + if (network->def && network->def->ipv4_dhcp) + dhcp = network->def->ipv4_dhcp; + + if (!dhcp && + network->def && network->def->ipv6_dhcp) + dhcp = network->def->ipv6_dhcp; + + return dhcp; +} + +static int +networkBuildDhcpRelayArgv(virNetworkObjPtr network, + const char *pidfile, + virCommandPtr cmd) +{ + int ret = -1; + + /* PID file */ + virCommandAddArgList(cmd, "-r", pidfile, NULL); + + /* Listen for DHCP requests on the bridge interface */ + virCommandAddArgList(cmd, "-i", network->def->bridge, NULL); + + /* Use the first forwarding device to broadcast to the upstream DHCP server */ + if (network->def->forward.nifs > 0) { + virCommandAddArgList(cmd, "-b", network->def->forward.ifs[0].device.dev, NULL); + ret = 0; + } else + virReportSystemError(VIR_ERR_INVALID_INTERFACE, + _("DHCP relay requires at least one network %s\n"), + "<forward ... dev='eth?'/> or <interface dev='eth?'/>"); + + return ret; +} + +static int +networkBuildDhcpRelayCommandLine(virNetworkObjPtr network, virCommandPtr *cmdout, + char *pidfile) +{ + virCommandPtr cmd = NULL; + int ret = -1; + + cmd = virCommandNew(DHCPRELAY); + if (networkBuildDhcpRelayArgv(network, pidfile, cmd) < 0) { + goto cleanup; + } + + if (cmdout) + *cmdout = cmd; + ret = 0; +cleanup: + if (ret < 0) + virCommandFree(cmd); + return ret; +} + +static int +networkStartDhcpRelayDaemon(struct network_driver *driver ATTRIBUTE_UNUSED, + virNetworkObjPtr network) +{ + virCommandPtr cmd = NULL; + virNetworkIpDefPtr ipdef = NULL; + char *pidfile = NULL; + char *tmp = NULL; + int pid_len; + int ret = 0; + const char *dhcprelay = "dhcprelay_"; + + ipdef = networkGetActiveDhcp(network); + /* Prepare for DHCP relay agent */ + if (ipdef && ipdef->dhcp_enabled && ipdef->dhcp_relay) { + ret = -1; + + if (virFileMakePath(NETWORK_PID_DIR) < 0) { + virReportSystemError(errno, + _("cannot create directory %s"), + NETWORK_PID_DIR); + goto cleanup; + } + + pid_len = strlen(dhcprelay) + strlen(network->def->name); + if ( VIR_ALLOC_N(tmp, pid_len+1) >= 0) { + tmp = strcpy(tmp, dhcprelay); + tmp = strncat(tmp, network->def->name, pid_len); + if (!(pidfile = virPidFileBuildPath(NETWORK_PID_DIR, tmp))) { + virReportOOMError(); + goto cleanup; + } + } else { + virReportOOMError(); + goto cleanup; + } + + ret = networkBuildDhcpRelayCommandLine(network, &cmd, pidfile); + if (ret < 0) + goto cleanup; + + ret = virCommandRun(cmd, NULL); + if (ret < 0) + goto cleanup; + + ret = virPidFileRead(NETWORK_PID_DIR, pidfile, &network->dhcprelayPid); + if (ret < 0) + virReportSystemError(errno, _("%s is not running"), DHCPRELAY); + +cleanup: + VIR_FREE(tmp); + VIR_FREE(pidfile); + virCommandFree(cmd); + } + return ret; +} + +static int +networkRestartDhcpRelayDaemon(struct network_driver *driver, + virNetworkObjPtr network) +{ + /* if there is a running DHCP relay agent, kill it */ + if (network->dhcprelayPid > 0) { + networkKillDaemon(network->dhcprelayPid, DHCPRELAY, + network->def->name); + network->dhcprelayPid = -1; + } + /* now start the daemon if it should be started */ + return networkStartDhcpRelayDaemon(driver, network); +} + +static int +networkRefreshDhcpRelayDaemon(struct network_driver *driver, + virNetworkObjPtr network) +{ + return networkRestartDhcpRelayDaemon(driver, network); +} + static int networkBuildDnsmasqDhcpHostsList(dnsmasqContext *dctx, virNetworkIpDefPtr ipdef) @@ -1496,6 +1635,7 @@ networkRefreshDaemons(struct network_driver *driver) * dnsmasq and/or radvd, or restart them if they've * disappeared. */ + networkRefreshDhcpRelayDaemon(driver, network); networkRefreshDhcpDaemon(driver, network); networkRefreshRadvd(driver, network); } @@ -2462,6 +2602,10 @@ networkStartNetworkVirtual(struct network_driver *driver, networkStartDhcpDaemon(driver, network) < 0) goto err3;
+ /* start DHCP relay-agent (doesn't need IP address(es) to function) */ + if (networkStartDhcpRelayDaemon(driver, network) < 0) + goto err3; + /* start radvd if there are any ipv6 addresses */ if (v6present && networkStartRadvd(driver, network) < 0) goto err4; @@ -3276,6 +3420,8 @@ networkUpdate(virNetworkPtr net, */ if (networkRestartDhcpDaemon(driver, network) < 0) goto cleanup; + if (networkRestartDhcpRelayDaemon(driver, network) < 0) + goto cleanup;
} else if (section == VIR_NETWORK_SECTION_IP_DHCP_HOST) { /* if we previously weren't listening for dhcp and now we
On 28/02/13 19:56, Laine Stump wrote:
On 02/27/2013 09:57 PM, TJ wrote:
From: TJ <linux@iam.tj>
A DHCP relay daemon will be started that will forward all DHCP/BOOTP requests on the bridge network via the first declared forward interface.
Okay, I think we've got our first candidate for something we might want to configure. Picking the forward interface would probably be correct 95% of the time, but it seems reasonable someone might want to forward it somewhere else. Also there's the fact that most people don't define any forward interface for their networks, just leaving it up to the host's IP routing to determine the appropriate egress for every packet. (I don't really like what's done with the forward interface anyway - it doesn't do anything about routing to force traffic egress via that particular interface, just rejects any traffic that would have gone out a different interface).
Are you thinking in terms of <dhcp relay='yes' forward='ethX'/> ? Should it be always explicit or use the current 'pick a sensible default' but allow this property to over-ride it?
The relay is broadcast rather than routed so no IP address is needed on the bridge.
The XML <relay> element's "relay" property of the active DHCP stanza defaults to 'no'. Set it to 'yes' to enable the relay:
<ip ...> <dhcp relay='yes'/> </ip>
The relay will not be started if the "enable" property is 'no':
<ip ...> <dhcp enable='no' relay='yes'/>
I don't see the utility of that - it's the same as simply omitting the <dhcp> element altogether.
The utility of the "enabled='(yes|no)'" properties is one of convenience for the sysadmin, and was something I felt in need of whilst testing many scenarios of dnsmaq *before* I embarked on this DHCP relay functionality, and again when I was testing dhcp-helper. Without needing to delete important sub-elements of <dhcp> (and lose through forgetting them) from an active network XML definition I could selectively switch a <dhcp> block on/off simply by adding the "enabled" property to the element. The alternative was continually having to switch amongst several network XML files all of which were permutations of the same settings in order that disabling a stanza in the primary XML didn't require needing to delete, for example, all the <host> and <range> sub-elements of <dhcp>.
From: TJ <linux@iam.tj> When dnsmasq's DNS services are required but the network is configured to use a DHCP relay agent (other than dnsmasq's proxy services) the configuration generated for dnsmasq should not enable DHCP services. Signed-off-by: TJ <linux@iam.tj> --- src/network/bridge_driver.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index c02d3de..a4cd727 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -951,11 +951,12 @@ networkDnsmasqConfContents(virNetworkObjPtr network, ipv4def = ipv6def = NULL; ipdef = network->def->ipv4_dhcp; - if (ipdef && (ipdef->nranges || ipdef->nhosts)) + if (ipdef && ipdef->dhcp_enabled && !ipdef->dhcp_relay && + (ipdef->nranges || ipdef->nhosts)) ipv4def = ipdef; ipdef = network->def->ipv6_dhcp; - if (ipdef) { + if (ipdef && ipdef->dhcp_enabled && !ipdef->dhcp_relay) { if (ipdef->nranges || ipdef->nhosts) { ipv6def = ipdef; @@ -1266,8 +1267,8 @@ static int networkRefreshDhcpDaemon(struct network_driver *driver, virNetworkObjPtr network) { - int ret = -1, ii; - virNetworkIpDefPtr ipdef, ipv4def, ipv6def; + int ret = -1; + virNetworkIpDefPtr ipv4def, ipv6def; dnsmasqContext *dctx = NULL; /* if no IP addresses specified, nothing to do */ -- 1.8.1.2.433.g9808ce0.dirty
On 02/27/2013 09:57 PM, TJ wrote:
From: TJ <linux@iam.tj>
When dnsmasq's DNS services are required but the network is configured to use a DHCP relay agent (other than dnsmasq's proxy services) the configuration generated for dnsmasq should not enable DHCP services.
Signed-off-by: TJ <linux@iam.tj> --- src/network/bridge_driver.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index c02d3de..a4cd727 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -951,11 +951,12 @@ networkDnsmasqConfContents(virNetworkObjPtr network,
ipv4def = ipv6def = NULL; ipdef = network->def->ipv4_dhcp; - if (ipdef && (ipdef->nranges || ipdef->nhosts)) + if (ipdef && ipdef->dhcp_enabled && !ipdef->dhcp_relay && + (ipdef->nranges || ipdef->nhosts)) ipv4def = ipdef;
ipdef = network->def->ipv6_dhcp; - if (ipdef) { + if (ipdef && ipdef->dhcp_enabled && !ipdef->dhcp_relay) { if (ipdef->nranges || ipdef->nhosts) { ipv6def = ipdef;
@@ -1266,8 +1267,8 @@ static int networkRefreshDhcpDaemon(struct network_driver *driver, virNetworkObjPtr network) { - int ret = -1, ii;
You apparently removed the usage of ii in an earlier patch, but are removing the definition of ii here. That would have cause a build failure after applying the earlier patch. AS Eric mentioned, you must be able to successfully complete an autogen.sh && make check && make syntax-check after applying each patch in the series in sequence. Otherwise, git bisect becomes unusable. (This is another argument for integrating more into each patch).
- virNetworkIpDefPtr ipdef, ipv4def, ipv6def; + int ret = -1; + virNetworkIpDefPtr ipv4def, ipv6def; dnsmasqContext *dctx = NULL;
/* if no IP addresses specified, nothing to do */
From: TJ <linux@iam.tj> This variable should name the path to the system's DHCP relay daemon. At this time the expected daemon is "dhcp-helper", a DHCP relay agent from Simon Kelly, author of dnsmasq. The supporting code, however, has been designed to work with any suitable DHCP relay agent. Later patches could allow configuration of the agent's command-line arguments. Currently the chosen agent must support: -b <bridge_interface> (the virtual network to listen on) -i <physical_interface> (the interface to broadcast on) -r </path/to/pid.file> (the PID file of the daemon) Signed-off-by: TJ <linux@iam.tj> --- configure.ac | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/configure.ac b/configure.ac index e3a749a..b62170e 100644 --- a/configure.ac +++ b/configure.ac @@ -295,6 +295,8 @@ dnl External programs that we can use if they are available. dnl We will hard-code paths to these programs unless we cannot dnl detect them, in which case we'll search for the program dnl along the $PATH at runtime and fail if it's not there. +AC_PATH_PROG([DHCPRELAY], [dhcp-helper], [dhcp-helper], + [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_PATH_PROG([DNSMASQ], [dnsmasq], [dnsmasq], [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_PATH_PROG([RADVD], [radvd], [radvd], @@ -314,6 +316,8 @@ AC_PATH_PROG([OVSVSCTL], [ovs-vsctl], [ovs-vsctl], AC_PATH_PROG([SCRUB], [scrub], [scrub], [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) +AC_DEFINE_UNQUOTED([DHCPRELAY], ["$DHCPRELAY"], + [Location or name of the dhcp relay-agent program]) AC_DEFINE_UNQUOTED([DNSMASQ],["$DNSMASQ"], [Location or name of the dnsmasq program]) AC_DEFINE_UNQUOTED([RADVD],["$RADVD"], -- 1.8.1.2.433.g9808ce0.dirty
On 02/27/2013 09:57 PM, TJ wrote:
From: TJ <linux@iam.tj>
This variable should name the path to the system's DHCP relay daemon.
At this time the expected daemon is "dhcp-helper", a DHCP relay agent from Simon Kelly, author of dnsmasq.
The supporting code, however, has been designed to work with any suitable DHCP relay agent. Later patches could allow configuration of the agent's command-line arguments.
Currently the chosen agent must support:
-b <bridge_interface> (the virtual network to listen on) -i <physical_interface> (the interface to broadcast on) -r </path/to/pid.file> (the PID file of the daemon)
Signed-off-by: TJ <linux@iam.tj> --- configure.ac | 4 ++++ 1 file changed, 4 insertions(+)
This should have come much earlier in the series, probably as just another part of a single larger patch.
diff --git a/configure.ac b/configure.ac index e3a749a..b62170e 100644 --- a/configure.ac +++ b/configure.ac @@ -295,6 +295,8 @@ dnl External programs that we can use if they are available. dnl We will hard-code paths to these programs unless we cannot dnl detect them, in which case we'll search for the program dnl along the $PATH at runtime and fail if it's not there. +AC_PATH_PROG([DHCPRELAY], [dhcp-helper], [dhcp-helper], + [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_PATH_PROG([DNSMASQ], [dnsmasq], [dnsmasq], [/sbin:/usr/sbin:/usr/local/sbin:$PATH]) AC_PATH_PROG([RADVD], [radvd], [radvd], @@ -314,6 +316,8 @@ AC_PATH_PROG([OVSVSCTL], [ovs-vsctl], [ovs-vsctl], AC_PATH_PROG([SCRUB], [scrub], [scrub], [/sbin:/usr/sbin:/usr/local/sbin:$PATH])
+AC_DEFINE_UNQUOTED([DHCPRELAY], ["$DHCPRELAY"], + [Location or name of the dhcp relay-agent program]) AC_DEFINE_UNQUOTED([DNSMASQ],["$DNSMASQ"], [Location or name of the dnsmasq program]) AC_DEFINE_UNQUOTED([RADVD],["$RADVD"],
From: TJ <linux@iam.tj> Signed-off-by: TJ <linux@iam.tj> --- src/conf/network_conf.c | 1 + src/network/bridge_driver.c | 1 + 2 files changed, 2 insertions(+) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index c5eab01..43c277c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3,6 +3,7 @@ * * Copyright (C) 2006-2013 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange + * Copyright (C) 2013 TJ <linux@iam.tj> * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index a4cd727..1667ae6 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -4,6 +4,7 @@ * * Copyright (C) 2006-2013 Red Hat, Inc. * Copyright (C) 2006 Daniel P. Berrange + * Copyright (C) 2013 TJ <linux@iam.tj> * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public -- 1.8.1.2.433.g9808ce0.dirty
On 02/27/2013 07:57 PM, TJ wrote:
From: TJ <linux@iam.tj>
Signed-off-by: TJ <linux@iam.tj> --- src/conf/network_conf.c | 1 + src/network/bridge_driver.c | 1 + 2 files changed, 2 insertions(+)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index c5eab01..43c277c 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -3,6 +3,7 @@ * * Copyright (C) 2006-2013 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange + * Copyright (C) 2013 TJ <linux@iam.tj>
You can't add a copyright without also adding content. This patch needs to be squashed in with the patches that actually add content where you claim copyright. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: TJ <linux@iam.tj> Signed-off-by: TJ <linux@iam.tj> --- docs/formatnetwork.html.in | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 41a83fa..c4c4def 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in @@ -650,12 +650,20 @@ <dt><code>dhcp</code></dt> <dd>Also within the <code>ip</code> element there is an optional <code>dhcp</code> element. The presence of this element - enables DHCP services on the virtual network. It will further + enables DHCP services on the virtual network. It can further contain one or more <code>range</code> elements. The <code>dhcp</code> element supported for both IPv4 <span class="since">Since 0.3.0</span> and IPv6 <span class="since">Since 1.0.1</span>, but only for one IP address of each type per network. + Since $TODO.$FIXME it can optionally contain a boolean <code>enable</code> attribute + ('yes' or 'no') where the value defaults to 'yes', and a boolean + <code>relay</code> attribute where the value defaults to 'no'. + When <code>relay='yes'</code> any settings within the <code>dhcp</code> block + are ignored and a DHCP relay agent is started instead of a local DHCP server. + The DHCP relay daemon will listen on the network's bridge interface for + DHCP/BOOTP traffic and relay it via broadcast from the first interface declared + in the <code>forward</code> element. <dl> <dt><code>range</code></dt> <dd>The <code>start</code> and <code>end</code> attributes on the @@ -779,6 +787,18 @@ <ip family="ipv6" address="2001:db8:ca2:2::1" prefix="64" /> </network></pre> + <p>Here is the same configuration using a DHCP relay agent instead of a local DHCP server.</p> + <pre> + <network> + <name>local</name> + <bridge name="virbr1" /> + <forward mode="route" dev="eth1"/> + <ip address="192.168.122.1" netmask="255.255.255.0"> + <dhcp relay='yes'/> + </ip> + <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. -- 1.8.1.2.433.g9808ce0.dirty
On 02/27/2013 07:57 PM, TJ wrote:
From: TJ <linux@iam.tj>
Signed-off-by: TJ <linux@iam.tj> --- docs/formatnetwork.html.in | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 41a83fa..c4c4def 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in
Yay - your series added documentation! But it didn't add any unit tests (a new .xml file somewhere under tests/networkxml2argvdata or networkxml2xml{in,out} to verify that we can round-trip the new XML and generate the expected command line), and it failed to add the RelaxNG grammar specification under docs/schemas/network.rng, so there is still more to be added. Also, I like to put documentation FIRST in the series, as it then sets the stage for what the reviewer will be expecting in the rest of the series.
@@ -650,12 +650,20 @@ <dt><code>dhcp</code></dt> <dd>Also within the <code>ip</code> element there is an optional <code>dhcp</code> element. The presence of this element - enables DHCP services on the virtual network. It will further + enables DHCP services on the virtual network. It can further contain one or more <code>range</code> elements. The <code>dhcp</code> element supported for both IPv4 <span class="since">Since 0.3.0</span> and IPv6 <span class="since">Since 1.0.1</span>, but only for one IP address of each type per network. + Since $TODO.$FIXME it can optionally contain a boolean <code>enable</code> attribute
Instead of using $TODO.$FIXME, just use <span class="since">Since 1.0.4</span>. We can later touch that up as part of merging it in if you miss the 1.0.4 release window.
+ ('yes' or 'no') where the value defaults to 'yes', and a boolean + <code>relay</code> attribute where the value defaults to 'no'. + When <code>relay='yes'</code> any settings within the <code>dhcp</code> block + are ignored and a DHCP relay agent is started instead of a local DHCP server. + The DHCP relay daemon will listen on the network's bridge interface for + DHCP/BOOTP traffic and relay it via broadcast from the first interface declared + in the <code>forward</code> element. <dl> <dt><code>range</code></dt> <dd>The <code>start</code> and <code>end</code> attributes on the @@ -779,6 +787,18 @@ <ip family="ipv6" address="2001:db8:ca2:2::1" prefix="64" /> </network></pre>
+ <p>Here is the same configuration using a DHCP relay agent instead of a local DHCP server.</p> + <pre> + <network> + <name>local</name> + <bridge name="virbr1" /> + <forward mode="route" dev="eth1"/> + <ip address="192.168.122.1" netmask="255.255.255.0"> + <dhcp relay='yes'/> + </ip> + <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
Not your fault, but made obvious by your patch, so I'll fix this typo in 'variation' independently. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 28/02/13 04:16, Eric Blake wrote:
diff --git a/docs/formatnetwork.html.in b/docs/formatnetwork.html.in index 41a83fa..c4c4def 100644 --- a/docs/formatnetwork.html.in +++ b/docs/formatnetwork.html.in
Yay - your series added documentation!
I commented to a student that the documentation would make someone's day :)
But it didn't add any unit tests (a new .xml file somewhere under tests/networkxml2argvdata or networkxml2xml{in,out} to verify that we can round-trip the new XML and generate the expected command line), and it failed to add the RelaxNG grammar specification under docs/schemas/network.rng, so there is still more to be added.
OK, I'll do those. As you probably realise I'm new to libvirt coding style and requirements, only touching it to add needed functionality, but hopefully can make it suitable for give-back to the project. I'll work up a V2 once all comments are in.
Also, I like to put documentation FIRST in the series, as it then sets the stage for what the reviewer will be expecting in the rest of the series.
Makes sense.
+ Since $TODO.$FIXME it can optionally contain a boolean <code>enable</code> attribute
Instead of using $TODO.$FIXME, just use <span class="since">Since 1.0.4</span>. We can later touch that up as part of merging it in if you miss the 1.0.4 release window.
OK. I did that deliberately so it would be flagged since I didn't know what the policy was for unreleased versions and didn't want it to slip through the cracks.
On 02/27/2013 09:29 PM, TJ wrote:
But it didn't add any unit tests (a new .xml file somewhere under tests/networkxml2argvdata or networkxml2xml{in,out} to verify that we can round-trip the new XML and generate the expected command line), and it failed to add the RelaxNG grammar specification under docs/schemas/network.rng, so there is still more to be added.
OK, I'll do those. As you probably realise I'm new to libvirt coding style and requirements, only touching it to add needed functionality, but hopefully can make it suitable for give-back to the project. I'll work up a V2 once all comments are in.
I guess I forgot to say one thing up front - while my review may have sounded negative, that's just because it's easier to be terse when pointing out how things can be improved. The fact that I replied on the same day that you posted is a GOOD sign that your work is interesting, and likely to be accepted once it is whipped into shape. Check out HACKING, and feel free to ask questions if you have them. You may want to wait for Laine Stump to also review, as he is more of the expert on networking related patches, and will have more technical comments as opposed to my stylistic overview. And a big THANK YOU for contributing to the community. Too often, I forget to express gratitude for new contributors braving the unknown waters of posting to a list where they are not sure of the conventions. Honestly, we aren't trying to scare you off :) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Feb 27, 2013 at 09:38:33PM -0700, Eric Blake wrote:
On 02/27/2013 09:29 PM, TJ wrote: [...] And a big THANK YOU for contributing to the community. Too often, I forget to express gratitude for new contributors braving the unknown waters of posting to a list where they are not sure of the conventions. Honestly, we aren't trying to scare you off :)
Definitely ! Honnestly a rather good patchset for a first contribution :-) thanks ! Daniel -- Daniel Veillard | Open Source and Standards, Red Hat veillard@redhat.com | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | virtualization library http://libvirt.org/
On 02/27/2013 07:57 PM, TJ wrote:
From: TJ <linux@iam.tj>
Maintain backwards XML compatibility by assuming existing default values and only adding the additional XML properties if settings are not default.
Signed-off-by: TJ <linux@iam.tj> --- src/conf/network_conf.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)
+ def->dhcp_enabled = true; + if ((tmp = virXMLPropString(node, "enabled"))) { + def->dhcp_enabled = strncmp("no", tmp, 2) == 0 ? false : def->dhcp_enabled;
Yuck. This lets a user pass in trailing garbage. Use STREQ, not strncmp. For that matter, assigning def->dhcp_enabled to itself looks odd. I'd probably write: def->dhcp_enabled = true; if ((tmp = virXMLPropString(node, "enabled"))) { if (STREQ(tmp, "no")) def->dhcp_enabled = false; VIR_FREE(tmp); so that it doesn't look so screwy.
+ VIR_FREE(tmp); + } + + def->dhcp_relay = false; + if ((tmp = virXMLPropString(node, "relay"))) { + def->dhcp_relay = strncmp("yes", tmp, 3) == 0 ? true : def->dhcp_relay; + VIR_FREE(tmp);
Same comments.
+ }
cur = node->children; while (cur != NULL) { @@ -2180,12 +2193,19 @@ virNetworkIpDefFormat(virBufferPtr buf, virBufferEscapeString(buf, "<tftp root='%s' />\n", def->tftproot); } - if ((def->nranges || def->nhosts)) { + if ((def->nranges || def->nhosts) ||
As long as you are touching this, you can drop the redundant ().
+ !def->dhcp_enabled || def->dhcp_relay) { int ii; - virBufferAddLit(buf, "<dhcp>\n"); + virBufferAddLit(buf, "<dhcp"); + if (!def->dhcp_enabled) + virBufferAddLit(buf, " enabled='no'"); + if (def->dhcp_relay) + virBufferAddLit(buf, " relay='yes'"); + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2);
- for (ii = 0 ; ii < def->nranges ; ii++) { + for (ii = 0 ; def->nranges && ii < def->nranges ; ii++) {
This line is a spurious change.
char *saddr = virSocketAddrFormat(&def->ranges[ii].start); if (!saddr) goto error; @@ -2199,7 +2219,7 @@ virNetworkIpDefFormat(virBufferPtr buf, VIR_FREE(saddr); VIR_FREE(eaddr); } - for (ii = 0 ; ii < def->nhosts ; ii++) { + for (ii = 0 ; def->nhosts && ii < def->nhosts ; ii++) {
So is this one.
virBufferAddLit(buf, "<host "); if (def->hosts[ii].mac) virBufferAsprintf(buf, "mac='%s' ", def->hosts[ii].mac);
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 28/02/13 03:23, Eric Blake wrote:
On 02/27/2013 07:57 PM, TJ wrote:
From: TJ <linux@iam.tj>
Maintain backwards XML compatibility by assuming existing default values and only adding the additional XML properties if settings are not default.
Signed-off-by: TJ <linux@iam.tj> --- src/conf/network_conf.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)
+ def->dhcp_enabled = true; + if ((tmp = virXMLPropString(node, "enabled"))) { + def->dhcp_enabled = strncmp("no", tmp, 2) == 0 ? false : def->dhcp_enabled;
Yuck. This lets a user pass in trailing garbage. Use STREQ, not strncmp.
For that matter, assigning def->dhcp_enabled to itself looks odd. I'd probably write:
def->dhcp_enabled = true; if ((tmp = virXMLPropString(node, "enabled"))) { if (STREQ(tmp, "no")) def->dhcp_enabled = false; VIR_FREE(tmp);
so that it doesn't look so screwy.
I knew there was probably a better 'libvirt' style but couldn't find examples when I was looking for them.
+ !def->dhcp_enabled || def->dhcp_relay) { int ii; - virBufferAddLit(buf, "<dhcp>\n"); + virBufferAddLit(buf, "<dhcp"); + if (!def->dhcp_enabled) + virBufferAddLit(buf, " enabled='no'"); + if (def->dhcp_relay) + virBufferAddLit(buf, " relay='yes'"); + virBufferAddLit(buf, ">\n"); + virBufferAdjustIndent(buf, 2);
- for (ii = 0 ; ii < def->nranges ; ii++) { + for (ii = 0 ; def->nranges && ii < def->nranges ; ii++) {
This line is a spurious change.
Ahh yes. I introduced that to catch a bug found using gdb when nranges == 0 in the mistaken thinking that the loop would do at least one iteration. Later I realised the bug was caused by another issue entirely but forgot to revert that change.
On 02/27/2013 09:57 PM, TJ wrote:
From: TJ <linux@iam.tj>
Maintain backwards XML compatibility by assuming existing default values and only adding the additional XML properties if settings are not default.
Signed-off-by: TJ <linux@iam.tj> --- src/conf/network_conf.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 3fc01cf..259de0a 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -828,6 +828,19 @@ virNetworkDHCPDefParseXML(const char *networkName, {
xmlNodePtr cur; + char *tmp = NULL; + + def->dhcp_enabled = true; + if ((tmp = virXMLPropString(node, "enabled"))) { + def->dhcp_enabled = strncmp("no", tmp, 2) == 0 ? false : def->dhcp_enabled; + VIR_FREE(tmp); + }
See my earlier comment about the lack of a need for an enable attribute for dhcp.
+ + def->dhcp_relay = false; + if ((tmp = virXMLPropString(node, "relay"))) { + def->dhcp_relay = strncmp("yes", tmp, 3) == 0 ? true : def->dhcp_relay; + VIR_FREE(tmp); + }
At the end of this function you should check for dhcp_relay and if it's true, verify that nothing else was included in the <dhcp> element. It's okay/desirable for the parser to ignore extra stuff if it just doesn't understand it, or if it's unknown whether or not the backend driver would accept it, but in this case it's certain that if you want a dhcp relay, adding an <ip> element etc would be nonsensical, so we might as well save the backend driver the trouble of checking for it.
cur = node->children; while (cur != NULL) { @@ -2180,12 +2193,19 @@ virNetworkIpDefFormat(virBufferPtr buf, virBufferEscapeString(buf, "<tftp root='%s' />\n", def->tftproot); } - if ((def->nranges || def->nhosts)) { + if ((def->nranges || def->nhosts) || + !def->dhcp_enabled || def->dhcp_relay) { int ii; - virBufferAddLit(buf, "<dhcp>\n"); + virBufferAddLit(buf, "<dhcp"); + if (!def->dhcp_enabled) + virBufferAddLit(buf, " enabled='no'"); + if (def->dhcp_relay) + virBufferAddLit(buf, " relay='yes'"); + virBufferAddLit(buf, ">\n"); +
Since relay='yes' necessarily mandates that there can be no subelements (at least until such time as we figure out options we need to add for a dhcp relay and add them in), you could be extra tidy by just closing the element right here on a single line, then skipping the rest of the function (the parser should have already validated that there was no extra information in the case of relay='yes')
virBufferAdjustIndent(buf, 2);
- for (ii = 0 ; ii < def->nranges ; ii++) { + for (ii = 0 ; def->nranges && ii < def->nranges ; ii++) { char *saddr = virSocketAddrFormat(&def->ranges[ii].start); if (!saddr) goto error; @@ -2199,7 +2219,7 @@ virNetworkIpDefFormat(virBufferPtr buf, VIR_FREE(saddr); VIR_FREE(eaddr); } - for (ii = 0 ; ii < def->nhosts ; ii++) { + for (ii = 0 ; def->nhosts && ii < def->nhosts ; ii++) { virBufferAddLit(buf, "<host "); if (def->hosts[ii].mac) virBufferAsprintf(buf, "mac='%s' ", def->hosts[ii].mac);
participants (5)
-
Daniel Veillard -
Eric Blake -
Laine Stump -
TJ -
TJ