Re: [libvirt] [PATCH] esx: Add read-only storage pool access

10 May 2010


      On 05/07/2010 02:02 PM, Matthias Bolte wrote:
...
Add VIR_STORAGE_POOL_INACCESSIBLE to denote a running but inaccessible
storage pool. For example an NFS pool is inaccessible when the NFS
server is currently unreachable.
Add CIFS to the list of network file systems because ESX distinguishes
between NFS and CIFS.
Alter the esxVI_ProductVersion enum in a way that allows to check for
product type by masking.
Make esxVI_*_CastFromAnyType dynamically dispatched in order to handle
the DatastoreInfo type and inheriting types properly.
Allow esxVI_X_DynamicCast to be called successfully on objects with
type X. This is necessary for handling DatastoreInfo and inheriting
types properly.
This seems like a lot of things; can any of it be split into smaller
patches (perhaps one patch for dynamic dispatch, and another patch for
plugging in ESX storage pool handling)?
...
+static int
+esxNumberOfStoragePools(virConnectPtr conn)
+{
+    int result = 0;
+    esxPrivate *priv = conn->storagePrivateData;
+    esxVI_ObjectContent *datastoreList = NULL;
+    esxVI_ObjectContent *datastore = NULL;
+
+    if (esxVI_EnsureSession(priv->host) < 0) {
+        goto failure;
+    }
+
+    if (esxVI_LookupObjectContentByType(priv->host, priv->host->datacenter,
+                                        "Datastore", NULL, esxVI_Boolean_True,
+                                        &datastoreList) < 0) {
+        goto failure;
+    }
+
+    for (datastore = datastoreList; datastore != NULL;
+         datastore = datastore->_next) {
+        ++result;
+    }
+
+  cleanup:
+    esxVI_ObjectContent_Free(&datastoreList);
+
+    return result;
+
+  failure:
+    result = -1;
+
+    goto cleanup;
That logic looks bad, with a goto jumping backwards.  Since there is no
other goto cleanup, you might as well inline the result=-1 into the
failure paths, and have a single label.  Or better yet, since
datastoreList is NULL unless esxVI_LooupObjectContentByType succeeded,
you can write this function without any gotos:

if esxVI_EnsureSession(priv->host) < 0) {
    return -1;
}
...
...
+
+static int
+esxListStoragePools(virConnectPtr conn, char **const names, int maxnames)
+{
+    esxPrivate *priv = conn->storagePrivateData;
+    esxVI_String *propertyNameList = NULL;
+    esxVI_DynamicProperty *dynamicProperty = NULL;
+    esxVI_ObjectContent *datastoreList = NULL;
+    esxVI_ObjectContent *datastore = NULL;
+    int count = 0;
+    int i;
+
+    if (names == NULL || maxnames < 0) {
+        ESX_ERROR(VIR_ERR_INVALID_ARG, "%s", _("Invalid argument"));
+        return -1;
+    }
+
+    if (maxnames == 0) {
+        return 0;
+    }
+
+    if (esxVI_EnsureSession(priv->host) < 0) {
+        goto failure;
+    }
+
+    if (esxVI_String_AppendValueToList(&propertyNameList,
+                                       "summary.name") < 0 ||
+        esxVI_LookupObjectContentByType(priv->host, priv->host->datacenter,
+                                        "Datastore", propertyNameList,
+                                        esxVI_Boolean_True,
+                                        &datastoreList) < 0) {
+        goto failure;
+    }
+
+    for (datastore = datastoreList; datastore != NULL;
+         datastore = datastore->_next) {
+        for (dynamicProperty = datastore->propSet; dynamicProperty != NULL;
+             dynamicProperty = dynamicProperty->_next) {
+            if (STREQ(dynamicProperty->name, "summary.name")) {
+                if (esxVI_AnyType_ExpectType(dynamicProperty->val,
+                                             esxVI_Type_String) < 0) {
+                    goto failure;
+                }
+
+                names[count] = strdup(dynamicProperty->val->string);
+
+                if (names[count] == NULL) {
+                    virReportOOMError();
+                    goto failure;
+                }
+
+                ++count;
+                break;
+            } else {
+                VIR_WARN("Unexpected '%s' property", dynamicProperty->name);
+            }
+        }
+    }
+
+  cleanup:
+    esxVI_String_Free(&propertyNameList);
+    esxVI_ObjectContent_Free(&datastoreList);
+
+    return count;
+
+  failure:
+    for (i = 0; i < count; ++i) {
+        VIR_FREE(names[i]);
+    }
+
+    count = -1;
+
+    goto cleanup;
Here, I think you need to keep one label, but think it can be written
with one instead of two, by tracking two variables instead of one:

{
    int count = 0;
    bool success = false;

    if (something fails)
        goto cleanup;
...

    success = true;

cleanup:
    if (!success) {
        for (i = 0; i < count; ++i)
            VIR_FREE(names[i]);
        count = -1;
    }
    esxVI_String_Free(&propertyNameList);
    esxVI_ObjectContent_Free(&datastoreList);

    return count;
}
...
+
+static int
+esxStoragePoolRefresh(virStoragePoolPtr pool,
+                      unsigned int flags ATTRIBUTE_UNUSED)
+{
+    int result = 0;
+    esxPrivate *priv = pool->conn->storagePrivateData;
+    esxVI_ObjectContent *datastore = NULL;
Is it worth a virCheckFlags() here?
...
+
+    if (esxVI_EnsureSession(priv->host) < 0) {
+        goto failure;
+    }
+
+    if (esxVI_LookupDatastoreByName(priv->host, pool->name, NULL, &datastore,
+                                    esxVI_Occurrence_RequiredItem) < 0 ||
+        esxVI_RefreshDatastore(priv->host, datastore->obj) < 0) {
+        goto failure;
+    }
+
+  cleanup:
+    esxVI_ObjectContent_Free(&datastore);
+
+    return result;
+
+  failure:
+    result = -1;
+
+    goto cleanup;
Another one of those reverse gotos that could be cleaned up.
...
+
+
+static char *
+esxStoragePoolGetXMLDesc(virStoragePoolPtr pool, unsigned int flags)
+{
...
+        /* See vSphere API documentation about HostDatastoreSystem for details */
+        if ((localInfo = esxVI_LocalDatastoreInfo_DynamicCast(info)) != NULL) {
+            def.type = VIR_STORAGE_POOL_DIR;
+            def.target.path = localInfo->path;
+        } else if ((nasInfo = esxVI_NasDatastoreInfo_DynamicCast(info)) != NULL) {
+            def.type = VIR_STORAGE_POOL_NETFS;
+            def.source.host.name = nasInfo->nas->remoteHost;
+            def.source.dir = nasInfo->nas->remotePath;
+
+            if (STRCASEEQ(nasInfo->nas->type, "NFS")) {
+                def.source.format = VIR_STORAGE_POOL_NETFS_NFS;
+            } else  if (STRCASEEQ(nasInfo->nas->type, "CIFS")) {
+                def.source.format = VIR_STORAGE_POOL_NETFS_CIFS;
+            } else {
+                ESX_ERROR(VIR_ERR_INTERNAL_ERROR,
+                          _("Datastore has unexpected type '%s'"),
+                          nasInfo->nas->type);
+                goto failure;
+            }
+        } else if ((vmfsInfo = esxVI_VmfsDatastoreInfo_DynamicCast(info)) != NULL) {
+            def.type = VIR_STORAGE_POOL_FS;
+            /* FIXME */
Details on what needs fixing?
...
+++ b/src/esx/esx_vi.c
@@ -1530,6 +1530,114 @@ esxVI_GetVirtualMachineQuestionInfo
@@ -2161,7 +2269,7 @@ esxVI_LookupDatastoreByName(esxVI_Context *ctx, const char *name,
     esxVI_ObjectContent *candidate = NULL;
     esxVI_DynamicProperty *dynamicProperty = NULL;
     esxVI_Boolean accessible = esxVI_Boolean_Undefined;
-    size_t offset = strlen("/vmfs/volumes/");
+    int offset = 14; /* = strlen("/vmfs/volumes/") */
Why the change from size_t to int?  Although it probably doesn't affect
code.
...
+++ b/src/esx/esx_vi.h
@@ -98,11 +98,14 @@ enum _esxVI_APIVersion {
enum _esxVI_ProductVersion {
     esxVI_ProductVersion_Undefined = 0,
-    esxVI_ProductVersion_GSX20,
-    esxVI_ProductVersion_ESX35,
-    esxVI_ProductVersion_ESX40,
-    esxVI_ProductVersion_VPX25,
-    esxVI_ProductVersion_VPX40
+    esxVI_ProductVersion_GSX   = 0x1000,
+    esxVI_ProductVersion_GSX20 = 0x1001,
+    esxVI_ProductVersion_ESX   = 0x2000,
+    esxVI_ProductVersion_ESX35 = 0x2001,
+    esxVI_ProductVersion_ESX40 = 0x2002,
+    esxVI_ProductVersion_VPX   = 0x4000,
+    esxVI_ProductVersion_VPX25 = 0x4001,
+    esxVI_ProductVersion_VPX40 = 0x4002
Is it worth documenting the convention you used here, in case someone
has to add to the enum later on?

-- 
Eric Blake   eblake@redhat.com    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

    