4:15 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
5:22 a.m.
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 :|
10:40 a.m.
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/
3:10 p.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
5:51 a.m.
Thanks, Daniel!
I have to go buy a van right now, but I'll be trying this out in a few
hours!
3:11 p.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
5663
days inactive
5663
days old
5 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Laine Stump