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 <[email protected]>
> ---
> 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 <[email protected]>
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
--
libvir-list mailing list
[email protected]
https://www.redhat.com/mailman/listinfo/libvir-list
