[libvirt] [PATCH] 1/3 add support for netcf XML import and export
Basically this implement the routines to read an netcf XML definition and build the associated internal data structures. It does the checking of the input XML up to making sure all the needed informations are availble in the right place, but does not check for extra data. It should support the full format as defined by interface.rng as of netcf version 0.1.0, both for input and output. This is rather repetitive and boring code, so I tried to factorize things following patterms similar to the one used in the grammar, the code reflects to a large extent the definition blocks there. The main data structure is virInterfaceDef, it defines the most common and shared attributes of all interfaces, the protocol definition allowed in the schemas is only ipv4 at the moment but with the expected extension to allow ipv6 in parallel I made a separate structure (currently embedded in the main one). There is a enum discriminating the specific structures needed for vlan, bride and bonding, and for the two last ones, a dynamically allocated array of bare interfaces (which can be of vlan or ethernet type, merged into a single structure). This passes valgrind, but since I'm not testing agaisnt misformed XML input I afraid of potential leaks on exit paths, otherwise this should be fine. 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/
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 :|
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/
On Wed, Jul 15, 2009 at 05:40:30PM +0200, Daniel Veillard wrote:
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.
Okay I commited the 3 patches (with some help from Jim, git can be surprizing for newbies !) , we can still change the enum handling, at least it's there to test, thanks, 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/
Thanks, Daniel! I have to go buy a van right now, but I'll be trying this out in a few hours!
On Wed, Jul 15, 2009 at 06:51:23AM -0400, Laine Stump wrote:
Thanks, Daniel!
I have to go buy a van right now, but I'll be trying this out in a few hours!
git pull and try/look ! Now we should commit your couple of patches from last week, 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/
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Laine Stump