Re: [libvirt] [PATCH 07/12] storage_conf: Improve the memory deallocation of pool def parsing

[Ján Tomko]

On 05/22/2013 02:05 PM, Osier Yang wrote:
 Changes:
 * Free all the strings at cleanup, instead of freeing them
   in the middle
 * Remove xmlFree
 * s/tmppath/target_path/, to make it more sensible
 * Add new goto label error
 ---
  src/conf/storage_conf.c | 54 
 -
  1 file changed, 26 insertions(+), 28 deletions(-)
 
 diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
 index efe02e8..6f0ed74 100644
 --- a/src/conf/storage_conf.c
 +++ b/src/conf/storage_conf.c

 @@ -914,7 +910,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
  virReportError(VIR_ERR_XML_ERROR, %s,
 _('wwnn' and 'wwpn' must be specified for 
 adapter 
   type 'fchost'));
 -goto cleanup;
 +goto error;
  }
  
  if (!virValidateWWN(ret-source.adapter.data.fchost.wwnn) ||

If the WWN validation fails here, you still jump to cleanup instead of error.

ACK if you fix that.

Jan

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[libvirt] [PATCH 07/12] storage_conf: Improve the memory deallocation of pool def parsing

[Osier Yang]

Changes:
* Free all the strings at cleanup, instead of freeing them
  in the middle
* Remove xmlFree
* s/tmppath/target_path/, to make it more sensible
* Add new goto label error
---
 src/conf/storage_conf.c | 54 -
 1 file changed, 26 insertions(+), 28 deletions(-)

diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index efe02e8..6f0ed74 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -820,7 +820,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
 xmlNodePtr source_node;
 char *type = NULL;
 char *uuid = NULL;
-char *tmppath;
+char *target_path = NULL;
 
 if (VIR_ALLOC(ret)  0) {
 virReportOOMError();
@@ -831,21 +831,18 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
 if ((ret-type = virStoragePoolTypeFromString(type))  0) {
 virReportError(VIR_ERR_XML_ERROR,
_(unknown storage pool type %s), type);
-goto cleanup;
+goto error;
 }
 
-xmlFree(type);
-type = NULL;
-
 if ((options = virStoragePoolOptionsForPoolType(ret-type)) == NULL) {
-goto cleanup;
+goto error;
 }
 
 source_node = virXPathNode(./source, ctxt);
 if (source_node) {
 if (virStoragePoolDefParseSource(ctxt, ret-source, ret-type,
  source_node)  0)
-goto cleanup;
+goto error;
 }
 
 ret-name = virXPathString(string(./name), ctxt);
@@ -855,7 +852,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
 if (ret-name == NULL) {
 virReportError(VIR_ERR_XML_ERROR, %s,
_(missing pool source name element));
-goto cleanup;
+goto error;
 }
 
 uuid = virXPathString(string(./uuid), ctxt);
@@ -863,22 +860,21 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
 if (virUUIDGenerate(ret-uuid)  0) {
 virReportError(VIR_ERR_INTERNAL_ERROR, %s,
_(unable to generate uuid));
-goto cleanup;
+goto error;
 }
 } else {
 if (virUUIDParse(uuid, ret-uuid)  0) {
 virReportError(VIR_ERR_XML_ERROR, %s,
_(malformed uuid element));
-goto cleanup;
+goto error;
 }
-VIR_FREE(uuid);
 }
 
 if (options-flags  VIR_STORAGE_POOL_SOURCE_HOST) {
 if (!ret-source.nhost) {
 virReportError(VIR_ERR_XML_ERROR, %s,
_(missing storage pool source host name));
-goto cleanup;
+goto error;
 }
 }
 
@@ -886,7 +882,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
 if (!ret-source.dir) {
 virReportError(VIR_ERR_XML_ERROR, %s,
_(missing storage pool source path));
-goto cleanup;
+goto error;
 }
 }
 if (options-flags  VIR_STORAGE_POOL_SOURCE_NAME) {
@@ -895,7 +891,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
 ret-source.name = strdup(ret-name);
 if (ret-source.name == NULL) {
 virReportOOMError();
-goto cleanup;
+goto error;
 }
 }
 }
@@ -904,7 +900,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
 if (!ret-source.adapter.type) {
 virReportError(VIR_ERR_XML_ERROR, %s,
_(missing storage pool source adapter));
-goto cleanup;
+goto error;
 }
 
 if (ret-source.adapter.type ==
@@ -914,7 +910,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
 virReportError(VIR_ERR_XML_ERROR, %s,
_('wwnn' and 'wwpn' must be specified for 
adapter 
  type 'fchost'));
-goto cleanup;
+goto error;
 }
 
 if (!virValidateWWN(ret-source.adapter.data.fchost.wwnn) ||
@@ -925,7 +921,7 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
 if (!ret-source.adapter.data.name) {
 virReportError(VIR_ERR_XML_ERROR, %s,
_(missing storage pool source adapter name));
-goto cleanup;
+goto error;
 }
 }
 }
@@ -935,36 +931,38 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
 if (!ret-source.ndevice) {
 virReportError(VIR_ERR_XML_ERROR, %s,
_(missing storage pool source device name));
-goto cleanup;
+goto error;
 }
 }
 
 /* When we are working with a virtual disk we can skip the target
  * path and permissions */
 if (!(options-flags  VIR_STORAGE_POOL_SOURCE_NETWORK)) {
-if ((tmppath = virXPathString(string(./target/path), ctxt)) == NULL) 
{
+if