Re: [libvirt] [PATCHv2 09/13] Change virtual network XML parsing/formatting to support IPv6

23 Dec 2010

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/
...
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.
...
+  
+  <define name='ipv6-addr'>
+    <data type='string'>
+      
+      <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.]+

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.
...
+    /* 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"?
...

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?
...
+    int nips;
s/int/size_t/
...
+    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?
...
virNetworkFindByUUID;
 virNetworkLoadAllConfigs;
+virNetworkIpDefNetmask;
+virNetworkIpDefPrefix;
Sorting.

Close call as to whether I pointed out enough things to warrant a v3, or
if you should just fix them.

-- 
Eric Blake   eblake@redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

    