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@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list