1:04 p.m.
(sorry for the previous message sent from "root" - I mistakenly ran
git send-email from a shell that was su'ed, and realized my mistake
midstream)
The first of these bugs is a regression that I didn't notice until it
was reported on libvirt-users list a couple days ago. The second is a
long-standing problem that I'm sure I saw discussed either on one of
the libvirt lists, or in a RHEL, Fedora, or upstream libvirt bugzilla
report, but can't find it right now.
1:04 p.m.
New subject: [libvirt] [PATCH 1/3] network driver: Start dnsmasq even if no dhcp ranges/hosts are specified.
This fixes a regression introduced in commit ad48df, and reported on
the libvirt-users list:
https://www.redhat.com/archives/libvirt-users/2011-March/msg00018.html
The problem in that commit was that we began searching a list of ip
address definitions (rather than just having one) to look for a dhcp
range or static host; when we didn't find any, our pointer (ipdef) was
left at NULL, and when ipdef was NULL, we returned without starting up
dnsmasq.
Previously dnsmasq was started even without any dhcp ranges or static
entries, because it's still useful for DNS services.
Another problem I noticed while investigating was that, if there are
IPv6 addresses, but no IPv4 addresses of any kind, we would jump out
at an ever higher level in the call chain.
This patch does the following:
1) networkBuildDnsmasqArgv() = all uses of ipdef are protected from
NULL dereference. (this patch doesn't change indentation, to make
review easier. The next patch will change just the
indentation). ipdef is intended to point to the first IPv4 address
with DHCP info (or the first IPv4 address if none of them have any
dhcp info).
2) networkStartDhcpDaemon() = if the loop looking for an ipdef with
DHCP info comes up empty, we then grab the first IPv4 def from the
list. Also, instead of returning if there are no IPv4 defs, we just
return if there are no IP defs at all (either v4 or v6). This way a
network that is IPv6-only will still get dnsmasq listening for DNS
queries.
3) in networkStartNetworkDaemon() - we will startup dhcp not just if there
are any IPv4 addresses, but also if there are any IPv6 addresses.
---
src/network/bridge_driver.c | 21 ++++++++++++++++-----
1 files changed, 16 insertions(+), 5 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index e666cbb..e3156fa 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -440,12 +440,9 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
virCommandPtr cmd) {
int r, ret = -1;
int nbleases = 0;
- char *bridgeaddr;
int ii;
virNetworkIpDefPtr tmpipdef;
- if (!(bridgeaddr = virSocketFormatAddr(&ipdef->address)))
- goto cleanup;
/*
* NB, be careful about syntax for dnsmasq options in long format.
*
@@ -501,6 +498,7 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
VIR_FREE(ipaddr);
}
+ if (ipdef) {
for (r = 0 ; r < ipdef->nranges ; r++) {
char *saddr = virSocketFormatAddr(&ipdef->ranges[r].start);
if (!saddr)
@@ -524,8 +522,12 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
* dnsmasq.
*/
if (!ipdef->nranges && ipdef->nhosts) {
+ char *bridgeaddr = virSocketFormatAddr(&ipdef->address);
+ if (!bridgeaddr)
+ goto cleanup;
virCommandAddArg(cmd, "--dhcp-range");
virCommandAddArgFormat(cmd, "%s,static", bridgeaddr);
+ VIR_FREE(bridgeaddr);
}
if (ipdef->nranges > 0) {
@@ -569,10 +571,10 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
virCommandAddArg(cmd, ipdef->bootfile);
}
}
+ }
ret = 0;
cleanup:
- VIR_FREE(bridgeaddr);
return ret;
}
@@ -594,7 +596,16 @@ networkStartDhcpDaemon(virNetworkObjPtr network)
if (ipdef->nranges || ipdef->nhosts)
break;
}
+ /* If no IPv4 addresses had dhcp info, pick the first (if there were any). */
if (!ipdef)
+ ipdef = virNetworkDefGetIpByIndex(network->def, AF_INET, 0);
+
+ /* If there are no IP addresses at all (v4 or v6), return now, since
+ * there won't be any address for dnsmasq to listen on anyway.
+ * If there are any addresses, even if no dhcp ranges or static entries,
+ * we should continue and run dnsmasq, just for the DNS capabilities.
+ */
+ if (!virNetworkDefGetIpByIndex(network->def, AF_UNSPEC, 0))
return 0;
if ((err = virFileMakePath(NETWORK_PID_DIR)) != 0) {
@@ -1678,7 +1689,7 @@ networkStartNetworkDaemon(struct network_driver *driver,
/* start dnsmasq if there are any IPv4 addresses */
- if (v4present && networkStartDhcpDaemon(network) < 0)
+ if ((v4present || v6present) && networkStartDhcpDaemon(network) < 0)
goto err3;
/* start radvd if there are any ipv6 addresses */
--
1.7.3.4
3:51 p.m.
New subject: [libvirt] [PATCH 1/3] network driver: Start dnsmasq even if no dhcp ranges/hosts are specified.
On 03/11/2011 12:04 PM, Laine Stump wrote:
1) networkBuildDnsmasqArgv() = all uses of ipdef are protected from
NULL dereference. (this patch doesn't change indentation, to make
review easier. The next patch will change just the
indentation). ipdef is intended to point to the first IPv4 address
with DHCP info (or the first IPv4 address if none of them have any
dhcp info).
2) networkStartDhcpDaemon() = if the loop looking for an ipdef with
DHCP info comes up empty, we then grab the first IPv4 def from the
list. Also, instead of returning if there are no IPv4 defs, we just
return if there are no IP defs at all (either v4 or v6). This way a
network that is IPv6-only will still get dnsmasq listening for DNS
queries.
3) in networkStartNetworkDaemon() - we will startup dhcp not just if there
are any IPv4 addresses, but also if there are any IPv6 addresses.
@@ -1678,7 +1689,7 @@ networkStartNetworkDaemon(struct network_driver *driver,
/* start dnsmasq if there are any IPv4 addresses */
- if (v4present && networkStartDhcpDaemon(network) < 0)
+ if ((v4present || v6present) && networkStartDhcpDaemon(network) < 0)
The comment is now out-of-date.
ACK with that nit fixed.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:50 p.m.
New subject: [libvirt] [PATCH 1/3] network driver: Start dnsmasq even if no dhcp ranges/hosts are specified.
On 03/11/2011 04:51 PM, Eric Blake wrote:
On 03/11/2011 12:04 PM, Laine Stump wrote:
> 1) networkBuildDnsmasqArgv() = all uses of ipdef are protected from
> NULL dereference. (this patch doesn't change indentation, to make
> review easier. The next patch will change just the
> indentation). ipdef is intended to point to the first IPv4 address
> with DHCP info (or the first IPv4 address if none of them have any
> dhcp info).
>
> 2) networkStartDhcpDaemon() = if the loop looking for an ipdef with
> DHCP info comes up empty, we then grab the first IPv4 def from the
> list. Also, instead of returning if there are no IPv4 defs, we just
> return if there are no IP defs at all (either v4 or v6). This way a
> network that is IPv6-only will still get dnsmasq listening for DNS
> queries.
>
> 3) in networkStartNetworkDaemon() - we will startup dhcp not just if there
> are any IPv4 addresses, but also if there are any IPv6 addresses.
> @@ -1678,7 +1689,7 @@ networkStartNetworkDaemon(struct network_driver *driver,
>
>
> /* start dnsmasq if there are any IPv4 addresses */
> - if (v4present&& networkStartDhcpDaemon(network)< 0)
> + if ((v4present || v6present)&& networkStartDhcpDaemon(network)< 0)
The comment is now out-of-date.
ACK with that nit fixed.
FIxed and pushed. Thanks!
1:04 p.m.
New subject: [libvirt] [PATCH 2/3] network driver: Fix indentation from previous commit
The previous commit put a large portion of networkBuildDnsmasqArgv
inside an if { } block. This readjusts the indentation.
---
src/network/bridge_driver.c | 122 +++++++++++++++++++++---------------------
1 files changed, 61 insertions(+), 61 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index e3156fa..484f35e 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -499,79 +499,79 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
}
if (ipdef) {
- for (r = 0 ; r < ipdef->nranges ; r++) {
- char *saddr = virSocketFormatAddr(&ipdef->ranges[r].start);
- if (!saddr)
- goto cleanup;
- char *eaddr = virSocketFormatAddr(&ipdef->ranges[r].end);
- if (!eaddr) {
+ for (r = 0 ; r < ipdef->nranges ; r++) {
+ char *saddr = virSocketFormatAddr(&ipdef->ranges[r].start);
+ if (!saddr)
+ goto cleanup;
+ char *eaddr = virSocketFormatAddr(&ipdef->ranges[r].end);
+ if (!eaddr) {
+ VIR_FREE(saddr);
+ goto cleanup;
+ }
+ virCommandAddArg(cmd, "--dhcp-range");
+ virCommandAddArgFormat(cmd, "%s,%s", saddr, eaddr);
VIR_FREE(saddr);
- goto cleanup;
+ VIR_FREE(eaddr);
+ nbleases += virSocketGetRange(&ipdef->ranges[r].start,
+ &ipdef->ranges[r].end);
}
- virCommandAddArg(cmd, "--dhcp-range");
- virCommandAddArgFormat(cmd, "%s,%s", saddr, eaddr);
- VIR_FREE(saddr);
- VIR_FREE(eaddr);
- nbleases += virSocketGetRange(&ipdef->ranges[r].start,
- &ipdef->ranges[r].end);
- }
- /*
- * For static-only DHCP, i.e. with no range but at least one host element,
- * we have to add a special --dhcp-range option to enable the service in
- * dnsmasq.
- */
- if (!ipdef->nranges && ipdef->nhosts) {
- char *bridgeaddr = virSocketFormatAddr(&ipdef->address);
- if (!bridgeaddr)
- goto cleanup;
- virCommandAddArg(cmd, "--dhcp-range");
- virCommandAddArgFormat(cmd, "%s,static", bridgeaddr);
- VIR_FREE(bridgeaddr);
- }
+ /*
+ * For static-only DHCP, i.e. with no range but at least one host element,
+ * we have to add a special --dhcp-range option to enable the service in
+ * dnsmasq.
+ */
+ if (!ipdef->nranges && ipdef->nhosts) {
+ char *bridgeaddr = virSocketFormatAddr(&ipdef->address);
+ if (!bridgeaddr)
+ goto cleanup;
+ virCommandAddArg(cmd, "--dhcp-range");
+ virCommandAddArgFormat(cmd, "%s,static", bridgeaddr);
+ VIR_FREE(bridgeaddr);
+ }
- if (ipdef->nranges > 0) {
- virCommandAddArgFormat(cmd, "--dhcp-lease-max=%d", nbleases);
- }
+ if (ipdef->nranges > 0) {
+ virCommandAddArgFormat(cmd, "--dhcp-lease-max=%d", nbleases);
+ }
+
+ if (ipdef->nranges || ipdef->nhosts)
+ virCommandAddArg(cmd, "--dhcp-no-override");
- if (ipdef->nranges || ipdef->nhosts)
- virCommandAddArg(cmd, "--dhcp-no-override");
+ if (ipdef->nhosts > 0) {
+ dnsmasqContext *dctx = dnsmasqContextNew(network->def->name,
+ DNSMASQ_STATE_DIR);
+ if (dctx == NULL) {
+ virReportOOMError();
+ goto cleanup;
+ }
- if (ipdef->nhosts > 0) {
- dnsmasqContext *dctx = dnsmasqContextNew(network->def->name,
- DNSMASQ_STATE_DIR);
- if (dctx == NULL) {
- virReportOOMError();
- goto cleanup;
+ if (networkSaveDnsmasqHostsfile(ipdef, dctx, false) == 0) {
+ virCommandAddArgPair(cmd, "--dhcp-hostsfile",
+ dctx->hostsfile->path);
+ }
+ dnsmasqContextFree(dctx);
}
- if (networkSaveDnsmasqHostsfile(ipdef, dctx, false) == 0) {
- virCommandAddArgPair(cmd, "--dhcp-hostsfile",
- dctx->hostsfile->path);
+ if (ipdef->tftproot) {
+ virCommandAddArgList(cmd, "--enable-tftp",
+ "--tftp-root", ipdef->tftproot,
+ NULL);
}
- dnsmasqContextFree(dctx);
- }
-
- if (ipdef->tftproot) {
- virCommandAddArgList(cmd, "--enable-tftp",
- "--tftp-root", ipdef->tftproot,
- NULL);
- }
- if (ipdef->bootfile) {
- virCommandAddArg(cmd, "--dhcp-boot");
- if (VIR_SOCKET_HAS_ADDR(&ipdef->bootserver)) {
- char *bootserver = virSocketFormatAddr(&ipdef->bootserver);
+ if (ipdef->bootfile) {
+ virCommandAddArg(cmd, "--dhcp-boot");
+ if (VIR_SOCKET_HAS_ADDR(&ipdef->bootserver)) {
+ char *bootserver = virSocketFormatAddr(&ipdef->bootserver);
- if (!bootserver)
- goto cleanup;
- virCommandAddArgFormat(cmd, "%s%s%s",
- ipdef->bootfile, ",,", bootserver);
- VIR_FREE(bootserver);
- } else {
- virCommandAddArg(cmd, ipdef->bootfile);
+ if (!bootserver)
+ goto cleanup;
+ virCommandAddArgFormat(cmd, "%s%s%s",
+ ipdef->bootfile, ",,", bootserver);
+ VIR_FREE(bootserver);
+ } else {
+ virCommandAddArg(cmd, ipdef->bootfile);
+ }
}
}
- }
ret = 0;
cleanup:
--
1.7.3.4
3:51 p.m.
New subject: [libvirt] [PATCH 2/3] network driver: Fix indentation from previous commit
On 03/11/2011 12:04 PM, Laine Stump wrote:
The previous commit put a large portion of networkBuildDnsmasqArgv
inside an if { } block. This readjusts the indentation.
---
src/network/bridge_driver.c | 122 +++++++++++++++++++++---------------------
1 files changed, 61 insertions(+), 61 deletions(-)
ACK. And thanks for splitting like this - it makes reviews much easier :)
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:51 p.m.
New subject: [libvirt] [PATCH 2/3] network driver: Fix indentation from previous commit
On 03/11/2011 04:51 PM, Eric Blake wrote:
On 03/11/2011 12:04 PM, Laine Stump wrote:
> The previous commit put a large portion of networkBuildDnsmasqArgv
> inside an if { } block. This readjusts the indentation.
> ---
> src/network/bridge_driver.c | 122 +++++++++++++++++++++---------------------
> 1 files changed, 61 insertions(+), 61 deletions(-)
ACK. And thanks for splitting like this - it makes reviews much easier :)
It was your idea! ;-) Pushed.
1:04 p.m.
New subject: [libvirt] [PATCH 3/3] network driver: Use a separate dhcp leases file for each network
By default, all dnsmasq processes share the same leases file. libvirt
also uses the --dhcp-lease-max option to control the maximum number of
leases allowed. The problem is that libvirt puts in a number equal to
the number of addresses in the range for the one network handled by a
single instance of dnsmasq, but dnsmasq checks the total number of
leases in the file (which could potentially contain many more).
The solution is to tell each instance of dnsmasq to create and use its
own leases file. (/var/lib/libvirt/network/<net-name>.leases).
This file is created by dnsmasq when it starts, but not deleted when
it exists. This is fine when the network is just being stopped, but if
the leases file was left around when a network was undefined, we could
end up with an ever-increasing number of dead files - instead, we
explicitly unlink the leases file when a network is undefined.
Note that Ubuntu carries a patch against an older version of libvirt for this:
hhttps://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/713071
ttp://bazaar.launchpad.net/~serge-hallyn/ubuntu/maverick/libvirt/bugall/r...
I was certain I'd also seen discussion of this on libvir-list or
libvirt-users, but couldn't find it.
---
src/network/bridge_driver.c | 22 ++++++++++++++++++++++
1 files changed, 22 insertions(+), 0 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index 484f35e..1fd179c 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -109,6 +109,16 @@ static void networkReloadIptablesRules(struct network_driver
*driver);
static struct network_driver *driverState = NULL;
static char *
+networkDnsmasqLeaseFileName(const char *netname)
+{
+ char *leasefile;
+
+ virAsprintf(&leasefile, DNSMASQ_STATE_DIR "/%s.leases",
+ netname);
+ return leasefile;
+}
+
+static char *
networkRadvdPidfileBasename(const char *netname)
{
/* this is simple but we want to be sure it's consistently done */
@@ -531,6 +541,11 @@ networkBuildDnsmasqArgv(virNetworkObjPtr network,
}
if (ipdef->nranges > 0) {
+ char *leasefile = networkDnsmasqLeaseFileName(network->def->name);
+ if (!leasefile)
+ goto cleanup;
+ virCommandAddArgFormat(cmd, "--dhcp-leasefile=%s", leasefile);
+ VIR_FREE(leasefile);
virCommandAddArgFormat(cmd, "--dhcp-lease-max=%d", nbleases);
}
@@ -2195,12 +2210,19 @@ static int networkUndefine(virNetworkPtr net) {
}
if (dhcp_present) {
+ char *leasefile;
dnsmasqContext *dctx = dnsmasqContextNew(network->def->name,
DNSMASQ_STATE_DIR);
if (dctx == NULL)
goto cleanup;
dnsmasqDelete(dctx);
dnsmasqContextFree(dctx);
+
+ leasefile = networkDnsmasqLeaseFileName(network->def->name);
+ if (!leasefile)
+ goto cleanup;
+ unlink(leasefile);
+ VIR_FREE(leasefile);
}
if (v6present) {
--
1.7.3.4
3:53 p.m.
New subject: [libvirt] [PATCH 3/3] network driver: Use a separate dhcp leases file for each network
On 03/11/2011 12:04 PM, Laine Stump wrote:
By default, all dnsmasq processes share the same leases file.
libvirt
also uses the --dhcp-lease-max option to control the maximum number of
leases allowed. The problem is that libvirt puts in a number equal to
the number of addresses in the range for the one network handled by a
single instance of dnsmasq, but dnsmasq checks the total number of
leases in the file (which could potentially contain many more).
The solution is to tell each instance of dnsmasq to create and use its
own leases file. (/var/lib/libvirt/network/<net-name>.leases).
This file is created by dnsmasq when it starts, but not deleted when
it exists. This is fine when the network is just being stopped, but if
the leases file was left around when a network was undefined, we could
end up with an ever-increasing number of dead files - instead, we
explicitly unlink the leases file when a network is undefined.
Note that Ubuntu carries a patch against an older version of libvirt for this:
hhttps://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/713071
ttp://bazaar.launchpad.net/~serge-hallyn/ubuntu/maverick/libvirt/bugall/r...
I was certain I'd also seen discussion of this on libvir-list or
libvirt-users, but couldn't find it.
Be that as it may, it's a real fix worth applying.
ACK.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
10:52 p.m.
New subject: [libvirt] [PATCH 3/3] network driver: Use a separate dhcp leases file for each network
On 03/11/2011 04:53 PM, Eric Blake wrote:
On 03/11/2011 12:04 PM, Laine Stump wrote:
> By default, all dnsmasq processes share the same leases file. libvirt
> also uses the --dhcp-lease-max option to control the maximum number of
> leases allowed. The problem is that libvirt puts in a number equal to
> the number of addresses in the range for the one network handled by a
> single instance of dnsmasq, but dnsmasq checks the total number of
> leases in the file (which could potentially contain many more).
>
> The solution is to tell each instance of dnsmasq to create and use its
> own leases file. (/var/lib/libvirt/network/<net-name>.leases).
>
> This file is created by dnsmasq when it starts, but not deleted when
> it exists. This is fine when the network is just being stopped, but if
> the leases file was left around when a network was undefined, we could
> end up with an ever-increasing number of dead files - instead, we
> explicitly unlink the leases file when a network is undefined.
>
> Note that Ubuntu carries a patch against an older version of libvirt for this:
>
> hhttps://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/713071
> ttp://bazaar.launchpad.net/~serge-hallyn/ubuntu/maverick/libvirt/bugall/r...
>
> I was certain I'd also seen discussion of this on libvir-list or
> libvirt-users, but couldn't find it.
Be that as it may, it's a real fix worth applying.
ACK.
Yeah, I was just hoping to find the reference so I could go back and
update whatever bug report there might be...
Thanks. Pushed.
7:36 a.m.
New subject: [libvirt] [PATCH 3/3] network driver: Use a separate dhcp leases file for each network
On 03/11/2011 11:52 PM, Laine Stump wrote:
On 03/11/2011 04:53 PM, Eric Blake wrote:
> On 03/11/2011 12:04 PM, Laine Stump wrote:
>> By default, all dnsmasq processes share the same leases file. libvirt
>> also uses the --dhcp-lease-max option to control the maximum number of
>> leases allowed. The problem is that libvirt puts in a number equal to
>> the number of addresses in the range for the one network handled by a
>> single instance of dnsmasq, but dnsmasq checks the total number of
>> leases in the file (which could potentially contain many more).
>>
>> The solution is to tell each instance of dnsmasq to create and use its
>> own leases file. (/var/lib/libvirt/network/<net-name>.leases).
>>
>> This file is created by dnsmasq when it starts, but not deleted when
>> it exists. This is fine when the network is just being stopped, but if
>> the leases file was left around when a network was undefined, we could
>> end up with an ever-increasing number of dead files - instead, we
>> explicitly unlink the leases file when a network is undefined.
>>
>> Note that Ubuntu carries a patch against an older version of libvirt
>> for this:
>>
>> hhttps://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/713071
>>
ttp://bazaar.launchpad.net/~serge-hallyn/ubuntu/maverick/libvirt/bugall/r...
>>
>>
>> I was certain I'd also seen discussion of this on libvir-list or
>> libvirt-users, but couldn't find it.
> Be that as it may, it's a real fix worth applying.
>
> ACK.
Yeah, I was just hoping to find the reference so I could go back and
update whatever bug report there might be...
And now I've found them:
https://bugzilla.redhat.com/show_bug.cgi?id=674356
https://bugzilla.redhat.com/show_bug.cgi?id=663664
5005
days inactive
5006
days old
10 comments
2 participants
participants (2)
-
Eric Blake -
Laine Stump