[libvirt] [PATCHv2 0/2] network: add netmask to dhcp range of dnsmasq conf file for IPv4
I sent V1 of this patch way back in October: https://www.redhat.com/archives/libvir-list/2018-October/msg00889.html but then self-NACKed it because I had written the patch to add the network prefix to dhcp-range, but IPv4 requires the netmask rather than range. Unfortunately, when I modified the patch accordingly the getnameinfo() function began failing, I couldn't immediately see why, and I had to pack everything up and go to KVM Forum. After that, I promptly forgot to look back at it. Today I finally revisited the problem (after the person who originally found it sent a reminder), and discovered the failure of getnameinfo() was due to a bug in a function that I added to the virSocketAddr library in 2010! Laine Stump (2): util: set missing data length in virSocketAddrPrefixToNetmask() network: add netmask to dhcp range of dnsmasq conf file for IPv4 src/network/bridge_driver.c | 27 +++++++++++++++---- src/util/virsocketaddr.c | 2 ++ .../dhcp6-nat-network.conf | 2 +- .../networkxml2confdata/isolated-network.conf | 2 +- .../nat-network-dns-srv-record-minimal.conf | 2 +- .../nat-network-dns-srv-record.conf | 2 +- .../nat-network-dns-txt-record.conf | 2 +- .../networkxml2confdata/nat-network-mtu.conf | 2 +- .../nat-network-name-with-quotes.conf | 2 +- tests/networkxml2confdata/nat-network.conf | 2 +- .../networkxml2confdata/netboot-network.conf | 2 +- .../netboot-proxy-network.conf | 2 +- .../networkxml2confdata/ptr-domains-auto.conf | 2 +- 13 files changed, 35 insertions(+), 16 deletions(-) -- 2.20.1
This fixes a bug that has been present since the original version of the function was pushed in commit 1ab80f3 on Nov. 26 2010 (by me). The virSocketAddr::len was not being set. Apparently until now we were always calling virSocketAddrPrefixToNetmask() with a virSocketAddr object that was already (coincidentally) initialized for the proper address family, but the bug became apparent when trying to use it to fill in an otherwise uninitialized object. Signed-off-by: Laine Stump <laine@laine.org> --- src/util/virsocketaddr.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 4bc14bbd15..ccfaeabe13 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -1032,6 +1032,7 @@ virSocketAddrPrefixToNetmask(unsigned int prefix, ip = prefix ? ~((1 << (32 - prefix)) - 1) : 0; netmask->data.inet4.sin_addr.s_addr = htonl(ip); netmask->data.stor.ss_family = AF_INET; + netmask->len = sizeof(struct sockaddr_in); result = 0; } else if (family == AF_INET6) { @@ -1055,6 +1056,7 @@ virSocketAddrPrefixToNetmask(unsigned int prefix, netmask->data.inet6.sin6_addr.s6_addr[i++] = 0; } netmask->data.stor.ss_family = AF_INET6; + netmask->len = sizeof(struct sockaddr_in6); result = 0; } -- 2.20.1
On 2/18/19 6:21 PM, Laine Stump wrote:
This fixes a bug that has been present since the original version of the function was pushed in commit 1ab80f3 on Nov. 26 2010 (by me). The virSocketAddr::len was not being set.
Apparently until now we were always calling virSocketAddrPrefixToNetmask() with a virSocketAddr object that was already (coincidentally) initialized for the proper address family, but the bug became apparent when trying to use it to fill in an otherwise uninitialized object.
Signed-off-by: Laine Stump <laine@laine.org> --- src/util/virsocketaddr.c | 2 ++ 1 file changed, 2 insertions(+)
I'm OK with this change as is - just curious on the below query... Reviewed-by: John Ferlan <jferlan@redhat.com> John
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 4bc14bbd15..ccfaeabe13 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -1032,6 +1032,7 @@ virSocketAddrPrefixToNetmask(unsigned int prefix, ip = prefix ? ~((1 << (32 - prefix)) - 1) : 0; netmask->data.inet4.sin_addr.s_addr = htonl(ip); netmask->data.stor.ss_family = AF_INET; + netmask->len = sizeof(struct sockaddr_in);
Hmmm.. how similar to virSocketAddrSetIPv4AddrNetOrder overall? I was looking for "data\.inet.*=" via cscope and found that...
result = 0;
} else if (family == AF_INET6) { @@ -1055,6 +1056,7 @@ virSocketAddrPrefixToNetmask(unsigned int prefix, netmask->data.inet6.sin6_addr.s6_addr[i++] = 0; } netmask->data.stor.ss_family = AF_INET6; + netmask->len = sizeof(struct sockaddr_in6);
My brain hurts thinking if similar to virSocketAddrSetIPv6AddrNetOrder
result = 0; }
On 2/20/19 4:10 PM, John Ferlan wrote:
On 2/18/19 6:21 PM, Laine Stump wrote:
This fixes a bug that has been present since the original version of the function was pushed in commit 1ab80f3 on Nov. 26 2010 (by me). The virSocketAddr::len was not being set.
Apparently until now we were always calling virSocketAddrPrefixToNetmask() with a virSocketAddr object that was already (coincidentally) initialized for the proper address family, but the bug became apparent when trying to use it to fill in an otherwise uninitialized object.
Signed-off-by: Laine Stump <laine@laine.org> --- src/util/virsocketaddr.c | 2 ++ 1 file changed, 2 insertions(+)
I'm OK with this change as is - just curious on the below query...
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
diff --git a/src/util/virsocketaddr.c b/src/util/virsocketaddr.c index 4bc14bbd15..ccfaeabe13 100644 --- a/src/util/virsocketaddr.c +++ b/src/util/virsocketaddr.c @@ -1032,6 +1032,7 @@ virSocketAddrPrefixToNetmask(unsigned int prefix, ip = prefix ? ~((1 << (32 - prefix)) - 1) : 0; netmask->data.inet4.sin_addr.s_addr = htonl(ip); netmask->data.stor.ss_family = AF_INET; + netmask->len = sizeof(struct sockaddr_in);
Hmmm.. how similar to virSocketAddrSetIPv4AddrNetOrder overall? I was looking for "data\.inet.*=" via cscope and found that...
Interesting. These 3 lines *are* identical in function to calling virSocketAddrSetIPv4Addr() (the one that does an htonl() on the address).
result = 0;
} else if (family == AF_INET6) { @@ -1055,6 +1056,7 @@ virSocketAddrPrefixToNetmask(unsigned int prefix, netmask->data.inet6.sin6_addr.s6_addr[i++] = 0; } netmask->data.stor.ss_family = AF_INET6; + netmask->len = sizeof(struct sockaddr_in6);
My brain hurts thinking if similar to virSocketAddrSetIPv6AddrNetOrder
Yeah. I'm not in the mood to figure out what's the right byte order for the input to what happens in virSocketAddrSetIPv6Addr() and virSocketAddrSetIPv6AddrNetOrder() is identical to what's currently done in the IPv6 clause of virSocketAddrPrefixToNetmask(), so I think I'd rather leave all of it for clarity's sake (and also to avoid the potential of having my bug fix create yet another regression :-P)
dnsmasq documentation says that the *IPv4* prefix/network address/broadcast address sent to dhcp clients will be automatically determined by dnsmasq by looking at the interface it's listening on, so the original libvirt code did not add a netmask to the dnsmasq commandline (or later, the dnsmasq conf file). For *IPv6* however, dnsmasq apparently cannot automatically determine the prefix (functionally the same as a netmask), and it must be explicitly provided in the conf file (as a part of the dhcp-range option). So many years after IPv4 DHCP support had been added, when IPv6 dhcp support was added the prefix was included at the end of the dhcp-range setting, but only for IPv6. Recently a user reported (privately, because they suspected a possible security implication, which turned out to be unfounded) a bug on a host where one of the interfaces was a superset of the libvirt network where dhcp is needed (e.g., the host's ethernet is 10.0.0.20/8, and the libvirt network is 10.10.0.1/24). For some reason dnsmasq was supplying the netmask for the /8 network to clients requesting an address on the /24 interface. This seems like a bug in dnsmasq, but even if/when it gets fixed there, it looks like there is no harm in just always adding the netmask to all IPv4 dhcp-range options similar to how prefix is added to all IPv6 dhcp-range options. Signed-off-by: Laine Stump <laine@laine.org> --- src/network/bridge_driver.c | 27 +++++++++++++++---- .../dhcp6-nat-network.conf | 2 +- .../networkxml2confdata/isolated-network.conf | 2 +- .../nat-network-dns-srv-record-minimal.conf | 2 +- .../nat-network-dns-srv-record.conf | 2 +- .../nat-network-dns-txt-record.conf | 2 +- .../networkxml2confdata/nat-network-mtu.conf | 2 +- .../nat-network-name-with-quotes.conf | 2 +- tests/networkxml2confdata/nat-network.conf | 2 +- .../networkxml2confdata/netboot-network.conf | 2 +- .../netboot-proxy-network.conf | 2 +- .../networkxml2confdata/ptr-domains-auto.conf | 2 +- 12 files changed, 33 insertions(+), 16 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 6d80818e40..9fa902896b 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1320,11 +1320,28 @@ networkDnsmasqConfContents(virNetworkObjPtr obj, !(eaddr = virSocketAddrFormat(&ipdef->ranges[r].end))) goto cleanup; - virBufferAsprintf(&configbuf, "dhcp-range=%s,%s", - saddr, eaddr); - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) - virBufferAsprintf(&configbuf, ",%d", prefix); - virBufferAddLit(&configbuf, "\n"); + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) { + virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%d\n", + saddr, eaddr, prefix); + } else { + /* IPv4 - dnsmasq requires a netmask rather than prefix */ + virSocketAddr netmask; + char *netmaskStr; + + if (virSocketAddrPrefixToNetmask(prefix, &netmask, AF_INET) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to translate bridge '%s' " + "prefix %d to netmask"), + def->bridge, prefix); + goto cleanup; + } + + if (!(netmaskStr = virSocketAddrFormat(&netmask))) + goto cleanup; + virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%s\n", + saddr, eaddr, netmaskStr); + VIR_FREE(netmaskStr); + } VIR_FREE(saddr); VIR_FREE(eaddr); diff --git a/tests/networkxml2confdata/dhcp6-nat-network.conf b/tests/networkxml2confdata/dhcp6-nat-network.conf index d1058df3b6..536974e508 100644 --- a/tests/networkxml2confdata/dhcp6-nat-network.conf +++ b/tests/networkxml2confdata/dhcp6-nat-network.conf @@ -8,7 +8,7 @@ strict-order except-interface=lo bind-dynamic interface=virbr0 -dhcp-range=192.168.122.2,192.168.122.254 +dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0 dhcp-no-override dhcp-authoritative dhcp-range=2001:db8:ac10:fd01::1:10,2001:db8:ac10:fd01::1:ff,64 diff --git a/tests/networkxml2confdata/isolated-network.conf b/tests/networkxml2confdata/isolated-network.conf index ce4a59f6c1..693a83d9a0 100644 --- a/tests/networkxml2confdata/isolated-network.conf +++ b/tests/networkxml2confdata/isolated-network.conf @@ -10,7 +10,7 @@ bind-interfaces listen-address=192.168.152.1 dhcp-option=3 no-resolv -dhcp-range=192.168.152.2,192.168.152.254 +dhcp-range=192.168.152.2,192.168.152.254,255.255.255.0 dhcp-no-override dhcp-authoritative dhcp-lease-max=253 diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf b/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf index f35ea1d5d4..0b2ca6f5aa 100644 --- a/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf +++ b/tests/networkxml2confdata/nat-network-dns-srv-record-minimal.conf @@ -13,7 +13,7 @@ listen-address=fc00:db8:ac10:fe01::1 listen-address=fc00:db8:ac10:fd01::1 listen-address=10.24.10.1 srv-host=_name._tcp -dhcp-range=192.168.122.2,192.168.122.254 +dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0 dhcp-no-override dhcp-authoritative dhcp-lease-max=253 diff --git a/tests/networkxml2confdata/nat-network-dns-srv-record.conf b/tests/networkxml2confdata/nat-network-dns-srv-record.conf index af1ed70758..a18c09aaa7 100644 --- a/tests/networkxml2confdata/nat-network-dns-srv-record.conf +++ b/tests/networkxml2confdata/nat-network-dns-srv-record.conf @@ -15,7 +15,7 @@ srv-host=_name4._tcp.test4.com,test4.example.com,4444 srv-host=_name5._udp,test5.example.com,1,55,555 srv-host=_name6._tcp.test6.com,test6.example.com,6666,0,666 srv-host=_name7._tcp.test7.com,test7.example.com,1,0,777 -dhcp-range=192.168.122.2,192.168.122.254 +dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0 dhcp-no-override dhcp-authoritative dhcp-lease-max=253 diff --git a/tests/networkxml2confdata/nat-network-dns-txt-record.conf b/tests/networkxml2confdata/nat-network-dns-txt-record.conf index 7f560fbb5c..735c261c01 100644 --- a/tests/networkxml2confdata/nat-network-dns-txt-record.conf +++ b/tests/networkxml2confdata/nat-network-dns-txt-record.conf @@ -9,7 +9,7 @@ except-interface=lo bind-dynamic interface=virbr0 txt-record=example,example value -dhcp-range=192.168.122.2,192.168.122.254 +dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0 dhcp-no-override dhcp-authoritative dhcp-lease-max=253 diff --git a/tests/networkxml2confdata/nat-network-mtu.conf b/tests/networkxml2confdata/nat-network-mtu.conf index 91b574b964..1dd4754f2a 100644 --- a/tests/networkxml2confdata/nat-network-mtu.conf +++ b/tests/networkxml2confdata/nat-network-mtu.conf @@ -8,7 +8,7 @@ strict-order except-interface=lo bind-dynamic interface=virbr0 -dhcp-range=192.168.122.2,192.168.122.254 +dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0 dhcp-no-override dhcp-authoritative dhcp-lease-max=253 diff --git a/tests/networkxml2confdata/nat-network-name-with-quotes.conf b/tests/networkxml2confdata/nat-network-name-with-quotes.conf index 36e11d17b9..1b06de3066 100644 --- a/tests/networkxml2confdata/nat-network-name-with-quotes.conf +++ b/tests/networkxml2confdata/nat-network-name-with-quotes.conf @@ -13,7 +13,7 @@ listen-address=fc00:db8:ac10:fe01::1 listen-address=fc00:db8:ac10:fd01::1 listen-address=10.24.10.1 srv-host=_name._tcp -dhcp-range=192.168.122.2,192.168.122.254 +dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0 dhcp-no-override dhcp-authoritative dhcp-lease-max=253 diff --git a/tests/networkxml2confdata/nat-network.conf b/tests/networkxml2confdata/nat-network.conf index a3c8b102d3..873a360acc 100644 --- a/tests/networkxml2confdata/nat-network.conf +++ b/tests/networkxml2confdata/nat-network.conf @@ -8,7 +8,7 @@ strict-order except-interface=lo bind-dynamic interface=virbr0 -dhcp-range=192.168.122.2,192.168.122.254 +dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0 dhcp-no-override dhcp-authoritative dhcp-lease-max=253 diff --git a/tests/networkxml2confdata/netboot-network.conf b/tests/networkxml2confdata/netboot-network.conf index b554a5456c..99272b9d68 100644 --- a/tests/networkxml2confdata/netboot-network.conf +++ b/tests/networkxml2confdata/netboot-network.conf @@ -10,7 +10,7 @@ expand-hosts except-interface=lo bind-interfaces listen-address=192.168.122.1 -dhcp-range=192.168.122.2,192.168.122.254 +dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0 dhcp-no-override dhcp-authoritative enable-tftp diff --git a/tests/networkxml2confdata/netboot-proxy-network.conf b/tests/networkxml2confdata/netboot-proxy-network.conf index afb4033f7e..fb0a20cff4 100644 --- a/tests/networkxml2confdata/netboot-proxy-network.conf +++ b/tests/networkxml2confdata/netboot-proxy-network.conf @@ -10,7 +10,7 @@ expand-hosts except-interface=lo bind-interfaces listen-address=192.168.122.1 -dhcp-range=192.168.122.2,192.168.122.254 +dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0 dhcp-no-override dhcp-authoritative dhcp-boot=pxeboot.img,,10.20.30.40 diff --git a/tests/networkxml2confdata/ptr-domains-auto.conf b/tests/networkxml2confdata/ptr-domains-auto.conf index 7f1a393dd5..86701c4ddf 100644 --- a/tests/networkxml2confdata/ptr-domains-auto.conf +++ b/tests/networkxml2confdata/ptr-domains-auto.conf @@ -10,7 +10,7 @@ local=/1.0.e.f.0.1.c.a.8.b.d.0.1.0.0.2.ip6.arpa/ except-interface=lo bind-dynamic interface=virbr0 -dhcp-range=192.168.122.2,192.168.122.254 +dhcp-range=192.168.122.2,192.168.122.254,255.255.255.0 dhcp-no-override dhcp-authoritative dhcp-lease-max=253 -- 2.20.1
On 2/18/19 6:21 PM, Laine Stump wrote:
dnsmasq documentation says that the *IPv4* prefix/network address/broadcast address sent to dhcp clients will be automatically determined by dnsmasq by looking at the interface it's listening on, so the original libvirt code did not add a netmask to the dnsmasq commandline (or later, the dnsmasq conf file).
For *IPv6* however, dnsmasq apparently cannot automatically determine the prefix (functionally the same as a netmask), and it must be explicitly provided in the conf file (as a part of the dhcp-range option). So many years after IPv4 DHCP support had been added, when IPv6 dhcp support was added the prefix was included at the end of the dhcp-range setting, but only for IPv6.
Recently a user reported (privately, because they suspected a possible security implication, which turned out to be unfounded) a bug on a host where one of the interfaces was a superset of the libvirt network where dhcp is needed (e.g., the host's ethernet is 10.0.0.20/8, and the libvirt network is 10.10.0.1/24). For some reason dnsmasq was supplying the netmask for the /8 network to clients requesting an address on the /24 interface.
This seems like a bug in dnsmasq, but even if/when it gets fixed there, it looks like there is no harm in just always adding the netmask to all IPv4 dhcp-range options similar to how prefix is added to all IPv6 dhcp-range options.
Signed-off-by: Laine Stump <laine@laine.org> --- src/network/bridge_driver.c | 27 +++++++++++++++---- .../dhcp6-nat-network.conf | 2 +- .../networkxml2confdata/isolated-network.conf | 2 +- .../nat-network-dns-srv-record-minimal.conf | 2 +- .../nat-network-dns-srv-record.conf | 2 +- .../nat-network-dns-txt-record.conf | 2 +- .../networkxml2confdata/nat-network-mtu.conf | 2 +- .../nat-network-name-with-quotes.conf | 2 +- tests/networkxml2confdata/nat-network.conf | 2 +- .../networkxml2confdata/netboot-network.conf | 2 +- .../netboot-proxy-network.conf | 2 +- .../networkxml2confdata/ptr-domains-auto.conf | 2 +- 12 files changed, 33 insertions(+), 16 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 6d80818e40..9fa902896b 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1320,11 +1320,28 @@ networkDnsmasqConfContents(virNetworkObjPtr obj, !(eaddr = virSocketAddrFormat(&ipdef->ranges[r].end))) goto cleanup;
- virBufferAsprintf(&configbuf, "dhcp-range=%s,%s", - saddr, eaddr); - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) - virBufferAsprintf(&configbuf, ",%d", prefix); - virBufferAddLit(&configbuf, "\n"); + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) { + virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%d\n", + saddr, eaddr, prefix); + } else { + /* IPv4 - dnsmasq requires a netmask rather than prefix */ + virSocketAddr netmask;
Does it matter if this isn't initialized? e.g. "= { 0 };"
+ char *netmaskStr;
VIR_AUTOFREE(char *) netmaskStr = NULL;
+ + if (virSocketAddrPrefixToNetmask(prefix, &netmask, AF_INET) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to translate bridge '%s' " + "prefix %d to netmask"), + def->bridge, prefix); + goto cleanup; + } + + if (!(netmaskStr = virSocketAddrFormat(&netmask))) + goto cleanup; + virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%s\n", + saddr, eaddr, netmaskStr); + VIR_FREE(netmaskStr);
w/ VIR_AUTOFREE, this isn't necessary I know bridge_driver doesn't use it yet, but doesn't harm to start. Reviewed-by: John Ferlan <jferlan@redhat.com> John [...]
On 2/20/19 4:10 PM, John Ferlan wrote:
On 2/18/19 6:21 PM, Laine Stump wrote:
dnsmasq documentation says that the *IPv4* prefix/network address/broadcast address sent to dhcp clients will be automatically determined by dnsmasq by looking at the interface it's listening on, so the original libvirt code did not add a netmask to the dnsmasq commandline (or later, the dnsmasq conf file).
For *IPv6* however, dnsmasq apparently cannot automatically determine the prefix (functionally the same as a netmask), and it must be explicitly provided in the conf file (as a part of the dhcp-range option). So many years after IPv4 DHCP support had been added, when IPv6 dhcp support was added the prefix was included at the end of the dhcp-range setting, but only for IPv6.
Recently a user reported (privately, because they suspected a possible security implication, which turned out to be unfounded) a bug on a host where one of the interfaces was a superset of the libvirt network where dhcp is needed (e.g., the host's ethernet is 10.0.0.20/8, and the libvirt network is 10.10.0.1/24). For some reason dnsmasq was supplying the netmask for the /8 network to clients requesting an address on the /24 interface.
This seems like a bug in dnsmasq, but even if/when it gets fixed there, it looks like there is no harm in just always adding the netmask to all IPv4 dhcp-range options similar to how prefix is added to all IPv6 dhcp-range options.
Signed-off-by: Laine Stump <laine@laine.org> --- src/network/bridge_driver.c | 27 +++++++++++++++---- .../dhcp6-nat-network.conf | 2 +- .../networkxml2confdata/isolated-network.conf | 2 +- .../nat-network-dns-srv-record-minimal.conf | 2 +- .../nat-network-dns-srv-record.conf | 2 +- .../nat-network-dns-txt-record.conf | 2 +- .../networkxml2confdata/nat-network-mtu.conf | 2 +- .../nat-network-name-with-quotes.conf | 2 +- tests/networkxml2confdata/nat-network.conf | 2 +- .../networkxml2confdata/netboot-network.conf | 2 +- .../netboot-proxy-network.conf | 2 +- .../networkxml2confdata/ptr-domains-auto.conf | 2 +- 12 files changed, 33 insertions(+), 16 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 6d80818e40..9fa902896b 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -1320,11 +1320,28 @@ networkDnsmasqConfContents(virNetworkObjPtr obj, !(eaddr = virSocketAddrFormat(&ipdef->ranges[r].end))) goto cleanup;
- virBufferAsprintf(&configbuf, "dhcp-range=%s,%s", - saddr, eaddr); - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) - virBufferAsprintf(&configbuf, ",%d", prefix); - virBufferAddLit(&configbuf, "\n"); + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) { + virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%d\n", + saddr, eaddr, prefix); + } else { + /* IPv4 - dnsmasq requires a netmask rather than prefix */ + virSocketAddr netmask; Does it matter if this isn't initialized? e.g. "= { 0 };"
None of the other instances of virSocketAddr are initialized to 0, and the intent is that the utility functions should fill in all fields that are relevant. I'd rather not be the one responsible for starting a cargo cult of explicitly initializing them when it's not necessary :-)
+ char *netmaskStr; VIR_AUTOFREE(char *) netmaskStr = NULL;
+ + if (virSocketAddrPrefixToNetmask(prefix, &netmask, AF_INET) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to translate bridge '%s' " + "prefix %d to netmask"), + def->bridge, prefix); + goto cleanup; + } + + if (!(netmaskStr = virSocketAddrFormat(&netmask))) + goto cleanup; + virBufferAsprintf(&configbuf, "dhcp-range=%s,%s,%s\n", + saddr, eaddr, netmaskStr); + VIR_FREE(netmaskStr); w/ VIR_AUTOFREE, this isn't necessary
I know bridge_driver doesn't use it yet, but doesn't harm to start.
Yeah, I'll do that before I push.
Reviewed-by: John Ferlan <jferlan@redhat.com>
John
[...]
participants (2)
-
John Ferlan -
Laine Stump