Re: [libvirt] [PATCH] 1/3 add support for netcf XML import and export
On Wed, Jul 15, 2009 at 11:22:43AM +0100, Daniel P. Berrange wrote:
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.
Ah, right ! Fixed.
> + 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
> + 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
[...]
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.
Well the current macros makes it impossible unless the enum names are
all suffixed by 'Type' which IMHO constraints it a bit more than it
should, that's my main beef against using it more widely, I'm using it
for virInterfaceType. Another problem I have with the macros is that
I used the enum value _NONE mapped to 0 to express that the value wasn't
defined by the user and I don't see a good way to map that to
VIR_ENUM_IMPL , NULL might work but you end up doing STREQ(NULL, something)
and I'm not sure strcmp(NULL, NULL) will work correctly ... well the man
page doesn't say anything about NULL parameter(s) , I guess
virEnumFromString() and virEnumToString() would have to be modified to
handle my case.
So right now VIR_ENUM doesn't really match what I'm doing, though
agreed it end up with slightly longuer code.
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/