[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]