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(a)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)