On 2012年05月09日 14:15, Alex Jia wrote: > Detected by valgrind. Leaks are introduced in commit 122fa379. > > src/conf/storage_conf.c: fix memory leaks. > > How to reproduce? > $ make&& make -C tests check TESTS=storagepoolxml2xmltest > $ cd tests&& valgrind -v --leak-check=full ./storagepoolxml2xmltest > > actual result: > ==28571== LEAK SUMMARY: > ==28571== definitely lost: 40 bytes in 5 blocks > ==28571== indirectly lost: 0 bytes in 0 blocks > ==28571== possibly lost: 0 bytes in 0 blocks > ==28571== still reachable: 1,054 bytes in 21 blocks > ==28571== suppressed: 0 bytes in 0 blocks > > Signed-off-by: Alex Jia<ajia(a)redhat.com> > --- > src/conf/storage_conf.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c > index 0b34f28..668e679 100644 > --- a/src/conf/storage_conf.c > +++ b/src/conf/storage_conf.c > @@ -462,6 +462,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr > ctxt, > goto cleanup; > } > } > + VIR_FREE(nodeset); > } > } > NACK, not the right fix, "nodeset" will still be used by the 'for' loop.

Osier

On 05/09/2012 08:15 AM, Alex Jia wrote: > Detected by valgrind. Leaks are introduced in commit 122fa379. > > src/conf/storage_conf.c: fix memory leaks. > > How to reproduce? > $ make&& make -C tests check TESTS=storagepoolxml2xmltest > $ cd tests&& valgrind -v --leak-check=full ./storagepoolxml2xmltest > > actual result: > ==28571== LEAK SUMMARY: > ==28571== definitely lost: 40 bytes in 5 blocks > ==28571== indirectly lost: 0 bytes in 0 blocks > ==28571== possibly lost: 0 bytes in 0 blocks > ==28571== still reachable: 1,054 bytes in 21 blocks > ==28571== suppressed: 0 bytes in 0 blocks > > Signed-off-by: Alex Jia&lt;ajia(a)redhat.com&gt; > --- > src/conf/storage_conf.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c > index 0b34f28..668e679 100644 > --- a/src/conf/storage_conf.c > +++ b/src/conf/storage_conf.c > @@ -462,6 +462,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt, > goto cleanup; > } > } > + VIR_FREE(nodeset); > } > } > The problem with the added line is not visible with this context, so I'm adding some more: source->nhost = virXPathNodeSet("./host", ctxt,&nodeset); if (source->nhost) { if (VIR_ALLOC_N(source->hosts, source->nhost)< 0) { virReportOOMError(); goto cleanup; } for (i = 0 ; i< source->nhost ; i++) { name = virXMLPropString(nodeset[i], "name"); if (name == NULL) { virStorageReportError(VIR_ERR_XML_ERROR, "%s", _("missing storage pool host name")); goto cleanup; } source->hosts[i].name = name; port = virXMLPropString(nodeset[i], "port"); if (port) { if (virStrToLong_i(port, NULL, 10,&source->hosts[i].port)< 0) { virStorageReportError(VIR_ERR_XML_ERROR, _("Invalid port number: %s"), port); goto cleanup; } } + VIR_FREE(nodeset); } } You added the VIR_FREE inside the for loop, so it gets freed before the next iteration and might cause a segfault in the second iteration.

NACK Peter