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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list