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

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



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


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