[libvirt] 3 patches to solve bug 524280
Basically dnsmasq limits itself by default to 150 leases maximum. Which means we may inflict an arbitrary limit in the number of domains running on a given host https://bugzilla.redhat.com/show_bug.cgi?id=524280 The patches do 3 things: - first introduce IPv4 and IPv6 parsing routines, generally useful this could be used to double check input in other places too - then when a DHCP range is defined use those routines to parse the start and end, do some cheking and keep the range size in the structure - last when exec'ing dnsmasq compute the sum of all dhcp ranges and pass the max lease option Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
* src/util/util.h src/util/util.c: two new functions virParseIPv4 and virParseIPv6 diff --git a/src/util/util.c b/src/util/util.c index e5135fc..bbd29dc 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1761,6 +1761,161 @@ void virGenerateMacAddr(const unsigned char *prefix, addr[5] = virRandom(256); } +static int virParseIPv4Byte(const char **addr) { + unsigned int v; + const char *cur = *addr; + + /* extend to accept hexa values ? */ + if (cur == NULL) + return(-1); + if ((*cur < '0') || (*cur > '9')) + return(-1); + v = *cur++ - '0'; + if ((*cur >= '0') && (*cur <= '9')) + v = 10 * v + *cur++ - '0'; + if ((*cur >= '0') && (*cur <= '9')) + v = 10 * v + *cur++ - '0'; + if (v > 255) + return(-1); + *addr = cur; + return(v); +} + +/* + * virParseIPv4: + * @addr: pointer to the address string in classic dotted format and zero + * terminated + * @res: optional pointer to the place to store the result + * + * Parse the IPv4 address in classic dot-decimal notation format like + * 192.0.2.235, and store the output if res is provided: + * for example res[0] = 192, res[1] = 0, res[2] = 2, res[3] = 235 + * + * Retuns 0 in case of success and -1 in case of error + */ +int virParseIPv4(const char *addr, virIPv4Val *res) { + int v; + const char *cur = addr; + + if (addr == NULL) + return(-1); + + if (res) + memset(res, 0, sizeof(*res)); + + v = virParseIPv4Byte(&cur); + if ((v < 0) || (*cur != '.')) + return(-1); + if (res) (*res)[0] = (unsigned char) v; + cur++; + + v = virParseIPv4Byte(&cur); + if ((v < 0) || (*cur != '.')) + return(-1); + if (res) (*res)[1] = (unsigned char) v; + cur++; + + v = virParseIPv4Byte(&cur); + if ((v < 0) || (*cur != '.')) + return(-1); + if (res) (*res)[2] = (unsigned char) v; + cur++; + + v = virParseIPv4Byte(&cur); + if ((v < 0) || (*cur != 0)) + return(-1); + if (res) (*res)[3] = (unsigned char) v; + + return(0); +} + +static int virParseIPv6Short(const char **addr) { + const char *cur = *addr; + char *end_ptr; + unsigned long result; + + if (cur == NULL) + return(-1); + errno = 0; + + /* This is solely to avoid accepting the leading + * space or "+" that strtoul would otherwise accept. + */ + if (!c_isxdigit(*cur)) + return(-1); + + result = strtoul(cur, &end_ptr, 16); + + if (((end_ptr - cur) < 1) || ((end_ptr - cur) > 4) || + (errno != 0) || (result > 0xFFFF)) + return(-1); + + *addr = end_ptr; + return((int) result); +} + +static int virCountColumn(const char *cur) { + int ret; + + for (ret = 0;*cur != 0;cur++) + if (*cur == ':') ret++; + return(ret); +} + +/* + * virParseIPv6: + * @addr: pointer to the address string in classic hex and column format + * and zero terminated + * @res: optional pointer to the place to store the result + * + * Parse the IPv6 address in classic dot-decimal notation format like + * 2001:db8:85a3:0:0:8a2e:370:7334, and store the output if res is provided: + * for example res[0] = 0x2001, res[1] = 0xdb8, etc. + * TODO: doesn't support the :: compression notation yet + * + * Retuns 0 in case of success and -1 in case of error + */ +int virParseIPv6(const char *addr, virIPv6Val *res) { + int v, i, left, compact; + const char *cur = addr; + + if (addr == NULL) + return(-1); + + if (res) + memset(res, 0, sizeof(*res)); + + compact = 0; + for (i = 0;i < 8;i++) { + if ((*cur == ':') && (compact == 0)) { + compact = 1; + cur++; + if (i == 0) { + /* if leading we expect :: */ + if (*cur != ':') + return(-1); + cur++; + if (*cur == 0) + return(0); + } + left = virCountColumn(cur); + if (7 - left <= i) + return(-1); + i = 7 - left; + } + v = virParseIPv6Short(&cur); + if (v < 0) + return(-1); + if (((i < 7) && (*cur != ':')) || + ((i == 7) && (*cur != 0))) + return(-1); + if (res) + (*res)[i] = (unsigned short) v; + cur++; + } + + return(0); +} int virEnumFromString(const char *const*types, unsigned int ntypes, diff --git a/src/util/util.h b/src/util/util.h index 8679636..b1be46f 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -185,6 +185,12 @@ void virFormatMacAddr(const unsigned char *addr, void virGenerateMacAddr(const unsigned char *prefix, unsigned char *addr); +typedef unsigned char virIPv4Val[4]; +typedef unsigned short virIPv6Val[8]; + +int virParseIPv4(const char *addr, virIPv4Val *res); +int virParseIPv6(const char *addr, virIPv6Val *res); + int virDiskNameToIndex(const char* str); -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, Oct 13, 2009 at 04:12:47PM +0200, Daniel Veillard wrote:
+/* + * virParseIPv6: + * @addr: pointer to the address string in classic hex and column format + * and zero terminated + * @res: optional pointer to the place to store the result + * + * Parse the IPv6 address in classic dot-decimal notation format like + * 2001:db8:85a3:0:0:8a2e:370:7334, and store the output if res is provided: + * for example res[0] = 0x2001, res[1] = 0xdb8, etc. + * TODO: doesn't support the :: compression notation yet
Oops old comment, actually it does !
+ * Retuns 0 in case of success and -1 in case of error + */
Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, Oct 13, 2009 at 04:12:47PM +0200, Daniel Veillard wrote:
* src/util/util.h src/util/util.c: two new functions virParseIPv4 and virParseIPv6
I think this should just be a thin wrapper around getaddrinfo() which already knows how to parse all possible address types. If we avoid a custom typedef, and just use 'struct sockaddr_storage' this in turn makes it easy for callers to pass the result straight into any socket API calls they might use. eg this could all be done with a couple of lines of code int virSocketAddr(const char *str, struct sockaddr_storage *ss) { int len; struct addrinfo hints = { 0 }; struct addrinfo *res = NULL; hints.ai_flags = AI_NUMERICHOST; if (getaddrinfo(str, NULL, &hints, &res) != 0 || !res) return -1; len = res->addrlen; memcpy(ss, res->ai_addr, len); freeaddrinfo(res); return len; } That would automatically cope with both IPv4 / 6 addresses. If we want to restrict it we could add a 3rd argument, 'int family' and use that to set hints.ai_family field - the caller would just pass AF_INET or AF_INET6 to specify a particular type, or leave it at 0 to allow any type. It is probably worth introducing a 'src/util/socketaddr.h' file for this, since I think we'll need more socket addr functions later Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Wed, Oct 14, 2009 at 10:49:20AM +0100, Daniel P. Berrange wrote:
On Tue, Oct 13, 2009 at 04:12:47PM +0200, Daniel Veillard wrote:
* src/util/util.h src/util/util.c: two new functions virParseIPv4 and virParseIPv6
I think this should just be a thin wrapper around getaddrinfo() which already knows how to parse all possible address types.
Are we allowing all possible address types in the XML ? I based that parsing routing in a large part as a syntactic check on the allowed set of addresses. For example I didn't allow ::ffff:12.34.56.78 kind of IPv6 syntax.
If we avoid a custom typedef, and just use 'struct sockaddr_storage' this in turn makes it easy for callers to pass the result straight into any socket API calls they might use. eg this could all be done with a couple of lines of code
int virSocketAddr(const char *str, struct sockaddr_storage *ss) {
The point of the exercise was to make some checks on the addresses given as string and passed down as string to an external tool
int len; struct addrinfo hints = { 0 }; struct addrinfo *res = NULL;
hints.ai_flags = AI_NUMERICHOST;
if (getaddrinfo(str, NULL, &hints, &res) != 0 || !res) return -1;
len = res->addrlen; memcpy(ss, res->ai_addr, len);
freeaddrinfo(res); return len; }
That would automatically cope with both IPv4 / 6 addresses. If we want to restrict it we could add a 3rd argument, 'int family' and use that to set hints.ai_family field - the caller would just pass AF_INET or AF_INET6 to specify a particular type, or leave it at 0 to allow any type.
Dunno, I find the getaddrinfo interface and the hints stuff fairly incomprehensible. When I see the OpenGroup definition of struct sockaddr_storage struct sockaddr_storage { sa_family_t ss_family; /* Address family. */ /* * Following fields are implementation-defined. */ char _ss_pad1[_SS_PAD1SIZE]; /* 6-byte pad; this is to make implementation-defined pad up to alignment field that follows explicit in the data structure. */ int64_t _ss_align; /* Field to force desired structure storage alignment. */ char _ss_pad2[_SS_PAD2SIZE]; /* 112-byte pad to achieve desired size, _SS_MAXSIZE value minus size of ss_family __ss_pad1, __ss_align fields is 112. */ }; I'm not too enthusiastic about using this for internal APIs. And I don't see how I would check ranges for the IP addresses based on this. Actually I don't find it helps to calculate a range, and I prefer my good old arrays of well defined ints for that purpose.
It is probably worth introducing a 'src/util/socketaddr.h' file for this, since I think we'll need more socket addr functions later
No idea you had that in mind, go for it ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Wed, Oct 14, 2009 at 12:51:32PM +0200, Daniel Veillard wrote:
On Wed, Oct 14, 2009 at 10:49:20AM +0100, Daniel P. Berrange wrote:
On Tue, Oct 13, 2009 at 04:12:47PM +0200, Daniel Veillard wrote:
* src/util/util.h src/util/util.c: two new functions virParseIPv4 and virParseIPv6
I think this should just be a thin wrapper around getaddrinfo() which already knows how to parse all possible address types.
Are we allowing all possible address types in the XML ?
Well at this time we only allow IPv4 addreses in this, so we would want to pass the AF_INET flag to restrict it. Other areas of libvirt which could uses this code would be more flexible. We've got an open RFE for IPv6 support https://bugzilla.redhat.com/show_bug.cgi?id=514749
I based that parsing routing in a large part as a syntactic check on the allowed set of addresses. For example I didn't allow
::ffff:12.34.56.78
kind of IPv6 syntax.
Why shouldn't we allow that - its valid IPv6 address syntax }
If we avoid a custom typedef, and just use 'struct sockaddr_storage' this in turn makes it easy for callers to pass the result straight into any socket API calls they might use. eg this could all be done with a couple of lines of code
That would automatically cope with both IPv4 / 6 addresses. If we want to restrict it we could add a 3rd argument, 'int family' and use that to set hints.ai_family field - the caller would just pass AF_INET or AF_INET6 to specify a particular type, or leave it at 0 to allow any type.
Dunno, I find the getaddrinfo interface and the hints stuff fairly incomprehensible. When I see the OpenGroup definition of struct sockaddr_storage
struct sockaddr_storage { sa_family_t ss_family; /* Address family. */ /* * Following fields are implementation-defined. */ char _ss_pad1[_SS_PAD1SIZE]; /* 6-byte pad; this is to make implementation-defined pad up to alignment field that follows explicit in the data structure. */ int64_t _ss_align; /* Field to force desired structure storage alignment. */ char _ss_pad2[_SS_PAD2SIZE]; /* 112-byte pad to achieve desired size, _SS_MAXSIZE value minus size of ss_family __ss_pad1, __ss_align fields is 112. */ };
NB it is not intended to use the internals of the sockaddr_storage struct, with the exception of the ss_family field. It is provided as a struct that is guarenteed large enough to store any type of address. To use the data, you'd cast to the appropriate struct based on ss_family. eg, if ss_family == AF_INET, then you can cast to struct sockaddr_in which has a 'struct in_addr sin_addr' field available to get the raw address - in this case 'in_addr' is just an int32 For AF_INET6, you would cast to sockaddr_in6, which has a 'struct in6_addr sin6_addr' which gives you easy acess to the 16 byte array of the address.
I'm not too enthusiastic about using this for internal APIs. And I don't see how I would check ranges for the IP addresses based on this. Actually I don't find it helps to calculate a range, and I prefer my good old arrays of well defined ints for that purpose.
The sockaddr_in/in6 structs both give you an array of bytes in exactly the same manner Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
* src/conf/network_conf.h: extend the structure to store the range * src/conf/network_conf.c: before adding a range parse the IP addresses do some checking and keep the size diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 14eb543..992e02a 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -217,6 +217,90 @@ void virNetworkRemoveInactive(virNetworkObjListPtr nets, } } +static int +virNetworkDHCPCheckRange(virConnectPtr conn, + const char *start, const char *end) { + virIPv4Val ip4s, ip4e; + virIPv6Val ip6s, ip6e; + int ret; + int i; + + if (virParseIPv4(start, &ip4s) == 0) { + if (virParseIPv4(end, &ip4e) < 0) { + virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot parse IPv4 address '%s'"), + end); + return(-1); + } + + /* + * check at least the 2 first IP match i.e on same class C subnet + */ + for (i = 0; i < 2;i++) { + if (ip4s[i] != ip4e[i]) { + virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("start and end of DHCP range do not match '%s' and '%s'"), + start, end); + return(-1); + } + } + ret = ip4e[3] - ip4s[3] + 256 * (ip4e[2] - ip4s[2]); + + /* + * a bit of sanity checking on the range + * Should we complain for a range of more than 10,000 ? + */ + if (ret < 0) { + virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("start and end of DHCP range swapped '%s' and '%s'"), + start, end); + return(-1); + } + + /* include the boundaries */ + ret++; + } else if (virParseIPv6(start, &ip6s) == 0) { + if (virParseIPv6(end, &ip6e) < 0) { + virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot parse IPv6 address '%s'"), + end); + return(-1); + } + + /* + * check the prefix up to the last group are identical + */ + for (i = 0; i < 7;i++) { + if (ip6s[i] != ip6e[i]) { + virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("start and end of DHCP range do not match '%s' and '%s'"), + start, end); + return(-1); + } + } + ret = ip6e[7] - ip6s[7]; + + /* + * a bit of sanity checking on the range + * Should we complain for a range of more than 10,000 ? + */ + if (ret < 0) { + virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("start and end of DHCP range swapped '%s' and '%s'"), + start, end); + return(-1); + } + + /* include the boundaries */ + ret++; + } else { + virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot parse IP address '%s'"), + start); + return(-1); + } + return(ret); +} static int virNetworkDHCPRangeDefParseXML(virConnectPtr conn, @@ -230,6 +314,7 @@ virNetworkDHCPRangeDefParseXML(virConnectPtr conn, if (cur->type == XML_ELEMENT_NODE && xmlStrEqual(cur->name, BAD_CAST "range")) { xmlChar *start, *end; + int range; if (!(start = xmlGetProp(cur, BAD_CAST "start"))) { cur = cur->next; @@ -240,6 +325,13 @@ virNetworkDHCPRangeDefParseXML(virConnectPtr conn, xmlFree(start); continue; } + range = virNetworkDHCPCheckRange(conn, (const char *)start, + (const char *)end); + if (range < 0) { + xmlFree(start); + xmlFree(end); + continue; + } if (VIR_REALLOC_N(def->ranges, def->nranges + 1) < 0) { xmlFree(start); @@ -249,6 +341,7 @@ virNetworkDHCPRangeDefParseXML(virConnectPtr conn, } def->ranges[def->nranges].start = (char *)start; def->ranges[def->nranges].end = (char *)end; + def->ranges[def->nranges].size = range; def->nranges++; } else if (cur->type == XML_ELEMENT_NODE && xmlStrEqual(cur->name, BAD_CAST "host")) { diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index e983a01..2960e8b 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -45,6 +45,7 @@ typedef virNetworkDHCPRangeDef *virNetworkDHCPRangeDefPtr; struct _virNetworkDHCPRangeDef { char *start; char *end; + int size; }; typedef struct _virNetworkDHCPHostDef virNetworkDHCPHostDef; Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, Oct 13, 2009 at 04:14:00PM +0200, Daniel Veillard wrote:
+ + /* + * check at least the 2 first IP match i.e on same class C subnet + */ + for (i = 0; i < 2;i++) { + if (ip4s[i] != ip4e[i]) { + virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("start and end of DHCP range do not match '%s' and '%s'"), + start, end); + return(-1); + } + }
Shouldn't we be comparing each of DHCP addresses against the 'netmask' field we have in virNetworkDef instead. It'd be nice to have a separate function for this like virSocketAddrInNetwork(struct sockaddr_storage *address, struct sockaddr_storage *netmask); since there's a couple of other places we ought todo this kind of validation.
+ ret = ip4e[3] - ip4s[3] + 256 * (ip4e[2] - ip4s[2]);
It would be nice to have this in a callable function too int virSocketAddrRange(struct sockaddr_storage *start, struct sockaddr_storage *end);
+ + /* + * a bit of sanity checking on the range + * Should we complain for a range of more than 10,000 ? + */
Its probably sufficient to leave dnsmasq to do validation.
+ if (ret < 0) { + virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("start and end of DHCP range swapped '%s' and '%s'"), + start, end); + return(-1); + } + + /* include the boundaries */ + ret++;
diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h index e983a01..2960e8b 100644 --- a/src/conf/network_conf.h +++ b/src/conf/network_conf.h @@ -45,6 +45,7 @@ typedef virNetworkDHCPRangeDef *virNetworkDHCPRangeDefPtr; struct _virNetworkDHCPRangeDef { char *start; char *end; + int size; };
Regards,x Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Wed, Oct 14, 2009 at 10:57:43AM +0100, Daniel P. Berrange wrote:
On Tue, Oct 13, 2009 at 04:14:00PM +0200, Daniel Veillard wrote:
+ + /* + * check at least the 2 first IP match i.e on same class C subnet + */ + for (i = 0; i < 2;i++) { + if (ip4s[i] != ip4e[i]) { + virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("start and end of DHCP range do not match '%s' and '%s'"), + start, end); + return(-1); + } + }
Shouldn't we be comparing each of DHCP addresses against the 'netmask' field we have in virNetworkDef instead. It'd be nice to have a separate function for this like
As far as I can tell the netmask is optional. but if it's there, sure.
virSocketAddrInNetwork(struct sockaddr_storage *address, struct sockaddr_storage *netmask);
Hum, I don't understand what that function would do suppose you have 1.2.3.4 and 255.255.255.0 what kind of thing can you do. Sure if you pass 2 addresses then you can check they pertain to the same netmask but that function signature can't work IMHO
since there's a couple of other places we ought todo this kind of validation.
+ ret = ip4e[3] - ip4s[3] + 256 * (ip4e[2] - ip4s[2]);
It would be nice to have this in a callable function too
int virSocketAddrRange(struct sockaddr_storage *start, struct sockaddr_storage *end);
Are you supposed to look struct sockaddr_storage ? As posted in my last mail this seems a completely opaque structure at least in theory and if you want to keep the portability it's supposed to bring.
+ + /* + * a bit of sanity checking on the range + * Should we complain for a range of more than 10,000 ? + */
Its probably sufficient to leave dnsmasq to do validation.
on the second point, yes, for start/end order it's probably easier to capture and report immediately. the Network parsing code is lax, it reports errors but doesn't stop processing (except for allocation failure) the definition, I assume the goal is to always have a dnsmasq running even if some args are missing, hence it's better to make as much checking before calling. If my assumption is incorrect then that code should be made strict and return -1 on error instead. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Wed, Oct 14, 2009 at 12:59:53PM +0200, Daniel Veillard wrote:
On Wed, Oct 14, 2009 at 10:57:43AM +0100, Daniel P. Berrange wrote:
On Tue, Oct 13, 2009 at 04:14:00PM +0200, Daniel Veillard wrote:
+ + /* + * check at least the 2 first IP match i.e on same class C subnet + */ + for (i = 0; i < 2;i++) { + if (ip4s[i] != ip4e[i]) { + virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("start and end of DHCP range do not match '%s' and '%s'"), + start, end); + return(-1); + } + }
Shouldn't we be comparing each of DHCP addresses against the 'netmask' field we have in virNetworkDef instead. It'd be nice to have a separate function for this like
As far as I can tell the netmask is optional. but if it's there, sure.
Netmask is actually compulsory, although it shouldn't be since there are well defined defaults for both IPv4 & 6 :-)
virSocketAddrInNetwork(struct sockaddr_storage *address, struct sockaddr_storage *netmask);
Hum, I don't understand what that function would do suppose you have 1.2.3.4 and 255.255.255.0 what kind of thing can you do. Sure if you pass 2 addresses then you can check they pertain to the same netmask but that function signature can't work IMHO
Opps, my mistake this should have had 3 args virSocketAddrInNetwork(struct sockaddr_storage *address, struct sockaddr_storage *netaddr, struct sockaddr_storage *netmask); The idea is to use 'netmask' to mask off the low bits in 'netaddr', and then check the the remaining bits in 'netaddr' match those in the requested 'address'. We already have some horrible code in virNetworkDefParseXML() which can do the netaddr+netmask bit masking for IPv4, to calculate a network address.
since there's a couple of other places we ought todo this kind of validation.
+ ret = ip4e[3] - ip4s[3] + 256 * (ip4e[2] - ip4s[2]);
It would be nice to have this in a callable function too
int virSocketAddrRange(struct sockaddr_storage *start, struct sockaddr_storage *end);
Are you supposed to look struct sockaddr_storage ? As posted in my last mail this seems a completely opaque structure at least in theory and if you want to keep the portability it's supposed to bring.
You cast to one of the address specific structs according to the ss_family field. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Wed, Oct 14, 2009 at 12:33:36PM +0100, Daniel P. Berrange wrote:
On Wed, Oct 14, 2009 at 12:59:53PM +0200, Daniel Veillard wrote:
On Wed, Oct 14, 2009 at 10:57:43AM +0100, Daniel P. Berrange wrote:
On Tue, Oct 13, 2009 at 04:14:00PM +0200, Daniel Veillard wrote:
+ + /* + * check at least the 2 first IP match i.e on same class C subnet + */ + for (i = 0; i < 2;i++) { + if (ip4s[i] != ip4e[i]) { + virNetworkReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("start and end of DHCP range do not match '%s' and '%s'"), + start, end); + return(-1); + } + }
Shouldn't we be comparing each of DHCP addresses against the 'netmask' field we have in virNetworkDef instead. It'd be nice to have a separate function for this like
As far as I can tell the netmask is optional. but if it's there, sure.
Netmask is actually compulsory, although it shouldn't be since there are well defined defaults for both IPv4 & 6 :-)
Then we need to fix the schemas ! docs/schemas/network.rng: <optional> <attribute name="netmask"><text/></attribute> </optional>
virSocketAddrInNetwork(struct sockaddr_storage *address, struct sockaddr_storage *netmask);
Hum, I don't understand what that function would do suppose you have 1.2.3.4 and 255.255.255.0 what kind of thing can you do. Sure if you pass 2 addresses then you can check they pertain to the same netmask but that function signature can't work IMHO
Opps, my mistake this should have had 3 args
virSocketAddrInNetwork(struct sockaddr_storage *address, struct sockaddr_storage *netaddr, struct sockaddr_storage *netmask);
Ah, okay, that makes sense.
We already have some horrible code in virNetworkDefParseXML() which can do the netaddr+netmask bit masking for IPv4, to calculate a network address.
if I have an array it's trivial. But the struct sockaddr_storage opaque is weird.
since there's a couple of other places we ought todo this kind of validation.
+ ret = ip4e[3] - ip4s[3] + 256 * (ip4e[2] - ip4s[2]);
It would be nice to have this in a callable function too
int virSocketAddrRange(struct sockaddr_storage *start, struct sockaddr_storage *end);
Are you supposed to look struct sockaddr_storage ? As posted in my last mail this seems a completely opaque structure at least in theory and if you want to keep the portability it's supposed to bring.
You cast to one of the address specific structs according to the ss_family field.
humpf ... okay it has to be cast to be accessed, that's weird, definitely. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Wed, Oct 14, 2009 at 02:31:29PM +0200, Daniel Veillard wrote:
On Wed, Oct 14, 2009 at 12:33:36PM +0100, Daniel P. Berrange wrote:
It would be nice to have this in a callable function too
int virSocketAddrRange(struct sockaddr_storage *start, struct sockaddr_storage *end);
Are you supposed to look struct sockaddr_storage ? As posted in my last mail this seems a completely opaque structure at least in theory and if you want to keep the portability it's supposed to bring.
You cast to one of the address specific structs according to the ss_family field.
humpf ... okay it has to be cast to be accessed, that's weird, definitely.
Yeah, ideally it would have been in a union, but POSIX the way it was defined when sockets API were first designed, didn't allow a union to be retrofitted. We could consider a union ourselves though for our API if it might make it a little clearer, eg typedef union { struct sockaddr_storage stor; struct sockaddr_in inet4; struct sockaddr_in6 inet6; } virSocketAddr; to allow more direct access without the casting Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Wed, Oct 14, 2009 at 02:07:15PM +0100, Daniel P. Berrange wrote:
On Wed, Oct 14, 2009 at 02:31:29PM +0200, Daniel Veillard wrote:
On Wed, Oct 14, 2009 at 12:33:36PM +0100, Daniel P. Berrange wrote:
It would be nice to have this in a callable function too
int virSocketAddrRange(struct sockaddr_storage *start, struct sockaddr_storage *end);
Are you supposed to look struct sockaddr_storage ? As posted in my last mail this seems a completely opaque structure at least in theory and if you want to keep the portability it's supposed to bring.
You cast to one of the address specific structs according to the ss_family field.
humpf ... okay it has to be cast to be accessed, that's weird, definitely.
Yeah, ideally it would have been in a union, but POSIX the way it was defined when sockets API were first designed, didn't allow a union to be retrofitted.
We could consider a union ourselves though for our API if it might make it a little clearer, eg
typedef union { struct sockaddr_storage stor; struct sockaddr_in inet4; struct sockaddr_in6 inet6; } virSocketAddr;
to allow more direct access without the casting
Ah, yes, that look way better that way, and use that for all our internal APIs. Fine then, I will look at this, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
* src/network/bridge_driver.c: when exec'ing dnsmaq, if there are DHCP ranges defined, then compute and pass the --dhcp-lease-max deriving the maximum number of leases diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 95bc810..bffb6f7 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -365,6 +365,7 @@ networkBuildDnsmasqArgv(virConnectPtr conn, const char *pidfile, const char ***argv) { int i, len, r; + int nbleases = 0; char *pidfileArg; char buf[1024]; @@ -398,6 +399,8 @@ networkBuildDnsmasqArgv(virConnectPtr conn, 2 + /* --except-interface lo */ 2 + /* --listen-address 10.0.0.1 */ (2 * network->def->nranges) + /* --dhcp-range 10.0.0.2,10.0.0.254 */ + /* --dhcp-lease-max=xxx if needed */ + (network->def->nranges ? 0 : 1) + /* --dhcp-host 01:23:45:67:89:0a,hostname,10.0.0.3 */ (2 * network->def->nhosts) + /* --enable-tftp --tftp-root /srv/tftp */ @@ -462,6 +465,12 @@ networkBuildDnsmasqArgv(virConnectPtr conn, APPEND_ARG(*argv, i++, "--dhcp-range"); APPEND_ARG(*argv, i++, buf); + nbleases += network->def->ranges[r].size; + } + + if (network->def->nranges > 0) { + snprintf(buf, sizeof(buf), "--dhcp-lease-max=%d", nbleases); + APPEND_ARG(*argv, i++, buf); } for (r = 0 ; r < network->def->nhosts ; r++) { Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, Oct 13, 2009 at 04:14:59PM +0200, Daniel Veillard wrote:
* src/network/bridge_driver.c: when exec'ing dnsmaq, if there are DHCP ranges defined, then compute and pass the --dhcp-lease-max deriving the maximum number of leases
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 95bc810..bffb6f7 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -365,6 +365,7 @@ networkBuildDnsmasqArgv(virConnectPtr conn, const char *pidfile, const char ***argv) { int i, len, r; + int nbleases = 0; char *pidfileArg; char buf[1024];
@@ -398,6 +399,8 @@ networkBuildDnsmasqArgv(virConnectPtr conn, 2 + /* --except-interface lo */ 2 + /* --listen-address 10.0.0.1 */ (2 * network->def->nranges) + /* --dhcp-range 10.0.0.2,10.0.0.254 */ + /* --dhcp-lease-max=xxx if needed */ + (network->def->nranges ? 0 : 1) + /* --dhcp-host 01:23:45:67:89:0a,hostname,10.0.0.3 */ (2 * network->def->nhosts) + /* --enable-tftp --tftp-root /srv/tftp */ @@ -462,6 +465,12 @@ networkBuildDnsmasqArgv(virConnectPtr conn,
APPEND_ARG(*argv, i++, "--dhcp-range"); APPEND_ARG(*argv, i++, buf); + nbleases += network->def->ranges[r].size; + } + + if (network->def->nranges > 0) { + snprintf(buf, sizeof(buf), "--dhcp-lease-max=%d", nbleases); + APPEND_ARG(*argv, i++, buf); }
for (r = 0 ; r < network->def->nhosts ; r++) {
ACK, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Wed, Oct 14, 2009 at 10:58:24AM +0100, Daniel P. Berrange wrote:
On Tue, Oct 13, 2009 at 04:14:59PM +0200, Daniel Veillard wrote:
* src/network/bridge_driver.c: when exec'ing dnsmaq, if there are DHCP ranges defined, then compute and pass the --dhcp-lease-max deriving the maximum number of leases
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 95bc810..bffb6f7 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -365,6 +365,7 @@ networkBuildDnsmasqArgv(virConnectPtr conn, const char *pidfile, const char ***argv) { int i, len, r; + int nbleases = 0; char *pidfileArg; char buf[1024];
@@ -398,6 +399,8 @@ networkBuildDnsmasqArgv(virConnectPtr conn, 2 + /* --except-interface lo */ 2 + /* --listen-address 10.0.0.1 */ (2 * network->def->nranges) + /* --dhcp-range 10.0.0.2,10.0.0.254 */ + /* --dhcp-lease-max=xxx if needed */ + (network->def->nranges ? 0 : 1) + /* --dhcp-host 01:23:45:67:89:0a,hostname,10.0.0.3 */ (2 * network->def->nhosts) + /* --enable-tftp --tftp-root /srv/tftp */ @@ -462,6 +465,12 @@ networkBuildDnsmasqArgv(virConnectPtr conn,
APPEND_ARG(*argv, i++, "--dhcp-range"); APPEND_ARG(*argv, i++, buf); + nbleases += network->def->ranges[r].size; + } + + if (network->def->nranges > 0) { + snprintf(buf, sizeof(buf), "--dhcp-lease-max=%d", nbleases); + APPEND_ARG(*argv, i++, buf); }
for (r = 0 ; r < network->def->nhosts ; r++) {
ACK,
Daniel
Okay, applied after having rebased the previous patch and fixed an array indexing problem in getIPv4Addr() after the ntoh conversion. This fixes 524280 and with the usual default network we have /usr/sbin/dnsmasq --strict-order --bind-interfaces --pid-file=/var/run/libvirt/network/default.pid --conf-file= --listen-address 192.168.122.1 --except-interface lo --dhcp-range 192.168.122.2,192.168.122.254 --dhcp-lease-max=253 running, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, Oct 13, 2009 at 04:10:04PM +0200, Daniel Veillard wrote:
Basically dnsmasq limits itself by default to 150 leases maximum. Which means we may inflict an arbitrary limit in the number of domains running on a given host https://bugzilla.redhat.com/show_bug.cgi?id=524280
The patches do 3 things: - first introduce IPv4 and IPv6 parsing routines, generally useful this could be used to double check input in other places too - then when a DHCP range is defined use those routines to parse the start and end, do some cheking and keep the range size in the structure - last when exec'ing dnsmasq compute the sum of all dhcp ranges and pass the max lease option
Forgot to add that with the 3 patches applied, if restarting the default network you get the following running: /usr/sbin/dnsmasq --strict-order --bind-interfaces --pid-file=/var/run/libvirt/network/default.pid --conf-file= --listen-address 192.168.122.1 --except-interface lo --dhcp-range 192.168.122.2,192.168.122.254 --dhcp-lease-max=253 I didn't tried on an IPv6, actually I'm not sure dnsmasq supports IPv6 but at least the libvirt side is then ready. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (2)
-
Daniel P. Berrange -
Daniel Veillard