[libvirt] [PATCH v2 8/9] pvs: add storage driver

[Stefan Berger]

On 04/18/2012 02:07 PM, Dmitry Guryanov wrote:
> PVS has one serious discrepancy with libvirt: libvirt stores
> domain configuration files always in one place, and storage files
> in other places (with API of storage pools and storage volumes).
> PVS store all domain data in a single directory, for example, you
> may have domain with name fedora-15, which will be located in
> '/var/parallels/fedora-15.pvm', and it's hard disk image will be
> in '/var/parallels/fedora-15.pvm/harddisk1.hdd'.
>
> I've decided to create storage driver, which produces pseudo-volumes
> (xml files with volume description), and they will be 'converted' to
> real disk images after attaching to a VM.
>
> So if someone creates VM with one hard disk using virt-manager,
> at first virt-manager creates a new volume, and then defines a
> domain. We can lookup a volume by path in XML domain definition
> and find out location of new domain and size of its hard disk.
>
> This code mostly duplicates code in libvirt's default storage
> driver, but I haven't found, how functions from that driver can
> be reused. So if it possible I'll be very grateful for the advice,
> how to do it.
>

[...]

> +
> +static int
> +pvsFindVolumes(virStoragePoolObjPtr pool)
> +{
> +    int ret;
> +    DIR *dir;
> +    struct dirent *ent;
> +    char *path;
> +
> +    if (!(dir = opendir(pool->def->target.path))) {
> +        virReportSystemError(errno,
> +                             _("cannot open path '%s'"),
> +                             pool->def->target.path);
> +        goto cleanup;

Need to set ret = -1.

> +    }
> +
> +    while ((ent = readdir(dir)) != NULL) {
> +        if (!virFileHasSuffix(ent->d_name, ".xml"))
> +            continue;
> +
> +        if (!(path = virFileBuildPath(pool->def->target.path,
> +                                      ent->d_name, NULL)))
> +            goto no_memory;
> +        if (!pvsStorageVolumeDefine(pool, NULL, path, false))
> +            goto cleanup;
> +        VIR_FREE(path);
> +    }
> +

Nothing assigns a value to ret...

> +    return ret;
> +  no_memory:
> +    virReportOOMError();
> +  cleanup:
> +    return ret;
> +
> +}
> +
> +static virDrvOpenStatus
> +pvsStorageOpen(virConnectPtr conn,
> +               virConnectAuthPtr auth ATTRIBUTE_UNUSED, unsigned int flags)
> +{
> +    char *base = NULL;
> +    virStorageDriverStatePtr storageState;
> +    int privileged = (geteuid() == 0);
> +    pvsConnPtr privconn = conn->privateData;
> +    virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR);
> +
> +    if (STRNEQ(conn->driver->name, "PVS"))
> +        return VIR_DRV_OPEN_DECLINED;
> +
> +    if (VIR_ALLOC(storageState)<  0)
> +        return VIR_DRV_OPEN_ERROR;

virReportOOMError() ?


