2:31 a.m.
On 12/22/2010 07:16 PM, Eric Blake wrote:
On 12/22/2010 11:58 AM, Laine Stump wrote:
> This commit adds support for IPv6 parsing and formatting to the
> virtual network XML parser, including moving around data definitions
> to allow for multiple<ip> elements on a single network, but only
> changes the consumers of this API to accomodate for the changes in
s/accomodate/accommodate/
Done.
> API/structure, not to add any actual IPv6 functionality. That
will
> come in a later patch - this patch attempts to maintain the same final
> functionality in both drivers that use the network XML parser - vbox
> and "bridge" (the Linux bridge-based driver used by the qemu
> hypervisor driver).
>
> * src/libvirt_private.syms: Add new private API functions.
> * src/conf/network_conf.[ch]: Change C data structure and
> parsing/formatting.
> * src/network/bridge_driver.c: Update to use new parser/formatter.
> * src/vbox/vbox_tmpl.c: update to use new parser/formatter
> * docs/schemas/network.rng: changes to the schema -
> * there can now be more than one<ip> element.
> * ip address is now an ip-addr (ipv4 or ipv6) rather than ipv4-addr
> * new optional "prefix" attribute that can be used in place of
"netmask"
> * new optional "family" attribute - "ipv4" or
"ipv6"
> (will default to ipv4)
> * define data types for the above
> * tests/networkxml2xml(in|out)/nat-network.xml: add multiple<ip> elements
> (including IPv6) to a single network definition to verify they are being
> correctly parsed and formatted.
> +<!-- Based on http://blog.mes-stats.fr/2008/10/09/regex-ipv4-et-ipv6 -->
> +<define name='ipv6-addr'>
> +<data type='string'>
> +<!-- To understand this better, take apart the toplevel '|'s -->
> +<param name="pattern">
(([0-9A-Fa-f]{1,4}:){7}[0-9A-Fa-f]{1,4})
|(([0-9A-Fa-f]{1,4}:){6}:[0-9A-Fa-f]{1,4})
|(([0-9A-Fa-f]{1,4}:){5}:([0-9A-Fa-f]{1,4}:)?[0-9A-Fa-f]{1,4})
|(([0-9A-Fa-f]{1,4}:){4}:([0-9A-Fa-f]{1,4}:){0,2}[0-9A-Fa-f]{1,4})
|(([0-9A-Fa-f]{1,4}:){3}:([0-9A-Fa-f]{1,4}:){0,3}[0-9A-Fa-f]{1,4})
|(([0-9A-Fa-f]{1,4}:){2}:([0-9A-Fa-f]{1,4}:){0,4}[0-9A-Fa-f]{1,4})
|(([0-9A-Fa-f]{1,4}:){6}((((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2})))\.){3}(((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2}))))
This doesn't properly represent the dotted IPv4 suffix (it allows
abcd::00.00.00.00). This should reuse the IPv4 pattern (swap 200 to
occur in the pattern before 100, and avoid double 0):
(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))
|(([0-9A-Fa-f]{1,4}:){0,5}:((((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2})))\.){3}(((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2}))))
same problem with the IPv4 portion
|(::([0-9A-Fa-f]{1,4}:){0,5}((((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2})))\.){3}(((25[0-5])|(1[0-9]{2})|(2[0-4][0-9])|([0-9]{1,2}))))
and again
|([0-9A-Fa-f]{1,4}::([0-9A-Fa-f]{1,4}:){0,5}[0-9A-Fa-f]{1,4})
|(::([0-9A-Fa-f]{1,4}:){0,6}[0-9A-Fa-f]{1,4})
|(([0-9A-Fa-f]{1,4}:){1,7}:)
Wow - that's quite the strict regexp. I probably would have copped out
and done the much shorter
[:0-9a-fA-F.]+
Wow. Great analysis of a strict regexp! :-) I would have copped out too,
but already had a regexp written by David Lutterkort for netcf, so I
figured I'd take the "easy" way out and re-use it.
I've changed the IPv6 regexp to replicate the fixed IPv4 regexp in each
of the places you point out.
and filter out the non-IPv6 strings later, but since you already
have
the regex, we might as well keep it.
> void virNetworkDefFree(virNetworkDefPtr def)
> {
> - int i;
> + int ii;
This rename looks funny.
It's not exactly a rename - I removed the original use (for nhosts), and
put in my own new loop. I prefer to use "ii" as an anonymous loop
variable rather than "i" because it makes it easier to search for (no
false positives).
> + /* parse/validate netmask */
> + if (netmask) {
> + if (address == NULL) {
> + /* netmask is meaningless without an address */
> + virNetworkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("netmask specified without address in
network '%s'"),
> + networkName);
> + goto error;
> + }
> +
> + if (!VIR_SOCKET_IS_FAMILY(&def->address, AF_INET)) {
> + virNetworkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("netmask not supported for address
'%s' in network '%s' (IPv4 only)"),
> + address, networkName);
> + goto error;
> + }
> +
> + if (def->prefix> 0) {
> + /* can't have both netmask and prefix at the same time */
> + virNetworkReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("network '%s' has
prefix='%u' but no address"),
> + networkName, def->prefix);
Should this message be "network '%s' cannot have both prefix='%u'
and a
netmask"?
Yes. Fixed.
> diff --git a/src/conf/network_conf.h b/src/conf/network_conf.h
> index a922d28..a51794d 100644
> --- a/src/conf/network_conf.h
> +++ b/src/conf/network_conf.h
> @@ -56,6 +56,33 @@ struct _virNetworkDHCPHostDef {
> virSocketAddr ip;
> };
>
> +typedef struct _virNetworkIpDef virNetworkIpDef;
> +typedef virNetworkIpDef *virNetworkIpDefPtr;
> +struct _virNetworkIpDef {
> + char *family; /* ipv4 or ipv6 - default is ipv4 */
> + virSocketAddr address; /* Bridge IP address */
> +
> + /* The first two items below are taken directly from the XML (one
> + * or the other is given, but not both) and the 3rd is derived
> + * from the first two. When formatting XML, always use netMasktStr
Typo in netMasktStr?
That entire comment was out of date - I had originally stored the
netmask as a string in addition to the virSocketAddr for some silly
reason. I've changed the comment to just point out that either prefix or
netmask will be valid (guaranteed by parser), and that the API functions
should be used rather than directly accessing the values.
> + int nips;
s/int/size_t/
Okay, I've changed it. I'll point out that the great majority of the
"n<thing>s" variables in *_conf.h are defined as int (and some more as
unsigned int). Should these all be standardized at some point?
> + virNetworkIpDefPtr ips; /* ptr to array of IP addresses on
this network */
> };
>
> typedef struct _virNetworkObj virNetworkObj;
> +virNetworkIpDefPtr
> +virNetworkDefGetIpByIndex(const virNetworkDefPtr def,
> + int family, int n);
Should n be size_t?
Sure, why not.
> virNetworkFindByUUID;
> virNetworkLoadAllConfigs;
> +virNetworkIpDefNetmask;
> +virNetworkIpDefPrefix;
Sorting.
Oops.
Close call as to whether I pointed out enough things to warrant a v3,
or
if you should just fix them.
I was going to attach a delta diff, but realized after the fact that I
didn't know how to get a diff between an old and new version of a commit
once I'd rebased. Instead, I'm pasting the new regexp below for you to
review; that's the only significant change. The others have all been
squashed in as well.
I've also incorporated all the fixes you suggested in your reviews of
01-08, but will wait to reply to those messages with a "pushed with
fixes" once the whole set is ACKed and I push them all.
Thanks again for the very thorough and timely reviews. I know this isn't
the easiest time of year to take on extra tasks!
************************
The new IPv6 regexp (constructed by first de-constructing the old
regexp, then removing the IPv4 sub-expressions and replacing them with
the fixed version from ipv4-addr, and finally concatenating everything
back together):
<param
name="pattern">(([0-9A-Fa-f]{1,4}:){7}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}:[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){5}:([0-9A-Fa-f]{1,4}:)?[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){4}:([0-9A-Fa-f]{1,4}:){0,2}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){3}:([0-9A-Fa-f]{1,4}:){0,3}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){2}:([0-9A-Fa-f]{1,4}:){0,4}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){6}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(([0-9A-Fa-f]{1,4}:){0,5}:(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|(::([0-9A-Fa-f]{1,4}:){0,5}(((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9]))\.){3}((25[0-5])|(2[0-4][0-9])|(1[0-9]{2})|([1-9][0-9])|([0-9])))|([0-9A-Fa-f]{1,4}::([0-9A-Fa-f]{1,4}:){0,5}[0-9A-Fa-f]{1,4})|(::([0-9A-Fa-f]{1,4}:){0,6}[0-9A-Fa-f]{1,4})|(([0-9A-Fa-f]{1,4}:){1,7}:)</param>