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

[libvirt] [PATCH 3/5] storage_conf: Various fixes or improvements on pool def parsing



virStorageDefParsePerms:
  * Use uid_t/gid_t to do casting

virStoragePoolDefParseSource:
  * Improve the error message for source "name" parsing
  * Remove the useless casting (const char *)

virStoragePoolDefParseAuthChap:
  * Fix the wrong error message

virStoragePoolDefParseAuthCephx:
  * Don't leak "uuid"

virStoragePoolDefParseXML:
  * Remove the useless casting "(const char *)"
  * Use VIR_ERR_XML_ERROR for pool 'type' parsing
  * Remove the xmlFree()
  * s/tmppath/path/,
  * Create "virStoragePoolDefPtr def", and use ret to track the
    return value; frees the strings at "cleanup" label instead
    if freeing them in the middle.
  * Other small changes.
---
 src/conf/storage_conf.c | 169 ++++++++++++++++++++++++------------------------
 1 file changed, 85 insertions(+), 84 deletions(-)

diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index 8fa805b..dd55d2c 100644
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -446,15 +446,15 @@ virStoragePoolDefParseAuthChap(xmlXPathContextPtr ctxt,
 {
     auth->login = virXPathString("string(./auth/@login)", ctxt);
     if (auth->login == NULL) {
-        virReportError(VIR_ERR_XML_ERROR,
-                       "%s", _("missing auth host attribute"));
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("missing auth login attribute"));
         return -1;
     }
 
     auth->passwd = virXPathString("string(./auth/@passwd)", ctxt);
     if (auth->passwd == NULL) {
-        virReportError(VIR_ERR_XML_ERROR,
-                       "%s", _("missing auth passwd attribute"));
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("missing auth passwd attribute"));
         return -1;
     }
 
@@ -466,10 +466,12 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt,
                                 virStoragePoolAuthCephxPtr auth)
 {
     char *uuid = NULL;
+    int ret = -1;
+
     auth->username = virXPathString("string(./auth/@username)", ctxt);
     if (auth->username == NULL) {
-        virReportError(VIR_ERR_XML_ERROR,
-                       "%s", _("missing auth username attribute"));
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("missing auth username attribute"));
         return -1;
     }
 
@@ -485,19 +487,22 @@ virStoragePoolDefParseAuthCephx(xmlXPathContextPtr ctxt,
         if (auth->secret.usage != NULL) {
             virReportError(VIR_ERR_XML_ERROR, "%s",
                            _("either auth secret uuid or usage expected"));
-            return -1;
+            goto cleanup;
         }
         if (virUUIDParse(uuid, auth->secret.uuid) < 0) {
-            virReportError(VIR_ERR_XML_ERROR,
-                           "%s", _("invalid auth secret uuid"));
-            return -1;
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("invalid auth secret uuid"));
+            goto cleanup;
         }
         auth->secret.uuidUsable = true;
     } else {
         auth->secret.uuidUsable = false;
     }
 
-    return 0;
+    ret = 0;
+cleanup:
+    VIR_FREE(uuid);
+    return ret;
 }
 
 static int
@@ -526,7 +531,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
     source->name = virXPathString("string(./name)", ctxt);
     if (pool_type == VIR_STORAGE_POOL_RBD && source->name == NULL) {
         virReportError(VIR_ERR_XML_ERROR, "%s",
-                       _("missing mandatory 'name' field for RBD pool name"));
+                       _("element 'name' is mandatory for RBD pool name"));
         goto cleanup;
     }
 
@@ -559,8 +564,8 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
         for (i = 0; i < source->nhost; i++) {
             name = virXMLPropString(nodeset[i], "name");
             if (name == NULL) {
-                virReportError(VIR_ERR_XML_ERROR,
-                               "%s", _("missing storage pool host name"));
+                virReportError(VIR_ERR_XML_ERROR, "%s",
+                               _("missing storage pool host name"));
                 goto cleanup;
             }
             source->hosts[i].name = name;
@@ -595,8 +600,8 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
             char *path = virXMLPropString(nodeset[i], "path");
             if (path == NULL) {
                 VIR_FREE(nodeset);
-                virReportError(VIR_ERR_XML_ERROR,
-                               "%s", _("missing storage pool source device path"));
+                virReportError(VIR_ERR_XML_ERROR, "%s",
+                               _("missing storage pool source device path"));
                 goto cleanup;
             }
             source->devices[i].path = path;
@@ -667,7 +672,7 @@ virStoragePoolDefParseSource(xmlXPathContextPtr ctxt,
         } else {
             virReportError(VIR_ERR_XML_ERROR,
                            _("unknown auth type '%s'"),
-                           (const char *)authType);
+                           authType);
             goto cleanup;
         }
     }