> +
> +    if (virMutexInit(&storageState->lock)<  0) {
> +        VIR_FREE(storageState);
> +        return VIR_DRV_OPEN_ERROR;
> +    }
> +    pvsStorageLock(storageState);
> +
> +    if (privileged) {
> +        if ((base = strdup(SYSCONFDIR "/libvirt")) == NULL)
> +            goto out_of_memory;
> +    } else {
> +        uid_t uid = geteuid();
> +
> +        char *userdir = virGetUserDirectory(uid);
> +
> +        if (!userdir)
> +            goto error;
> +
> +        if (virAsprintf(&base, "%s/.libvirt", userdir) == -1) {
> +            VIR_FREE(userdir);
> +            goto out_of_memory;
> +        }
> +        VIR_FREE(userdir);
> +    }
> +
> +    /* Configuration paths are either ~/.libvirt/storage/... (session) or
> +     * /etc/libvirt/storage/... (system).
> +     */
> +    if (virAsprintf(&storageState->configDir,
> +                    "%s/pvs-storage", base) == -1)
> +        goto out_of_memory;
> +
> +    if (virAsprintf(&storageState->autostartDir,
> +                    "%s/pvs-storage/autostart", base) == -1)
> +        goto out_of_memory;
> +
> +    VIR_FREE(base);
> +
> +    if (virStoragePoolLoadAllConfigs(&privconn->pools,
> +                                     storageState->configDir,
> +                                     storageState->autostartDir)<  0) {
> +        pvsError(VIR_ERR_INTERNAL_ERROR, _("Failed to load pool configs"));
> +        goto error;
> +    }
> +
> +    for (int i = 0; i<  privconn->pools.count; i++) {
> +        virStoragePoolObjLock(privconn->pools.objs[i]);
> +        virStoragePoolObjPtr pool;
> +
> +        pool = privconn->pools.objs[i];
> +        if (pool->autostart)
> +            pool->active = 1;
> +        pool->active = 1;

pool is always active ? test before seems redundant

> +
> +		if (pvsStoragePoolGetAlloc(pool->def))

< 0 check

> +			goto error;
> +
> +        if (pvsFindVolumes(pool))

< 0 check ?

> +            goto error;
> +
> +        virStoragePoolObjUnlock(privconn->pools.objs[i]);
> +    }
> +
> +    pvsStorageUnlock(storageState);
> +
> +    conn->storagePrivateData = storageState;
> +
> +    return VIR_DRV_OPEN_SUCCESS;
> +
> +  out_of_memory:
> +    virReportOOMError();
> +  error:
> +    VIR_FREE(base);
> +    pvsStorageUnlock(storageState);
> +    pvsStorageClose(conn);
> +    return -1;
> +}
> +
> +static int
> +pvsStorageClose(virConnectPtr conn)
> +{
> +    pvsConnPtr privconn = conn->privateData;
> +    virStorageDriverStatePtr storageState = conn->storagePrivateData;

add empty line here after var decl.

> +    conn->storagePrivateData = NULL;
> +
> +    pvsStorageLock(storageState);
> +    virStoragePoolObjListFree(&privconn->pools);+
> +/*
> + * Fill capacity, available and allocation
> + * fields in pool definition.
> + */
> +static int
> +pvsStoragePoolGetAlloc(virStoragePoolDefPtr def)
> +{
> +	struct statvfs sb;
> +

Different spacing here?

> +	if (statvfs(def->target.path,&sb)<  0) {
> +		virReportSystemError(errno,
> +							 _("cannot statvfs path '%s'"),
> +							 def->target.path);
> +		return -1;
> +	}
> +
> +	def->capacity = ((unsigned long long)sb.f_frsize *
> +					 (unsigned long long)sb.f_blocks);
> +	def->available = ((unsigned long long)sb.f_bfree *
> +							(unsigned long long)sb.f_bsize);
> +	def->allocation = def->capacity - def->available;
> +
> +	return 0;
> +}
> +
> +static virStoragePoolPtr
> +pvsStoragePoolDefine(virConnectPtr conn,
> +                     const char *xml, unsigned int flags)
> +{
[...]
> +
> +    if (virStoragePoolSourceFindDuplicate(&privconn->pools, def)<  0)
> +        goto cleanup;
> +
> +	if (pvsStoragePoolGetAlloc(def))
> +		goto cleanup;

Spacing...

> +pvsStoragePoolDestroy(virStoragePoolPtr pool)
> +{
> +    pvsConnPtr privconn = pool->conn->privateData;
> +    virStoragePoolObjPtr privpool;
> +    int ret = -1;
> +
> +    pvsDriverLock(privconn);
> +    privpool = virStoragePoolObjFindByName(&privconn->pools, pool->name);
> +
> +    if (privpool == NULL) {
> +        pvsError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto cleanup;
> +    }
> +
> +    if (!virStoragePoolObjIsActive(privpool)) {
> +        pvsError(VIR_ERR_OPERATION_INVALID,
> +                 _("storage pool '%s' is not active"), pool->name);
> +        goto cleanup;
> +    }
> +
> +    privpool->active = 0;
> +

No need to set this...


> +pvsStorageVolumeDefine(virStoragePoolObjPtr pool,
> +                       const char *xmldesc,
> +                       const char *xmlfile, bool is_new)
> +{
> +    virStorageVolDefPtr privvol = NULL;
> +    virStorageVolDefPtr ret = NULL;
> +    char *xml_path = NULL;
> +
> +    if (xmlfile)
> +        privvol = virStorageVolDefParseFile(pool->def, xmlfile);
> +    else
> +        privvol = virStorageVolDefParseString(pool->def, xmldesc);
> +    if (privvol == NULL)
> +        goto cleanup;
> +
> +    if (virStorageVolDefFindByName(pool, privvol->name)) {
> +        pvsError(VIR_ERR_OPERATION_FAILED,
> +                 "%s", _("storage vol already exists"));
> +        goto cleanup;
> +    }
> +
> +    if (is_new) {
> +        /* Make sure enough space */
> +        if ((pool->def->allocation + privvol->allocation)>
> +            pool->def->capacity) {
> +            pvsError(VIR_ERR_INTERNAL_ERROR,
> +                     _("Not enough free space in pool for volume '%s'"),
> +                     privvol->name);
> +            goto cleanup;
> +        }
> +    }
> +
> +    if (VIR_REALLOC_N(pool->volumes.objs, pool->volumes.count + 1)<  0) {
> +        virReportOOMError();
> +        goto cleanup;
> +    }
> +
> +    if (virAsprintf(&privvol->target.path, "%s/%s",
> +                    pool->def->target.path, privvol->name) == -1) {

Nit: < 0

> +static virStorageVolPtr
> +pvsStorageVolumeCreateXML(virStoragePoolPtr pool,
> +                          const char *xmldesc, unsigned int flags)
> +{
> +    pvsConnPtr privconn = pool->conn->privateData;
> +    virStoragePoolObjPtr privpool;
> +    virStorageVolPtr ret = NULL;
> +    virStorageVolDefPtr privvol = NULL;
> +
> +    virCheckFlags(0, NULL);
> +
> +    pvsDriverLock(privconn);
> +    privpool = virStoragePoolObjFindByName(&privconn->pools, pool->name);
> +    pvsDriverUnlock(privconn);
> +
> +    if (privpool == NULL) {
> +        pvsError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> +        goto cleanup;
> +    }
> +
> +    if (!virStoragePoolObjIsActive(privpool)) {
> +        pvsError(VIR_ERR_OPERATION_INVALID,
> +                 _("storage pool '%s' is not active"), pool->name);
> +        goto cleanup;
> +    }
> +
> +    privvol = pvsStorageVolumeDefine(privpool, xmldesc, NULL, true);
> +

NULL pointer test?

> +/*
> + * Return new file path in malloced string created by
> + * concatenating first and second function arguments.
> + */
> +char *
> +pvsAddFileExt(const char *path, const char *ext)
> +{
> +    char *new_path = NULL;
> +    size_t len = strlen(path) + strlen(ext) + 1;
> +
> +    if (VIR_ALLOC_N(new_path, len)<  0)
> +        return NULL;
> +

Nit: virReportOOMError() right where it happens rather than at the callers

> +    if (!virStrcpy(new_path, path, len))
> +        return NULL;
> +    strcat(new_path, ext);
> +
> +    return new_path;
> +}