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

Re: [libvirt] [PATCH] update input ip processing



On 04/21/2013 10:34 AM, Gene Czarcinski wrote:
> 1. Handle invalid ULong prefix specified.
> When parsing for @prefix as a ULong, a -2 can be returned
> if the specification is not a valid ULong.
>
> 2.  Error out if address= is not specified.
>
> 3.  Merge netmask process/tests under family tests.
>
> 4. Max sure that prefix does not exceed maximum.
> .
> Signed-off-by: Gene Czarcinski <gene czarc net>
> ---
>  src/conf/network_conf.c | 118 +++++++++++++++++++++++++++---------------------
>  1 file changed, 66 insertions(+), 52 deletions(-)
>
> diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
> index 2b56ca7..1503ece 100644
> --- a/src/conf/network_conf.c
> +++ b/src/conf/network_conf.c
> @@ -1136,7 +1136,8 @@ virNetworkIPDefParseXML(const char *networkName,
>  
>      xmlNodePtr cur, save;
>      char *address = NULL, *netmask = NULL;
> -    unsigned long prefix;
> +    unsigned long prefix = 0;
> +    int prefixRc;
>      int result = -1;
>  
>      save = ctxt->node;
> @@ -1144,14 +1145,8 @@ virNetworkIPDefParseXML(const char *networkName,
>  
>      /* grab raw data from XML */
>      def->family = virXPathString("string(./@family)", ctxt);
> -    address = virXPathString("string(./@address)", ctxt);
> -    if (virXPathULong("string(./@prefix)", ctxt, &prefix) < 0)
> -        def->prefix = 0;
> -    else
> -        def->prefix = prefix;
> -
> -    netmask = virXPathString("string(./@netmask)", ctxt);
>  
> +    address = virXPathString("string(./@address)", ctxt);
>      if (address) {
>          if (virSocketAddrParse(&def->address, address, AF_UNSPEC) < 0) {
>              virReportError(VIR_ERR_XML_ERROR,

Since I decided to just tweak a couple messages in this patch, I
modified the existing log message here (not shown by diff) to:

                       _("Invalid address '%s' in network '%s'"),

> @@ -1160,23 +1155,66 @@ virNetworkIPDefParseXML(const char *networkName,
>              goto cleanup;
>          }
>  
> +    } else {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("Network address must be specified in definition of network '%s'"),

                       _("Missing required address attribute in network
'%s'"),


> +                       networkName);
> +        goto cleanup;
> +    }
> +

Structuring it this way leads to less indentation:

if (!address) {
   log error
   goto cleanup;
}
if (virSocketAddrParse(....) < 0) {
    log error
    goto cleanup;
}

> +    netmask = virXPathString("string(./@netmask)", ctxt);
> +    if (netmask) {
> +        if (virSocketAddrParse(&def->netmask, netmask, AF_UNSPEC) < 0) {
> +            virReportError(VIR_ERR_XML_ERROR,
> +                           _("Bad netmask '%s' in definition of network '%s'"),

                           _("Invalid netmask '%s' in network '%s'"),

> +                           netmask, networkName);
> +            goto cleanup;
> +        }
> +
> +    }


Those two nested ifs can be combined into a single if.


> +
> +    prefixRc = virXPathULong("string(./@prefix)", ctxt, &prefix);
> +    if (prefixRc == -2) {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("Invalid ULong value specified for prefix in definition of network '%s'"),

                       _("Invalid prefix in network '%s'"),

(I tweaked a few other messages, and will attach an interdiff of what I
pushed at the bottom of this message)


> +                       networkName);
> +        goto cleanup;
>      }
> +    if (prefixRc < 0)
> +        def->prefix = 0;
> +    else
> +        def->prefix = prefix;
>  
> -    /* validate family vs. address */
> -    if (def->family == NULL) {
> +    /* validate address, etc. for each family */
> +    if ((def->family == NULL) || (STREQ(def->family, "ipv4"))) {
>          if (!(VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET) ||
>                VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_UNSPEC))) {
>              virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                           _("no family specified for non-IPv4 address '%s' in network '%s'"),
> -                           address, networkName);
> +                           _("%s family specified for non-IPv4 address '%s' in network '%s'"),
> +                           def->family == NULL? "no" : "ipv4", address, networkName);
>              goto cleanup;
>          }
> -    } else if (STREQ(def->family, "ipv4")) {
> -        if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET)) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                           _("family 'ipv4' specified for non-IPv4 address '%s' in network '%s'"),
> -                           address, networkName);
> -            goto cleanup;
> +        if (netmask) {
> +            if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->netmask, AF_INET)) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("network '%s' has invalid netmask '%s' for address '%s' (both must be IPv4)"),
> +                               networkName, netmask, address);
> +                goto cleanup;
> +            }
> +            if (def->prefix > 0) {
> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("network '%s' cannot have both a prefix and a netmask"),
> +                               networkName);
> +                goto cleanup;
> +            }
> +        }
> +        else {
> +            if (def->prefix > 32 ) {

    } else if (def->prefix > 32) {

> +                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> +                               _("Invalid IPv4 prefix '%lu'specified in network'%s'"),
> +                               prefix, networkName);
> +                goto cleanup;
> +            }
>          }
>      } else if (STREQ(def->family, "ipv6")) {
>          if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET6)) {
> @@ -1185,47 +1223,23 @@ virNetworkIPDefParseXML(const char *networkName,
>                             address, networkName);
>              goto cleanup;
>          }
> -    } else {
> -        virReportError(VIR_ERR_XML_ERROR,
> -                       _("Unrecognized family '%s' in definition of network '%s'"),
> -                       def->family, networkName);
> -        goto cleanup;
> -    }
> -
> -    /* parse/validate netmask */
> -    if (netmask) {
> -        if (address == NULL) {
> -            /* netmask is meaningless without an address */
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                           _("netmask specified without address in network '%s'"),
> -                           networkName);
> -            goto cleanup;
> -        }
> -
> -        if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET)) {
> +        if (netmask) {
>              virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                           _("netmask not supported for address '%s' in network '%s' (IPv4 only)"),
> +                           _("specifying netmask invalid for IPv6 address '%s' in network '%s'"),
>                             address, networkName);
>              goto cleanup;
>          }
> -
> -        if (def->prefix > 0) {
> -            /* can't have both netmask and prefix at the same time */
> +        if (def->prefix > 128 ) {
>              virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                           _("network '%s' cannot have both prefix='%u' and a netmask"),
> -                           networkName, def->prefix);
> -            goto cleanup;
> -        }
> -
> -        if (virSocketAddrParse(&def->netmask, netmask, AF_UNSPEC) < 0)
> -            goto cleanup;
> -
> -        if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->netmask, AF_INET)) {
> -            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> -                           _("network '%s' has invalid netmask '%s' for address '%s' (both must be IPv4)"),
> -                           networkName, netmask, address);
> +                           _("Invalid IPv6 prefix '%lu'specified in network'%s'"),
> +                           prefix, networkName);
>              goto cleanup;
>          }
> +    } else {
> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("Unrecognized family '%s' in definition of network '%s'"),
> +                       def->family, networkName);
> +        goto cleanup;
>      }
>  
>      cur = node->children;