@@ -768,8 +773,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
 
         if (virStrToLong_i(mode, NULL, 8, &tmp) < 0 || (tmp & ~0777)) {
             VIR_FREE(mode);
-            virReportError(VIR_ERR_XML_ERROR,
-                           "%s", _("malformed octal mode"));
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("malformed octal mode"));
             goto error;
         }
         perms->mode = tmp;
@@ -780,22 +785,22 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
         perms->uid = (uid_t) -1;
     } else {
         if (virXPathLong("number(./owner)", ctxt, &v) < 0) {
-            virReportError(VIR_ERR_XML_ERROR,
-                           "%s", _("malformed owner element"));
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("malformed owner element"));
             goto error;
         }
-        perms->uid = (int)v;
+        perms->uid = (uid_t)v;
     }
 
     if (virXPathNode("./group", ctxt) == NULL) {
         perms->gid = (gid_t) -1;
     } else {
         if (virXPathLong("number(./group)", ctxt, &v) < 0) {
-            virReportError(VIR_ERR_XML_ERROR,
-                           "%s", _("malformed group element"));
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("malformed group element"));
             goto error;
         }
-        perms->gid = (int)v;
+        perms->gid = (gid_t)v;
     }
 
     /* NB, we're ignoring missing labels here - they'll simply inherit */
