[libvirt] [PATCH 3/3] Support for IPv6 / multiple addresses per interface in virInterface

[Daniel P. Berrange]

On Fri, Oct 16, 2009 at 10:56:19AM -0400, laine at laine.org wrote:
>  static int
> -virInterfaceDefParseProtoIPv4(virConnectPtr conn, virInterfaceDefPtr def,
> +virInterfaceDefParseProtoIPv4(virConnectPtr conn, virInterfaceProtocolDefPtr def,
>                                xmlXPathContextPtr ctxt) {
> -    xmlNodePtr dhcp, ip;
> -    int ret = 0;
> +    xmlNodePtr dhcp;
> +    xmlNodePtr *ipNodes = NULL;
> +    int nIpNodes, ii, ret = 0;
> +    char *tmp;
> +
> +    tmp = virXPathString(conn, "string(./route[1]/@gateway)", ctxt);
> +    def->gateway = tmp;
>  
>      dhcp = virXPathNode(conn, "./dhcp", ctxt);
>      if (dhcp != NULL)
>          ret = virInterfaceDefParseDhcp(conn, def, dhcp, ctxt);
> +    if (ret != 0)
> +        return(ret);
> +
> +    nIpNodes = virXPathNodeSet(conn, "./ip", ctxt, &ipNodes);
> +    if (ipNodes == NULL)
> +        return 0;
> +
> +    if (VIR_ALLOC_N(def->ips, nIpNodes) < 0) {
> +        virReportOOMError(conn);
> +        ret = -1;
> +        goto error;
> +    }
> +
> +    def->nips = 0;
> +    for (ii = 0; ii < nIpNodes; ii++) {
>  
> +        virInterfaceIpDefPtr ip;
> +
> +        if (VIR_ALLOC(ip) < 0) {
> +            virReportOOMError(conn);
> +            break;
> +        }
> +
> +        ctxt->node = ipNodes[ii];
> +        ret = virInterfaceDefParseIp(conn, ip, ctxt);
> +        if (ret != 0) {
> +            virInterfaceIpDefFree(ip);
> +            break;
> +        }
> +        def->ips[def->nips++] = ip;
> +    }
> +
> +error:
> +    VIR_FREE(ipNodes);
> +    return(ret);
> +}

I don't think the error reporting is quite right here. At the time of
declaration you  initialize 'ret = 0', so those two error cases inside
the for loop which break out will end up returning 0. I think it is
better to initialize 'ret = -1', change the 'break' to 'goto error',
and after the for loop put 'ret = 0' to mark success. 

> +
> +static int
> +virInterfaceDefParseProtoIPv6(virConnectPtr conn, virInterfaceProtocolDefPtr def,
> +                              xmlXPathContextPtr ctxt) {
> +    xmlNodePtr dhcp, autoconf;
> +    xmlNodePtr *ipNodes = NULL;
> +    int nIpNodes, ii, ret = 0;
> +    char *tmp;
> +
> +    tmp = virXPathString(conn, "string(./route[1]/@gateway)", ctxt);
> +    def->gateway = tmp;
> +
> +    autoconf = virXPathNode(conn, "./autoconf", ctxt);
> +    if (autoconf != NULL)
> +        def->autoconf = 1;
> +
> +    dhcp = virXPathNode(conn, "./dhcp", ctxt);
> +    if (dhcp != NULL)
> +        ret = virInterfaceDefParseDhcp(conn, def, dhcp, ctxt);
>      if (ret != 0)
>          return(ret);
>  
> -    ip = virXPathNode(conn, "./ip", ctxt);
> -    if (ip != NULL)
> -        ret = virInterfaceDefParseIp(conn, def, ip, ctxt);
> +    nIpNodes = virXPathNodeSet(conn, "./ip", ctxt, &ipNodes);
> +    if (ipNodes == NULL)
> +        return 0;
> +
> +    if (VIR_ALLOC_N(def->ips, nIpNodes) < 0) {
> +        virReportOOMError(conn);
> +        ret = -1;
> +        goto error;
> +    }
> +
> +    def->nips = 0;
> +    for (ii = 0; ii < nIpNodes; ii++) {
> +
> +        virInterfaceIpDefPtr ip;
> +
> +        if (VIR_ALLOC(ip) < 0) {
> +            virReportOOMError(conn);
> +            break;
> +        }
> +
> +        ctxt->node = ipNodes[ii];
> +        ret = virInterfaceDefParseIp(conn, ip, ctxt);
> +        if (ret != 0) {
> +            virInterfaceIpDefFree(ip);
> +            break;
> +        }
> +        def->ips[def->nips++] = ip;
> +    }
> +
> +error:
> +    VIR_FREE(ipNodes);
>      return(ret);
>  }

Same here - that looks like it'll return 0 for several error
cases.


Regards,
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 :|