On Fri, Oct 16, 2009 at 10:56:19AM -0400, laine@xxxxxxxxx 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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list