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)