[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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.

> +  <!-- 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.]+

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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]