[libvirt] [PATCH V2] Fix parsing of bond interface XML
Noticed that parsing bond interface XML containing the miimon element fails <interface type="bond" name="bond0"> ... <bond mode="active-backup"> <miimon freq="100" carrier="netif"/> ... </bond> </interface> This configuration does not contain the optional updelay and downdelay attributes, but parsing will fail due to returning the result of virXPathULong (a -1 when the attribute doesn't exist) from virInterfaceDefParseBond after examining the updelay attribute. While fixing this bug, cleanup the function to use virXPathInt instead of virXPathULong, and store the result directly instead of using a tmp variable. Using virXPathInt actually fixes a potential silent truncation bug noted by Eric Blake. Also, there is no cleaup in the error label. Remove the label, returning failure where failure occurs and success if the end of the function is reached. --- V2: Use virXPathInt instead of virXPathULong to avoid silent truncation, and store value directly in the def structure instead of using a tmp variable. src/conf/interface_conf.c | 63 ++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 36 deletions(-) diff --git a/src/conf/interface_conf.c b/src/conf/interface_conf.c index 9301ec0..7ca9c86 100644 --- a/src/conf/interface_conf.c +++ b/src/conf/interface_conf.c @@ -572,81 +572,72 @@ error: static int virInterfaceDefParseBond(virInterfaceDefPtr def, xmlXPathContextPtr ctxt) { - int ret = -1; - unsigned long tmp; + int res; def->data.bond.mode = virInterfaceDefParseBondMode(ctxt); if (def->data.bond.mode < 0) - goto error; + return -1; - ret = virInterfaceDefParseBondItfs(def, ctxt); - if (ret != 0) - goto error; + if (virInterfaceDefParseBondItfs(def, ctxt) != 0) + return -1; if (virXPathNode("./miimon[1]", ctxt) != NULL) { def->data.bond.monit = VIR_INTERFACE_BOND_MONIT_MII; - ret = virXPathULong("string(./miimon/@freq)", ctxt, &tmp); - if ((ret == -2) || (ret == -1)) { + res = virXPathInt("string(./miimon/@freq)", ctxt, + &def->data.bond.frequency); + if ((res == -2) || (res == -1)) { virReportError(VIR_ERR_XML_ERROR, "%s", _("bond interface miimon freq missing or invalid")); - goto error; + return -1; } - def->data.bond.frequency = (int) tmp; - ret = virXPathULong("string(./miimon/@downdelay)", ctxt, &tmp); - if (ret == -2) { + res = virXPathInt("string(./miimon/@downdelay)", ctxt, + &def->data.bond.downdelay); + if (res == -2) { virReportError(VIR_ERR_XML_ERROR, "%s", _("bond interface miimon downdelay invalid")); - goto error; - } else if (ret == 0) { - def->data.bond.downdelay = (int) tmp; + return -1; } - ret = virXPathULong("string(./miimon/@updelay)", ctxt, &tmp); - if (ret == -2) { + res = virXPathInt("string(./miimon/@updelay)", ctxt, + &def->data.bond.updelay); + if (res == -2) { virReportError(VIR_ERR_XML_ERROR, "%s", _("bond interface miimon updelay invalid")); - goto error; - } else if (ret == 0) { - def->data.bond.updelay = (int) tmp; + return -1; } def->data.bond.carrier = virInterfaceDefParseBondMiiCarrier(ctxt); - if (def->data.bond.carrier < 0) { - ret = -1; - goto error; - } + if (def->data.bond.carrier < 0) + return -1; } else if (virXPathNode("./arpmon[1]", ctxt) != NULL) { def->data.bond.monit = VIR_INTERFACE_BOND_MONIT_ARP; - ret = virXPathULong("string(./arpmon/@interval)", ctxt, &tmp); - if ((ret == -2) || (ret == -1)) { + res = virXPathInt("string(./arpmon/@interval)", ctxt, + &def->data.bond.interval); + if ((res == -2) || (res == -1)) { virReportError(VIR_ERR_XML_ERROR, "%s", _("bond interface arpmon interval missing or invalid")); - goto error; + return -1; } - def->data.bond.interval = (int) tmp; def->data.bond.target = virXPathString("string(./arpmon/@target)", ctxt); if (def->data.bond.target == NULL) { virReportError(VIR_ERR_XML_ERROR, "%s", _("bond interface arpmon target missing")); - ret = -1; - goto error; + return -1; } def->data.bond.validate = virInterfaceDefParseBondArpValid(ctxt); - if (def->data.bond.validate < 0) { - ret = -1; - goto error; - } + if (def->data.bond.validate < 0) + return -1; } -error: - return ret; + + return 0; } static int -- 1.8.0.1
On 03/21/2013 05:13 PM, Jim Fehlig wrote:
Noticed that parsing bond interface XML containing the miimon element fails
<interface type="bond" name="bond0"> ... <bond mode="active-backup"> <miimon freq="100" carrier="netif"/> ... </bond> </interface>
This configuration does not contain the optional updelay and downdelay attributes, but parsing will fail due to returning the result of virXPathULong (a -1 when the attribute doesn't exist) from virInterfaceDefParseBond after examining the updelay attribute.
While fixing this bug, cleanup the function to use virXPathInt instead of virXPathULong, and store the result directly instead of using a tmp variable. Using virXPathInt actually fixes a potential silent truncation bug noted by Eric Blake.
Also, there is no cleaup in the error label. Remove the label,
s/cleaup/cleanup/
returning failure where failure occurs and success if the end of the function is reached. ---
ACK -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Eric Blake wrote:
On 03/21/2013 05:13 PM, Jim Fehlig wrote:
Noticed that parsing bond interface XML containing the miimon element fails
<interface type="bond" name="bond0"> ... <bond mode="active-backup"> <miimon freq="100" carrier="netif"/> ... </bond> </interface>
This configuration does not contain the optional updelay and downdelay attributes, but parsing will fail due to returning the result of virXPathULong (a -1 when the attribute doesn't exist) from virInterfaceDefParseBond after examining the updelay attribute.
While fixing this bug, cleanup the function to use virXPathInt instead of virXPathULong, and store the result directly instead of using a tmp variable. Using virXPathInt actually fixes a potential silent truncation bug noted by Eric Blake.
Also, there is no cleaup in the error label. Remove the label,
s/cleaup/cleanup/
Will fix.
returning failure where failure occurs and success if the end of the function is reached. ---
ACK
But I'm going to wait to push this until I test it! After fiddling with the network too much on my test machine, I can no longer reach it. I'll have access to that machine tomorrow and will push this after testing. Thanks, Jim
Jim Fehlig wrote:
Eric Blake wrote:
On 03/21/2013 05:13 PM, Jim Fehlig wrote:
Noticed that parsing bond interface XML containing the miimon element fails
<interface type="bond" name="bond0"> ... <bond mode="active-backup"> <miimon freq="100" carrier="netif"/> ... </bond> </interface>
This configuration does not contain the optional updelay and downdelay attributes, but parsing will fail due to returning the result of virXPathULong (a -1 when the attribute doesn't exist) from virInterfaceDefParseBond after examining the updelay attribute.
While fixing this bug, cleanup the function to use virXPathInt instead of virXPathULong, and store the result directly instead of using a tmp variable. Using virXPathInt actually fixes a potential silent truncation bug noted by Eric Blake.
Also, there is no cleaup in the error label. Remove the label,
s/cleaup/cleanup/
Will fix.
returning failure where failure occurs and success if the end of the function is reached. ---
ACK
But I'm going to wait to push this until I test it! After fiddling with the network too much on my test machine, I can no longer reach it. I'll have access to that machine tomorrow and will push this after testing.
Tested now and pushed. Thanks, Jim
participants (2)
-
Eric Blake -
Jim Fehlig