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@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/