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

On Mon, Aug 23, 2010 at 12:25:19AM +0200, Matthias Bolte wrote: > This allows to list existing volumes and to retrieve information > about them. > --- >  src/esx/esx_driver.c           |  112 +---------- >  src/esx/esx_storage_driver.c   |  433 +++++++++++++++++++++++++++++++++++++++- >  src/esx/esx_vi.c               |  279 ++++++++++++++++++++++++++ >  src/esx/esx_vi.h               |    9 + >  src/esx/esx_vi_generator.input |    7 + >  5 files changed, 724 insertions(+), 116 deletions(-) > +static char * > +esxStorageVolumeDumpXML(virStorageVolPtr volume, unsigned int flags) > +{ > +    esxPrivate *priv = volume->conn->storagePrivateData; > +    esxVI_String *propertyNameList = NULL; > +    esxVI_ObjectContent *datastore = NULL; > +    esxVI_DynamicProperty *dynamicProperty = NULL; > +    esxVI_DatastoreInfo *datastoreInfo = NULL; > +    virStoragePoolDef pool; > +    char *datastorePath = NULL; > +    esxVI_FileInfo *fileInfo = NULL; > +    esxVI_VmDiskFileInfo *vmDiskFileInfo = NULL; > +    esxVI_IsoImageFileInfo *isoImageFileInfo = NULL; > +    esxVI_FloppyImageFileInfo *floppyImageFileInfo = NULL; > +    virStorageVolDef def; > +    char *xml = NULL; > + > +    virCheckFlags(0, NULL); > + > +    memset(&pool, 0, sizeof (pool)); > +    memset(&def, 0, sizeof (def)); > + > +    if (esxVI_EnsureSession(priv->primary) < 0) { > +        return NULL; > +    } > + > +    /* Lookup storage pool type */ > +    if (esxVI_String_AppendValueToList(&propertyNameList, "info") < 0 || > +        esxVI_LookupDatastoreByName(priv->primary, volume->pool, > +                                    propertyNameList, &datastore, > +                                    esxVI_Occurrence_RequiredItem) < 0) { > +        goto cleanup; > +    } > + > +    for (dynamicProperty = datastore->propSet; dynamicProperty != NULL; > +         dynamicProperty = dynamicProperty->_next) { > +        if (STREQ(dynamicProperty->name, "info")) { > +            if (esxVI_DatastoreInfo_CastFromAnyType(dynamicProperty->val, > +                                                    &datastoreInfo) < 0) { > +                goto cleanup; > +            } > + > +            break; > +        } > +    } > + > +    if (esxVI_LocalDatastoreInfo_DynamicCast(datastoreInfo) != NULL) { > +        pool.type = VIR_STORAGE_POOL_DIR; > +    } else if (esxVI_NasDatastoreInfo_DynamicCast(datastoreInfo) != NULL) { > +        pool.type = VIR_STORAGE_POOL_NETFS; > +    } else if (esxVI_VmfsDatastoreInfo_DynamicCast(datastoreInfo) != NULL) { > +        pool.type = VIR_STORAGE_POOL_FS; > +    } else { > +        ESX_ERROR(VIR_ERR_INTERNAL_ERROR, "%s", > +                  _("DatastoreInfo has unexpected type")); > +        goto cleanup; > +    } > + > +    /* Lookup file info */ > +    if (virAsprintf(&datastorePath, "[%s] %s", volume->pool, volume->name) < 0) { > +        virReportOOMError(); > +        goto cleanup; > +    } > + > +    if (esxVI_LookupFileInfoByDatastorePath(priv->primary, datastorePath, > +                                            &fileInfo, > +                                            esxVI_Occurrence_RequiredItem) < 0) { > +        goto cleanup; > +    } > + > +    vmDiskFileInfo = esxVI_VmDiskFileInfo_DynamicCast(fileInfo); > +    isoImageFileInfo = esxVI_IsoImageFileInfo_DynamicCast(fileInfo); > +    floppyImageFileInfo = esxVI_FloppyImageFileInfo_DynamicCast(fileInfo); > + > +    def.name = volume->name; > +    def.key = datastorePath; I know that the main libvirt storage driver uses a path for 'key' currently, but if you have a choice with ESX, it would be desirable to use a 'better' unique identifier for key. The idea is that 'key' is trying to provide a unique identifier that is stable even if the volume is moved or renamed, but still points at the same underlying data. A path is fine as the catchall fallback case, but if there's a a UUID, or SCSI/iSCSI LUN WWID then that's better.

ACK, to the patch anyway since we can change this later if desired.