[libvirt] [PATCH] 1/3 add support for netcf XML import and export

[Daniel P. Berrange]

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 :|