[libvirt] [PATCH] Improve error reporting when parsing dhcp info for virtual networks
This is partially in response to https://bugzilla.redhat.com/show_bug.cgi?id=653300 The crash in that report was coincidentally fixed when we switched from using inet_pton() to using virSocketParseAddr(), but the absence of an ip address in a dhcp static host definition was still silently ignored (and that entry discarded from the saved XML). This patch turns that into a logged failure; likewise if the entry has neither a mac address nor a name attribute (the entry is useless without at least one of those, plus an ip address). Since the network name is now pulled into this function in order for those error logs to be more informative, the other error messages in the function have also been changed to take advantage. --- src/conf/network_conf.c | 32 +++++++++++++++++++------------- 1 files changed, 19 insertions(+), 13 deletions(-) diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c index 420b94a..fe91617 100644 --- a/src/conf/network_conf.c +++ b/src/conf/network_conf.c @@ -295,7 +295,8 @@ int virNetworkIpDefNetmask(const virNetworkIpDefPtr def, static int -virNetworkDHCPRangeDefParseXML(virNetworkIpDefPtr def, +virNetworkDHCPRangeDefParseXML(const char *networkName, + virNetworkIpDefPtr def, xmlNodePtr node) { @@ -333,8 +334,8 @@ virNetworkDHCPRangeDefParseXML(virNetworkIpDefPtr def, range = virSocketGetRange(&saddr, &eaddr); if (range < 0) { virNetworkReportError(VIR_ERR_XML_ERROR, - _("dhcp range '%s' to '%s' invalid"), - start, end); + _("Invalid dhcp range '%s' to '%s' in network '%s'"), + start, end, networkName); VIR_FREE(start); VIR_FREE(end); return -1; @@ -359,33 +360,38 @@ virNetworkDHCPRangeDefParseXML(virNetworkIpDefPtr def, if ((mac != NULL) && (virParseMacAddr(mac, &addr[0]) != 0)) { virNetworkReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot parse MAC address '%s'"), - mac); + _("Cannot parse MAC address '%s' in network '%s'"), + mac, networkName); VIR_FREE(mac); } name = virXMLPropString(cur, "name"); if ((name != NULL) && (!c_isalpha(name[0]))) { virNetworkReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot use name address '%s'"), - name); + _("Cannot use name address '%s' in network '%s'"), + name, networkName); VIR_FREE(name); } /* * You need at least one MAC address or one host name */ if ((mac == NULL) && (name == NULL)) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("Static host definition in network '%s' must have mac or name attribute"), + networkName); VIR_FREE(mac); VIR_FREE(name); - cur = cur->next; - continue; + return -1; } ip = virXMLPropString(cur, "ip"); - if (virSocketParseAddr(ip, &inaddr, AF_UNSPEC) < 0) { + if ((ip == NULL) || + (virSocketParseAddr(ip, &inaddr, AF_UNSPEC) < 0)) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("Missing IP address in static host definition for network '%s'"), + networkName); VIR_FREE(ip); VIR_FREE(mac); VIR_FREE(name); - cur = cur->next; - continue; + return -1; } VIR_FREE(ip); if (VIR_REALLOC_N(def->hosts, def->nhosts + 1) < 0) { @@ -541,7 +547,7 @@ virNetworkIPParseXML(const char *networkName, while (cur != NULL) { if (cur->type == XML_ELEMENT_NODE && xmlStrEqual(cur->name, BAD_CAST "dhcp")) { - result = virNetworkDHCPRangeDefParseXML(def, cur); + result = virNetworkDHCPRangeDefParseXML(networkName, def, cur); if (result) goto error; -- 1.7.3.4
On 01/04/2011 11:14 PM, Laine Stump wrote:
This is partially in response to
https://bugzilla.redhat.com/show_bug.cgi?id=653300
The crash in that report was coincidentally fixed when we switched from using inet_pton() to using virSocketParseAddr(), but the absence of an ip address in a dhcp static host definition was still silently ignored (and that entry discarded from the saved XML). This patch turns that into a logged failure; likewise if the entry has neither a mac address nor a name attribute (the entry is useless without at least one of those, plus an ip address).
/* * You need at least one MAC address or one host name */ if ((mac == NULL) && (name == NULL)) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("Static host definition in network '%s' must have mac or name attribute"), + networkName); VIR_FREE(mac); VIR_FREE(name);
These two frees are pointless, given that you can only get here if both variables are NULL. (might have been my fault in a previous patch, but we should clean it up now while touching it again). ACK with that nit fixed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On 01/05/2011 11:26 AM, Eric Blake wrote:
On 01/04/2011 11:14 PM, Laine Stump wrote:
This is partially in response to
https://bugzilla.redhat.com/show_bug.cgi?id=653300
The crash in that report was coincidentally fixed when we switched from using inet_pton() to using virSocketParseAddr(), but the absence of an ip address in a dhcp static host definition was still silently ignored (and that entry discarded from the saved XML). This patch turns that into a logged failure; likewise if the entry has neither a mac address nor a name attribute (the entry is useless without at least one of those, plus an ip address). /* * You need at least one MAC address or one host name */ if ((mac == NULL)&& (name == NULL)) { + virNetworkReportError(VIR_ERR_XML_ERROR, + _("Static host definition in network '%s' must have mac or name attribute"), + networkName); VIR_FREE(mac); VIR_FREE(name); These two frees are pointless, given that you can only get here if both variables are NULL. (might have been my fault in a previous patch,
Nope. git blame says it's been there since the day support for static dhcp hosts was added, so you're off the hook ;-)
but we should clean it up now while touching it again).
ACK with that nit fixed.
Thanks. pushed with your suggested change.
participants (2)
-
Eric Blake -
Laine Stump