Nice cleanup!

I made the two small changes to logic (collapsing nested ifs), changed a
few log messages as detailed in the attached interdiff, and pushed
(along with patch 1/2 of your <route> patches)

>From 3ed550be22bcc350656368bcad3c00ae8406cf5f Mon Sep 17 00:00:00 2001
From: Laine Stump <laine laine org>
Date: Mon, 22 Apr 2013 14:02:14 -0400
Subject: [PATCH] things to squash in

---
 src/conf/network_conf.c | 64 ++++++++++++++++++++++---------------------------
 1 file changed, 29 insertions(+), 35 deletions(-)

diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 97c20e6..1c88547 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -1136,30 +1136,26 @@ virNetworkIPDefParseXML(const char *networkName,
     def->family = virXPathString("string(./@family)", ctxt);
 
     address = virXPathString("string(./@address)", ctxt);
-    if (address) {
-        if (virSocketAddrParse(&def->address, address, AF_UNSPEC) < 0) {
-            virReportError(VIR_ERR_XML_ERROR,
-                           _("Bad address '%s' in definition of network '%s'"),
-                           address, networkName);
-            goto cleanup;
-        }
-
-    } else {
+    if (!address) {
         virReportError(VIR_ERR_XML_ERROR,
-                       _("Network address must be specified in definition of network '%s'"),
+                       _("Missing required address attribute in network '%s'"),
                        networkName);
         goto cleanup;
     }
+    if (virSocketAddrParse(&def->address, address, AF_UNSPEC) < 0) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       _("Invalid address '%s' in network '%s'"),
+                       address, networkName);
+        goto cleanup;
+    }
 
     netmask = virXPathString("string(./@netmask)", ctxt);
