[libvirt] [PATCH] update input ip processing
Laine Stump
laine at laine.org
Mon Apr 22 18:11:37 UTC 2013
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 at 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)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-things-to-squash-in.patch
Type: text/x-patch
Size: 5451 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130422/41e52af9/attachment-0001.bin>
More information about the libvir-list
mailing list