@@ -811,85 +816,81 @@ static virStoragePoolDefPtr
 virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
 {
     virStoragePoolOptionsPtr options;
-    virStoragePoolDefPtr ret;
+    virStoragePoolDefPtr ret = NULL;
+    virStoragePoolDefPtr def;
     xmlNodePtr source_node;
     char *type = NULL;
     char *uuid = NULL;
-    char *tmppath;
+    char *target_path = NULL;
 
-    if (VIR_ALLOC(ret) < 0) {
+    if (VIR_ALLOC(def) < 0) {
         virReportOOMError();
         return NULL;
     }
 
     type = virXPathString("string(./@type)", ctxt);
-    if ((ret->type = virStoragePoolTypeFromString((const char *)type)) < 0) {
-        virReportError(VIR_ERR_INTERNAL_ERROR,
-                       _("unknown storage pool type %s"), (const char*)type);
+    if ((def->type = virStoragePoolTypeFromString(type)) < 0) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       _("unknown storage pool type %s"), type);
         goto cleanup;
     }
 
-    xmlFree(type);
-    type = NULL;
-
-    if ((options = virStoragePoolOptionsForPoolType(ret->type)) == NULL) {
+    if ((options = virStoragePoolOptionsForPoolType(def->type)) == NULL)
         goto cleanup;
-    }
 
     source_node = virXPathNode("./source", ctxt);
     if (source_node) {
-        if (virStoragePoolDefParseSource(ctxt, &ret->source, ret->type,
+        if (virStoragePoolDefParseSource(ctxt, &def->source, def->type,
                                          source_node) < 0)
             goto cleanup;
     }
 
-    ret->name = virXPathString("string(./name)", ctxt);
-    if (ret->name == NULL &&
+    def->name = virXPathString("string(./name)", ctxt);
+    if (def->name == NULL &&
         options->flags & VIR_STORAGE_POOL_SOURCE_NAME)
-        ret->name = ret->source.name;
-    if (ret->name == NULL) {
-        virReportError(VIR_ERR_XML_ERROR,
-                       "%s", _("missing pool source name element"));
+        def->name = def->source.name;
+    if (def->name == NULL) {
+        virReportError(VIR_ERR_XML_ERROR, "%s",
+                       _("missing pool source name element"));
         goto cleanup;
     }
 
     uuid = virXPathString("string(./uuid)", ctxt);
     if (uuid == NULL) {
-        if (virUUIDGenerate(ret->uuid) < 0) {
-            virReportError(VIR_ERR_INTERNAL_ERROR,
-                           "%s", _("unable to generate uuid"));
+        if (virUUIDGenerate(def->uuid) < 0) {
+            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
+                           _("unable to generate uuid"));
             goto cleanup;
         }
     } else {
-        if (virUUIDParse(uuid, ret->uuid) < 0) {
-            virReportError(VIR_ERR_XML_ERROR,
-                           "%s", _("malformed uuid element"));
+        if (virUUIDParse(uuid, def->uuid) < 0) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("malformed uuid element"));
             goto cleanup;
         }
-        VIR_FREE(uuid);
     }
 
     if (options->flags & VIR_STORAGE_POOL_SOURCE_HOST) {
-        if (!ret->source.nhost) {
-            virReportError(VIR_ERR_XML_ERROR,
-                           "%s",
+        if (!def->source.nhost) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
                            _("missing storage pool source host name"));
             goto cleanup;
         }
     }
 
     if (options->flags & VIR_STORAGE_POOL_SOURCE_DIR) {
-        if (!ret->source.dir) {
-            virReportError(VIR_ERR_XML_ERROR,
-                           "%s", _("missing storage pool source path"));
+        if (!def->source.dir) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("missing storage pool source path"));
             goto cleanup;
         }
     }
+
     if (options->flags & VIR_STORAGE_POOL_SOURCE_NAME) {
-        if (ret->source.name == NULL) {
+        if (def->source.name == NULL) {
             /* source name defaults to pool name */
-            ret->source.name = strdup(ret->name);
-            if (ret->source.name == NULL) {
+            def->source.name = strdup(def->name);
+            if (def->source.name == NULL) {
                 virReportOOMError();
                 goto cleanup;
             }
@@ -897,30 +898,30 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
     }
 
     if (options->flags & VIR_STORAGE_POOL_SOURCE_ADAPTER) {
-        if (!ret->source.adapter.type) {
+        if (!def->source.adapter.type) {
             virReportError(VIR_ERR_XML_ERROR, "%s",
                            _("missing storage pool source adapter"));
             goto cleanup;
         }
 
-        if (ret->source.adapter.type ==
+        if (def->source.adapter.type ==
             VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_FC_HOST) {
-            if (!ret->source.adapter.data.fchost.wwnn ||
-                !ret->source.adapter.data.fchost.wwpn) {
+            if (!def->source.adapter.data.fchost.wwnn ||
+                !def->source.adapter.data.fchost.wwpn) {
                 virReportError(VIR_ERR_XML_ERROR, "%s",
                                _("'wwnn' and 'wwpn' must be specified for adapter "
                                  "type 'fchost'"));
                 goto cleanup;
             }
 
-            if (!virValidateWWN(ret->source.adapter.data.fchost.wwnn) ||
-                !virValidateWWN(ret->source.adapter.data.fchost.wwpn))
+            if (!virValidateWWN(def->source.adapter.data.fchost.wwnn) ||
+                !virValidateWWN(def->source.adapter.data.fchost.wwpn))
                 goto cleanup;
-        } else if (ret->source.adapter.type ==
+        } else if (def->source.adapter.type ==
                    VIR_STORAGE_POOL_SOURCE_ADAPTER_TYPE_SCSI_HOST) {
-            if (!ret->source.adapter.data.name) {
-                virReportError(VIR_ERR_XML_ERROR,
-                               "%s", _("missing storage pool source adapter name"));
+            if (!def->source.adapter.data.name) {
+                virReportError(VIR_ERR_XML_ERROR, "%s",
+                               _("missing storage pool source adapter name"));
                 goto cleanup;
             }
         }
@@ -928,9 +929,9 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
 
     /* If DEVICE is the only source type, then its required */
     if (options->flags == VIR_STORAGE_POOL_SOURCE_DEVICE) {
-        if (!ret->source.ndevice) {
-            virReportError(VIR_ERR_XML_ERROR,
-                           "%s", _("missing storage pool source device name"));
+        if (!def->source.ndevice) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("missing storage pool source device name"));
             goto cleanup;
         }
     }
@@ -938,29 +939,29 @@ virStoragePoolDefParseXML(xmlXPathContextPtr ctxt)
     /* 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) {
-            virReportError(VIR_ERR_XML_ERROR,
-                           "%s", _("missing storage pool target path"));
+        if (!(target_path = virXPathString("string(./target/path)", ctxt))) {
+            virReportError(VIR_ERR_XML_ERROR, "%s",
+                           _("missing storage pool target path"));
             goto cleanup;
         }
-        ret->target.path = virFileSanitizePath(tmppath);
-        VIR_FREE(tmppath);
-        if (!ret->target.path)
+        def->target.path = virFileSanitizePath(target_path);
+        if (!def->target.path)
             goto cleanup;
 
-        if (virStorageDefParsePerms(ctxt, &ret->target.perms,
+        if (virStorageDefParsePerms(ctxt, &def->target.perms,
                                     "./target/permissions",
                                     DEFAULT_POOL_PERM_MODE) < 0)
             goto cleanup;
     }
 
-    return ret;
-
+    ret = def;
 cleanup:
     VIR_FREE(uuid);
-    xmlFree(type);
-    virStoragePoolDefFree(ret);
-    return NULL;
+    VIR_FREE(type);
+    VIR_FREE(target_path);
+    if (!ret)
+        virStoragePoolDefFree(ret);
+    return ret;
 }
 
 virStoragePoolDefPtr
-- 
1.8.1.4


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