-    if (netmask) {
-        if (virSocketAddrParse(&def->netmask, netmask, AF_UNSPEC) < 0) {
-            virReportError(VIR_ERR_XML_ERROR,
-                           _("Bad netmask '%s' in definition of network '%s'"),
-                           netmask, networkName);
-            goto cleanup;
-        }
-
+    if (netmask &&
+        (virSocketAddrParse(&def->netmask, netmask, AF_UNSPEC) < 0)) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       _("Invalid netmask '%s' in network '%s'"),
+                       netmask, networkName);
+        goto cleanup;
     }
 
     prefixRc = virXPathULong("string(./@prefix)", ctxt, &prefix);
@@ -1186,47 +1182,45 @@ virNetworkIPDefParseXML(const char *networkName,
         if (netmask) {
             if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->netmask, AF_INET)) {
                 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                               _("network '%s' has invalid netmask '%s' for address '%s' (both must be IPv4)"),
-                               networkName, netmask, address);
+                               _("Invalid netmask '%s' for address '%s' "
+                                 "in network '%s' (both must be IPv4)"),
+                               netmask, address, networkName);
                 goto cleanup;
             }
             if (def->prefix > 0) {
                 virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                               _("network '%s' cannot have both a prefix and a netmask"),
-                               networkName);
-                goto cleanup;
-            }
-        }
-        else {
-            if (def->prefix > 32 ) {
-                virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                               _("Invalid IPv4 prefix '%lu'specified in network'%s'"),
-                               prefix, networkName);
+                               _("Network '%s' IP address cannot have "
+                                 "both a prefix and a netmask"), networkName);
                 goto cleanup;
             }
+        } else if (def->prefix > 32) {
+            virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+                           _("Invalid IPv4 prefix '%lu' in network '%s'"),
+                           prefix, networkName);
+            goto cleanup;
         }
     } else if (STREQ(def->family, "ipv6")) {
         if (!VIR_SOCKET_ADDR_IS_FAMILY(&def->address, AF_INET6)) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("family 'ipv6' specified for non-IPv6 address '%s' in network '%s'"),
+                           _("Family 'ipv6' specified for non-IPv6 address '%s' in network '%s'"),
                            address, networkName);
             goto cleanup;
         }
         if (netmask) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("specifying netmask invalid for IPv6 address '%s' in network '%s'"),
+                           _("netmask not allowed for IPv6 address '%s' in network '%s'"),
                            address, networkName);
             goto cleanup;
         }
-        if (def->prefix > 128 ) {
+        if (def->prefix > 128) {
             virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
-                           _("Invalid IPv6 prefix '%lu'specified in network'%s'"),
+                           _("Invalid IPv6 prefix '%lu' in network '%s'"),
                            prefix, networkName);
             goto cleanup;
         }
     } else {
         virReportError(VIR_ERR_XML_ERROR,
-                       _("Unrecognized family '%s' in definition of network '%s'"),
+                       _("Unrecognized family '%s' in network '%s'"),
                        def->family, networkName);
         goto cleanup;
     }
-- 
1.7.11.7


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