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@xxxxxxxxxx +1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list