On Wed, Jul 15, 2009 at 11:15:25AM +0200, Daniel Veillard wrote:
+static int +virInterfaceDefParseBasicAttrs(virConnectPtr conn, virInterfaceDefPtr def, + xmlXPathContextPtr ctxt) { + char *tmp; + unsigned long mtu; + int ret; + + tmp = virXPathString(conn, "string(./@name)", ctxt); + if (tmp == NULL) { + virInterfaceReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("interface has no name")); + return(-1); + } + def->name = tmp; + + ret = virXPathULong(conn, "string(./mtu/@size)", ctxt, &mtu); + if ((ret == -2) || ((ret == 0) && (mtu > 100000))) { + virInterfaceReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("interface mtu value is improper")); + } else if (ret == 0) { + def->mtu = (unsigned int) mtu; + } + return(0); +}
I think you need to return '-1' in that second error case.
+static int +virInterfaceDefParseStartMode(virConnectPtr conn, virInterfaceDefPtr def, + xmlXPathContextPtr ctxt) { + char *tmp; + + tmp = virXPathString(conn, "string(./start/@mode)", ctxt); + if (tmp == NULL) { + virInterfaceReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("interface misses the start mode attribute")); + return(-1); + } + if (STREQ(tmp, "onboot")) + def->startmode = VIR_INTERFACE_START_ONBOOT; + else if (STREQ(tmp, "hotplug")) + def->startmode = VIR_INTERFACE_START_HOTPLUG; + else if (STREQ(tmp, "none")) + def->startmode = VIR_INTERFACE_START_NONE;
It'd be nice to use VIR_ENUM_DECL/IMPL for these strings<->enum conversions
+ else { + virInterfaceReportError(conn, VIR_ERR_XML_ERROR, + _("unknown interface startmode %s"), tmp); + VIR_FREE(tmp); + return(-1); + } + VIR_FREE(tmp); + return(0); +} + +static int +virInterfaceDefParseBondMode(virConnectPtr conn, xmlXPathContextPtr ctxt) { + char *tmp; + int ret = 0; + + tmp = virXPathString(conn, "string(./@mode)", ctxt); + if (tmp == NULL) + return(VIR_INTERFACE_BOND_NONE); + if (STREQ(tmp, "balance-rr")) + ret = VIR_INTERFACE_BOND_BALRR; + else if (STREQ(tmp, "active-backup")) + ret = VIR_INTERFACE_BOND_ABACKUP; + else if (STREQ(tmp, "balance-xor")) + ret = VIR_INTERFACE_BOND_BALXOR; + else if (STREQ(tmp, "broadcast")) + ret = VIR_INTERFACE_BOND_BCAST; + else if (STREQ(tmp, "802.3ad")) + ret = VIR_INTERFACE_BOND_8023AD; + else if (STREQ(tmp, "balance-tlb")) + ret = VIR_INTERFACE_BOND_BALTLB; + else if (STREQ(tmp, "balance-alb")) + ret = VIR_INTERFACE_BOND_BALALB;
Likewise this could be done with a VIR_ENUM
+ else { + virInterfaceReportError(conn, VIR_ERR_XML_ERROR, + _("unknown bonding mode %s"), tmp); + ret = -1; + } + VIR_FREE(tmp); + return(ret); +} + +static int +virInterfaceDefParseBondMiiCarrier(virConnectPtr conn, xmlXPathContextPtr ctxt) { + char *tmp; + int ret = 0; + + tmp = virXPathString(conn, "string(./miimon/@carrier)", ctxt); + if (tmp == NULL) + return(VIR_INTERFACE_BOND_MII_NONE); + if (STREQ(tmp, "ioctl")) + ret = VIR_INTERFACE_BOND_MII_IOCTL; + else if (STREQ(tmp, "netif")) + ret = VIR_INTERFACE_BOND_MII_NETIF; + else { + virInterfaceReportError(conn, VIR_ERR_XML_ERROR, + _("unknown mii bonding carrier %s"), tmp); + ret = -1; + } + VIR_FREE(tmp); + return(ret); +} + +static int +virInterfaceDefParseBondArpValid(virConnectPtr conn, xmlXPathContextPtr ctxt) { + char *tmp; + int ret = 0; + + tmp = virXPathString(conn, "string(./arpmon/@validate)", ctxt); + if (tmp == NULL) + return(VIR_INTERFACE_BOND_ARP_NONE); + if (STREQ(tmp, "active")) + ret = VIR_INTERFACE_BOND_ARP_ACTIVE; + else if (STREQ(tmp, "backup")) + ret = VIR_INTERFACE_BOND_ARP_BACKUP; + else if (STREQ(tmp, "all")) + ret = VIR_INTERFACE_BOND_ARP_ALL; + else { + virInterfaceReportError(conn, VIR_ERR_XML_ERROR, + _("unknown arp bonding validate %s"), tmp); + ret = -1; + } + VIR_FREE(tmp); + return(ret); +}
And these two.... Aside from optimizing to use more VIR_ENUMs the code all looks good to me, and i didnt' spot any obvious leaks/errors. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|