2:53 p.m.
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(-)
3:52 p.m.
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
8:18 p.m.
New subject: [libvirt] [PATCH 01/10] conf: DHCP - add state for DHCP Relay and On/Off switch
Signed-off-by: TJ <linux(a)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
9:15 p.m.
New subject: [libvirt] [PATCH 01/10] conf: DHCP - add state for DHCP Relay and On/Off switch
On 02/27/2013 07:18 PM, TJ wrote:
Signed-off-by: TJ <linux(a)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
9:26 p.m.
New subject: [libvirt] [PATCH 01/10] conf: DHCP - add state for DHCP Relay and On/Off switch
On 28/02/13 03:15, Eric Blake wrote:
On 02/27/2013 07:18 PM, TJ wrote:
> Signed-off-by: TJ <linux(a)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.
9:49 p.m.
New subject: [libvirt] [PATCH 01/10] conf: DHCP - add state for DHCP Relay and On/Off switch
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(a)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
9:51 p.m.
New subject: [libvirt] [PATCH 01/10] conf: DHCP - add state for DHCP Relay and On/Off switch
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(a)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.
10:45 p.m.
New subject: [libvirt] [PATCH 01/10] conf: DHCP - add state for DHCP Relay and On/Off switch
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
1:25 p.m.
New subject: [libvirt] [PATCH 01/10] conf: DHCP - add state for DHCP Relay and On/Off switch
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.
1:45 p.m.
New subject: [libvirt] [PATCH 01/10] conf: DHCP - add state for DHCP Relay and On/Off switch
On 02/27/2013 09:18 PM, TJ wrote:
Signed-off-by: TJ <linux(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
5:06 p.m.
New subject: [libvirt] [PATCH 01/10] conf: DHCP - add state for DHCP Relay and On/Off switch
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
8:37 p.m.
New subject: [libvirt] [PATCH 02/10] conf: Network - add ability to read/write XML DHCP state
From: "TJ" <libvirt(a)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(a)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(a)iam.tj>
References: <512E724A.10805(a)iam.tj>
From: "TJ" <libvirt(a)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(a)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(a)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(a)iam.tj>
References: <512E724A.10805(a)iam.tj>
From: "TJ" <libvirt(a)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(a)redhat.com
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(a)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
From ed60402970098706b49e3d71c008520168968277 Mon Sep 17 00:00:00 2001
In-Reply-To: <512E724A.10805(a)iam.tj>
References: <512E724A.10805(a)iam.tj>
From: "TJ" <libvirt(a)iam.tj>
Date: Tue, 26 Feb 2013 17:43:46 +0000
Subject: [PATCH 05/10] network: Bridge - use IPv4 and IPv6 active DHCP stanza
pointers
To: libvir-list(a)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(a)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(a)iam.tj>
References: <512E724A.10805(a)iam.tj>
From: "TJ" <libvirt(a)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(a)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(a)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(a)iam.tj>
References: <512E724A.10805(a)iam.tj>
From: "TJ" <libvirt(a)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(a)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(a)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(a)iam.tj>
References: <512E724A.10805(a)iam.tj>
From: "TJ" <libvirt(a)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(a)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(a)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(a)iam.tj>
References: <512E724A.10805(a)iam.tj>
From: "TJ" <libvirt(a)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(a)redhat.com
Signed-off-by: TJ <linux(a)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(a)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(a)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(a)iam.tj>
References: <512E724A.10805(a)iam.tj>
From: "TJ" <libvirt(a)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(a)redhat.com
Signed-off-by: TJ <linux(a)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
8:57 p.m.
New subject: [libvirt] [PATCH 02/10] conf: Network - add ability to read/write XML DHCP state
From: TJ <linux(a)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(a)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
8:57 p.m.
New subject: [libvirt] [PATCH 03/10] conf: Network - add pointers to enabled virNetworkIpDef DHCP settings
From: TJ <linux(a)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(a)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
9:26 p.m.
New subject: [libvirt] [PATCH 03/10] conf: Network - add pointers to enabled virNetworkIpDef DHCP settings
On 02/27/2013 07:57 PM, TJ wrote:
From: TJ <linux(a)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(a)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
1:37 p.m.
New subject: [libvirt] [PATCH 03/10] conf: Network - add pointers to enabled virNetworkIpDef DHCP settings
On 02/27/2013 09:57 PM, TJ wrote:
From: TJ <linux(a)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(a)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;
8:57 p.m.
New subject: [libvirt] [PATCH 04/10] conf: Network - keep track of active DHCP stanza in virNetworkDef
From: TJ <linux(a)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(a)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
1:45 p.m.
New subject: [libvirt] [PATCH 04/10] conf: Network - keep track of active DHCP stanza in virNetworkDef
On 02/27/2013 09:57 PM, TJ wrote:
From: TJ <linux(a)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(a)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);
8:57 p.m.
New subject: [libvirt] [PATCH 05/10] network: Bridge - use IPv4 and IPv6 active DHCP stanza pointers
From: TJ <linux(a)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(a)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
1:51 p.m.
New subject: [libvirt] [PATCH 05/10] network: Bridge - use IPv4 and IPv6 active DHCP stanza pointers
On 02/27/2013 09:57 PM, TJ wrote:
From: TJ <linux(a)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(a)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;
5:07 p.m.
New subject: [libvirt] [PATCH 05/10] network: Bridge - use IPv4 and IPv6 active DHCP stanza pointers
On 28/02/13 19:51, Laine Stump wrote:
On 02/27/2013 09:57 PM, TJ wrote:
> From: TJ <linux(a)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(a)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.
5:11 p.m.
New subject: [libvirt] [PATCH 05/10] network: Bridge - use IPv4 and IPv6 active DHCP stanza pointers
On 28/02/13 19:51, Laine Stump wrote:
On 02/27/2013 09:57 PM, TJ wrote:
> From: TJ <linux(a)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(a)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.
8:57 p.m.
New subject: [libvirt] [PATCH 06/10] network: Bridge - Add support for a DHCP Relay Agent
From: TJ <linux(a)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(a)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
9:38 p.m.
New subject: [libvirt] [PATCH 06/10] network: Bridge - Add support for a DHCP Relay Agent
On 02/27/2013 07:57 PM, TJ wrote:
From: TJ <linux(a)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
9:47 p.m.
New subject: [libvirt] [PATCH 06/10] network: Bridge - Add support for a DHCP Relay Agent
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.
10:09 p.m.
New subject: [libvirt] [PATCH 06/10] network: Bridge - Add support for a DHCP Relay Agent
On 02/27/2013 07:57 PM, TJ wrote:
From: TJ <linux(a)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
12:32 a.m.
New subject: [libvirt] [PATCH 06/10] network: Bridge - Add support for a DHCP Relay Agent
On 28/02/13 04:09, Eric Blake wrote:
On 02/27/2013 07:57 PM, TJ wrote:
> From: TJ <linux(a)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.
12:08 p.m.
New subject: [libvirt] [PATCH 06/10] network: Bridge - Add support for a DHCP Relay Agent
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
2:03 p.m.
New subject: [libvirt] [PATCH 06/10] network: Bridge - Add support for a DHCP Relay Agent
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.
1:56 p.m.
New subject: [libvirt] [PATCH 06/10] network: Bridge - Add support for a DHCP Relay Agent
On 02/27/2013 09:57 PM, TJ wrote:
From: TJ <linux(a)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(a)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
5:12 p.m.
New subject: [libvirt] [PATCH 06/10] network: Bridge - Add support for a DHCP Relay Agent
On 28/02/13 19:56, Laine Stump wrote:
On 02/27/2013 09:57 PM, TJ wrote:
> From: TJ <linux(a)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>.
8:57 p.m.
New subject: [libvirt] [PATCH 07/10] network: Bridge - don't offer dnsmasq DHCP services when DHCP relay is enabled
From: TJ <linux(a)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(a)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
2:08 p.m.
New subject: [libvirt] [PATCH 07/10] network: Bridge - don't offer dnsmasq DHCP services when DHCP relay is enabled
On 02/27/2013 09:57 PM, TJ wrote:
From: TJ <linux(a)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(a)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 */
8:57 p.m.
New subject: [libvirt] [PATCH 08/10] configure: Add DHCPRELAY to the set of external program definitions
From: TJ <linux(a)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(a)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
2:10 p.m.
New subject: [libvirt] [PATCH 08/10] configure: Add DHCPRELAY to the set of external program definitions
On 02/27/2013 09:57 PM, TJ wrote:
From: TJ <linux(a)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(a)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"],
8:57 p.m.
New subject: [libvirt] [PATCH 09/10] Add copyright attribution for DHCP relay functionality
From: TJ <linux(a)iam.tj>
Signed-off-by: TJ <linux(a)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(a)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(a)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
10:10 p.m.
New subject: [libvirt] [PATCH 09/10] Add copyright attribution for DHCP relay functionality
On 02/27/2013 07:57 PM, TJ wrote:
From: TJ <linux(a)iam.tj>
Signed-off-by: TJ <linux(a)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(a)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
8:57 p.m.
New subject: [libvirt] [PATCH 10/10] docs: Describe the <dhcp> 'enable' and 'relay' attributes
From: TJ <linux(a)iam.tj>
Signed-off-by: TJ <linux(a)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
10:16 p.m.
New subject: [libvirt] [PATCH 10/10] docs: Describe the <dhcp> 'enable' and 'relay' attributes
On 02/27/2013 07:57 PM, TJ wrote:
From: TJ <linux(a)iam.tj>
Signed-off-by: TJ <linux(a)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
10:29 p.m.
New subject: [libvirt] [PATCH 10/10] docs: Describe the <dhcp> 'enable' and 'relay' attributes
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.
10:38 p.m.
New subject: [libvirt] [PATCH 10/10] docs: Describe the <dhcp> 'enable' and 'relay' attributes
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
6:31 a.m.
New subject: [libvirt] [PATCH 10/10] docs: Describe the <dhcp> 'enable' and 'relay' attributes
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(a)redhat.com | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | virtualization library http://libvirt.org/
9:23 p.m.
New subject: [libvirt] [PATCH 02/10] conf: Network - add ability to read/write XML DHCP state
On 02/27/2013 07:57 PM, TJ wrote:
From: TJ <linux(a)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(a)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
9:35 p.m.
New subject: [libvirt] [PATCH 02/10] conf: Network - add ability to read/write XML DHCP state
On 28/02/13 03:23, Eric Blake wrote:
On 02/27/2013 07:57 PM, TJ wrote:
> From: TJ <linux(a)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(a)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.
1:40 p.m.
New subject: [libvirt] [PATCH 02/10] conf: Network - add ability to read/write XML DHCP state
On 02/27/2013 09:57 PM, TJ wrote:
From: TJ <linux(a)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(a)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);
4285
days inactive
4286
days old
44 comments
5 participants
participants (5)
-
Daniel Veillard -
Eric Blake -
Laine Stump -
TJ -
TJ