9:10 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:12 a.m.
New subject: [libvirt] [PATCH] 1/3 Add IPv4 and IPv6 parsing routines
* 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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
3:55 p.m.
New subject: [libvirt] [PATCH] 1/3 Add IPv4 and IPv6 parsing routines
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4:49 a.m.
New subject: [libvirt] [PATCH] 1/3 Add IPv4 and IPv6 parsing routines
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 :|
5:51 a.m.
New subject: [libvirt] [PATCH] 1/3 Add IPv4 and IPv6 parsing routines
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
6:28 a.m.
New subject: [libvirt] [PATCH] 1/3 Add IPv4 and IPv6 parsing routines
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 :|
9:14 a.m.
New subject: [libvirt] [PATCH] 2/3 Store the range size when adding a DHCP range
* 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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4:57 a.m.
New subject: [libvirt] [PATCH] 2/3 Store the range size when adding a DHCP range
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 :|
5:59 a.m.
New subject: [libvirt] [PATCH] 2/3 Store the range size when adding a DHCP range
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
6:33 a.m.
New subject: [libvirt] [PATCH] 2/3 Store the range size when adding a DHCP range
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 :|
7:31 a.m.
New subject: [libvirt] [PATCH] 2/3 Store the range size when adding a DHCP range
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
8:07 a.m.
New subject: [libvirt] [PATCH] 2/3 Store the range size when adding a DHCP range
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 :|
8:13 a.m.
New subject: [libvirt] [PATCH] 2/3 Store the range size when adding a DHCP range
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:14 a.m.
New subject: [libvirt] [PATCH] 3/3 524280 pass max lease option to dnsmasq
* 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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4:58 a.m.
New subject: [libvirt] [PATCH] 3/3 524280 pass max lease option to dnsmasq
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 :|
11:11 a.m.
New subject: [libvirt] [PATCH] 3/3 524280 pass max lease option to dnsmasq
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:24 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
5496
days inactive
5520
days old
16 comments
2 participants
participants (2)
-
Daniel P. Berrange -
Daniel Veillard