Re: [PATCH v8 7/8] parallels: add storage driver

[Dmitry Guryanov <dguryanov@xxxxxxxxxxxxx>]

On 07/20/2012 07:34 PM, Peter Krempa wrote:
> On 07/04/12 19:42, Dmitry Guryanov wrote:
> > PARALLELS has one serious discrepancy with libvirt: libvirt stores
> > domain configuration files in one place, and storage files
> > in other places (with the API of storage pools and storage volumes).
> > PARALLELS stores 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.
> >
> > Signed-off-by: Dmitry Guryanov <dguryanov@xxxxxxxxxxxxx>
> > ---
>
> Comments inline.

Hello, Peter,

Thank you very much for review !

This storage driver isn't completely working yet, it just allows to create
a new VM using virt-manager or virsh.

I'm going to add support of hard disk devices to parallels_driver.c,
then it will be possible to populate actual list of volumes. Now this
driver can operate on the volume definitions only.

> >   po/POTFILES.in                    |    1 +
> >   src/Makefile.am                   |    3 +-
> >   src/parallels/parallels_driver.c  |    6 +-
> >   src/parallels/parallels_driver.h  |    5 +
> >   src/parallels/parallels_storage.c | 1460 
> > +++++++++++++++++++++++++++++++++++++
> >   src/parallels/parallels_utils.c   |   24 +
> >   6 files changed, 1496 insertions(+), 3 deletions(-)
> >   create mode 100644 src/parallels/parallels_storage.c
> >
> > diff --git a/po/POTFILES.in b/po/POTFILES.in
> > index dcb0813..240becb 100644
> > --- a/po/POTFILES.in
> > +++ b/po/POTFILES.in
> > @@ -66,6 +66,7 @@ src/phyp/phyp_driver.c
......
> > +        goto cleanup;
> > +    }
> > +
> > +    if (virStoragePoolObjIsActive(privpool)) {
> > +        parallelsError(VIR_ERR_OPERATION_INVALID,
> > +                 _("storage pool '%s' is already active"), pool->name);
> > +        goto cleanup;
> > +    }
>
> Also this function doesn't appear to be doing what it is supposed to do.

I'll remove it from this patch.

> > +
> > +    ret = 0;
> > +
> > +  cleanup:
> > +    if (privpool)
> > +        virStoragePoolObjUnlock(privpool);
> > +    return ret;
> > +}
> > +
> > +
> > +static int
> > +parallelsStoragePoolRefresh(virStoragePoolPtr pool, unsigned int flags)
> > +{
> > +    parallelsConnPtr privconn = pool->conn->privateData;
> > +    virStoragePoolObjPtr privpool;
> > +    int ret = -1;
> > +
> > +    virCheckFlags(0, -1);
> > +
> > +    parallelsDriverLock(privconn);
> > +    privpool = virStoragePoolObjFindByName(&privconn->pools, 
> > pool->name);
> > +    parallelsDriverUnlock(privconn);
> > +
> > +    if (privpool == NULL) {
> > +        parallelsError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> > +        goto cleanup;
> > +    }
> > +
> > +    if (!virStoragePoolObjIsActive(privpool)) {
> > +        parallelsError(VIR_ERR_OPERATION_INVALID,
> > +                 _("storage pool '%s' is not active"), pool->name);
> > +        goto cleanup;
> > +    }
> > +    ret = 0;
>
> Same here.

And this function too.

> .....
> > +
> > +  cleanup:
> > +    if (privpool)
> > +        virStoragePoolObjUnlock(privpool);
> > +    return ret;
> > +}
> > +
> > +
> > +static virStorageVolPtr
> > +parallelsStorageVolumeCreateXMLFrom(virStoragePoolPtr pool,
> > +                              const char *xmldesc,
> > +                              virStorageVolPtr clonevol,
> > +                              unsigned int flags)
> > +{
> > +    parallelsConnPtr privconn = pool->conn->privateData;
> > +    virStoragePoolObjPtr privpool;
> > +    virStorageVolDefPtr privvol = NULL, origvol = NULL;
> > +    virStorageVolPtr ret = NULL;
> > +
> > +    virCheckFlags(0, NULL);
> > +
> > +    parallelsDriverLock(privconn);
> > +    privpool = virStoragePoolObjFindByName(&privconn->pools, 
> > pool->name);
> > +    parallelsDriverUnlock(privconn);
> > +
> > +    if (privpool == NULL) {
> > +        parallelsError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> > +        goto cleanup;
> > +    }
> > +
> > +    if (!virStoragePoolObjIsActive(privpool)) {
> > +        parallelsError(VIR_ERR_OPERATION_INVALID,
> > +                 _("storage pool '%s' is not active"), pool->name);
> > +        goto cleanup;
> > +    }
> > +
> > +    privvol = virStorageVolDefParseString(privpool->def, xmldesc);
> > +    if (privvol == NULL)
> > +        goto cleanup;
> > +
> > +    if (virStorageVolDefFindByName(privpool, privvol->name)) {
> > +        parallelsError(VIR_ERR_OPERATION_FAILED,
> > +                 "%s", _("storage vol already exists"));
> > +        goto cleanup;
> > +    }
> > +
> > +    origvol = virStorageVolDefFindByName(privpool, clonevol->name);
> > +    if (!origvol) {
> > +        parallelsError(VIR_ERR_NO_STORAGE_VOL,
> > +                 _("no storage vol with matching name '%s'"),
> > +                 clonevol->name);
> > +        goto cleanup;
> > +    }
> > +
> > +    /* Make sure enough space */
> > +    if ((privpool->def->allocation + privvol->allocation) >
> > +        privpool->def->capacity) {
> > +        parallelsError(VIR_ERR_INTERNAL_ERROR,
> > +                 _("Not enough free space in pool for volume '%s'"),
> > +                 privvol->name);
> > +        goto cleanup;
> > +    }
> > +    privpool->def->available = (privpool->def->capacity -
> > + privpool->def->allocation);
> > +
> > +    if (VIR_REALLOC_N(privpool->volumes.objs,
> > +                      privpool->volumes.count + 1) < 0) {
> > +        virReportOOMError();
> > +        goto cleanup;
> > +    }
> > +
> > +    if (virAsprintf(&privvol->target.path, "%s/%s",
> > +                    privpool->def->target.path, privvol->name) == -1) {
> > +        virReportOOMError();
> > +        goto cleanup;
> > +    }
> > +
> > +    privvol->key = strdup(privvol->target.path);
> > +    if (privvol->key == NULL) {
> > +        virReportOOMError();
> > +        goto cleanup;
> > +    }
> > +
> > +    privpool->def->allocation += privvol->allocation;
> > +    privpool->def->available = (privpool->def->capacity -
> > + privpool->def->allocation);
> > +
> > +    privpool->volumes.objs[privpool->volumes.count++] = privvol;
>
> This function copies the volume definition but does not actualy copy 
> data, is that ok?

It seems  this function isn't needed, I'll remove it.

> > +
> > +    ret = virGetStorageVol(pool->conn, privpool->def->name,
> > +                           privvol->name, privvol->key);
> > +    privvol = NULL;
> > +
> > +  cleanup:
> > +    virStorageVolDefFree(privvol);
> > +    if (privpool)
> > +        virStoragePoolObjUnlock(privpool);
> > +    return ret;
> > +}
> > +
> > +static int
> > +parallelsStorageVolumeDelete(virStorageVolPtr vol, unsigned int flags)
> > +{
> > +    parallelsConnPtr privconn = vol->conn->privateData;
> > +    virStoragePoolObjPtr privpool;
> > +    virStorageVolDefPtr privvol;
> > +    int i;
> > +    int ret = -1;
> > +    char *xml_path = NULL;
> > +
> > +    virCheckFlags(0, -1);
> > +
> > +    parallelsDriverLock(privconn);
> > +    privpool = virStoragePoolObjFindByName(&privconn->pools, 
> > vol->pool);
> > +    parallelsDriverUnlock(privconn);
> > +
> > +    if (privpool == NULL) {
> > +        parallelsError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> > +        goto cleanup;
> > +    }
> > +
> > +
> > +    privvol = virStorageVolDefFindByName(privpool, vol->name);
> > +
> > +    if (privvol == NULL) {
> > +        parallelsError(VIR_ERR_NO_STORAGE_VOL,
> > +                 _("no storage vol with matching name '%s'"), 
> > vol->name);
> > +        goto cleanup;
> > +    }
> > +
> > +    if (!virStoragePoolObjIsActive(privpool)) {
> > +        parallelsError(VIR_ERR_OPERATION_INVALID,
> > +                 _("storage pool '%s' is not active"), vol->pool);
> > +        goto cleanup;
> > +    }
> > +
> > +
> > +    privpool->def->allocation -= privvol->allocation;
> > +    privpool->def->available = (privpool->def->capacity -
> > + privpool->def->allocation);
> > +
> > +    for (i = 0; i < privpool->volumes.count; i++) {
> > +        if (privpool->volumes.objs[i] == privvol) {
> > +            xml_path = parallelsAddFileExt(privvol->target.path, 
> > ".xml");
> > +            if (!xml_path)
> > +                goto cleanup;
> > +
> > +            if (unlink(xml_path)) {
> > +                parallelsError(VIR_ERR_OPERATION_FAILED,
> > +                         _("Can't remove file '%s'"), xml_path);
> > +                goto cleanup;
> > +            }
> > +
> > +            virStorageVolDefFree(privvol);
> > +
> > +            if (i < (privpool->volumes.count - 1))
> > +                memmove(privpool->volumes.objs + i,
> > +                        privpool->volumes.objs + i + 1,
> > +                        sizeof(*(privpool->volumes.objs)) *
> > +                        (privpool->volumes.count - (i + 1)));
> > +
> > +            if (VIR_REALLOC_N(privpool->volumes.objs,
> > +                              privpool->volumes.count - 1) < 0) {
> > +                ;               /* Failure to reduce memory 
> > allocation isn't fatal */
> > +            }
> > +            privpool->volumes.count--;
> > +
> > +            break;
> > +        }
> > +    }
>
> Same here. You remove the definition but you don't touch the data.

Yes, this is OK for now.

> > +    ret = 0;
> > +
> > +  cleanup:
> > +    if (privpool)
> > +        virStoragePoolObjUnlock(privpool);
> > +    VIR_FREE(xml_path);
> > +    return ret;
> > +}
> > +
> > +
> > +static int
> > +parallelsStorageVolumeTypeForPool(int pooltype)
> > +{
> > +
> > +    switch (pooltype) {
> > +        case VIR_STORAGE_POOL_DIR:
> > +        case VIR_STORAGE_POOL_FS:
> > +        case VIR_STORAGE_POOL_NETFS:
> > +            return VIR_STORAGE_VOL_FILE;
> > +        default:
> > +            return VIR_STORAGE_VOL_BLOCK;
> > +    }
> > +}
> > +
> > +static int
> > +parallelsStorageVolumeGetInfo(virStorageVolPtr vol, 
> > virStorageVolInfoPtr info)
> > +{
> > +    parallelsConnPtr privconn = vol->conn->privateData;
> > +    virStoragePoolObjPtr privpool;
> > +    virStorageVolDefPtr privvol;
> > +    int ret = -1;
> > +
> > +    parallelsDriverLock(privconn);
> > +    privpool = virStoragePoolObjFindByName(&privconn->pools, 
> > vol->pool);
> > +    parallelsDriverUnlock(privconn);
> > +
> > +    if (privpool == NULL) {
> > +        parallelsError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> > +        goto cleanup;
> > +    }
> > +
> > +    privvol = virStorageVolDefFindByName(privpool, vol->name);
> > +
> > +    if (privvol == NULL) {
> > +        parallelsError(VIR_ERR_NO_STORAGE_VOL,
> > +                 _("no storage vol with matching name '%s'"), 
> > vol->name);
> > +        goto cleanup;
> > +    }
> > +
> > +    if (!virStoragePoolObjIsActive(privpool)) {
> > +        parallelsError(VIR_ERR_OPERATION_INVALID,
> > +                 _("storage pool '%s' is not active"), vol->pool);
> > +        goto cleanup;
> > +    }
> > +
> > +    memset(info, 0, sizeof(*info));
> > +    info->type = 
> > parallelsStorageVolumeTypeForPool(privpool->def->type);
> > +    info->capacity = privvol->capacity;
> > +    info->allocation = privvol->allocation;
> > +    ret = 0;
> > +
> > +  cleanup:
> > +    if (privpool)
> > +        virStoragePoolObjUnlock(privpool);
> > +    return ret;
> > +}
> > +
> > +static char *
> > +parallelsStorageVolumeGetXMLDesc(virStorageVolPtr vol, unsigned int 
> > flags)
> > +{
> > +    parallelsConnPtr privconn = vol->conn->privateData;
> > +    virStoragePoolObjPtr privpool;
> > +    virStorageVolDefPtr privvol;
> > +    char *ret = NULL;
> > +
> > +    virCheckFlags(0, NULL);
> > +
> > +    parallelsDriverLock(privconn);
> > +    privpool = virStoragePoolObjFindByName(&privconn->pools, 
> > vol->pool);
> > +    parallelsDriverUnlock(privconn);
> > +
> > +    if (privpool == NULL) {
> > +        parallelsError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> > +        goto cleanup;
> > +    }
> > +
> > +    privvol = virStorageVolDefFindByName(privpool, vol->name);
> > +
> > +    if (privvol == NULL) {
> > +        parallelsError(VIR_ERR_NO_STORAGE_VOL,
> > +                 _("no storage vol with matching name '%s'"), 
> > vol->name);
> > +        goto cleanup;
> > +    }
> > +
> > +    if (!virStoragePoolObjIsActive(privpool)) {
> > +        parallelsError(VIR_ERR_OPERATION_INVALID,
> > +                 _("storage pool '%s' is not active"), vol->pool);
> > +        goto cleanup;
> > +    }
> > +
> > +    ret = virStorageVolDefFormat(privpool->def, privvol);
> > +
> > +  cleanup:
> > +    if (privpool)
> > +        virStoragePoolObjUnlock(privpool);
> > +    return ret;
> > +}
> > +
> > +static char *
> > +parallelsStorageVolumeGetPath(virStorageVolPtr vol)
> > +{
> > +    parallelsConnPtr privconn = vol->conn->privateData;
> > +    virStoragePoolObjPtr privpool;
> > +    virStorageVolDefPtr privvol;
> > +    char *ret = NULL;
> > +
> > +    parallelsDriverLock(privconn);
> > +    privpool = virStoragePoolObjFindByName(&privconn->pools, 
> > vol->pool);
> > +    parallelsDriverUnlock(privconn);
> > +
> > +    if (privpool == NULL) {
> > +        parallelsError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> > +        goto cleanup;
> > +    }
> > +
> > +    privvol = virStorageVolDefFindByName(privpool, vol->name);
> > +
> > +    if (privvol == NULL) {
> > +        parallelsError(VIR_ERR_NO_STORAGE_VOL,
> > +                 _("no storage vol with matching name '%s'"), 
> > vol->name);
> > +        goto cleanup;
> > +    }
> > +
> > +    if (!virStoragePoolObjIsActive(privpool)) {
> > +        parallelsError(VIR_ERR_OPERATION_INVALID,
> > +                 _("storage pool '%s' is not active"), vol->pool);
> > +        goto cleanup;
> > +    }
> > +
> > +    ret = strdup(privvol->target.path);
> > +    if (ret == NULL)
> > +        virReportOOMError();
> > +
> > +  cleanup:
> > +    if (privpool)
> > +        virStoragePoolObjUnlock(privpool);
> > +    return ret;
> > +}
> > +
> > +static virStorageDriver parallelsStorageDriver = {
> > +    .name = "PARALLELS",
> > +    .open = parallelsStorageOpen,     /* 0.10.0 */
> > +    .close = parallelsStorageClose,   /* 0.10.0 */
> > +
> > +    .numOfPools = parallelsStorageNumPools,   /* 0.10.0 */
> > +    .listPools = parallelsStorageListPools,   /* 0.10.0 */
> > +    .numOfDefinedPools = parallelsStorageNumDefinedPools, /* 0.10.0 */
> > +    .listDefinedPools = parallelsStorageListDefinedPools, /* 0.10.0 */
> > +    .findPoolSources = parallelsStorageFindPoolSources, /* 0.10.0 */
> > +    .poolLookupByName = parallelsStoragePoolLookupByName, /* 0.10.0 */
> > +    .poolLookupByUUID = parallelsStoragePoolLookupByUUID, /* 0.10.0 */
> > +    .poolLookupByVolume = parallelsStoragePoolLookupByVolume, /* 
> > 0.10.0 */
> > +    .poolDefineXML = parallelsStoragePoolDefine,      /* 0.10.0 */
> > +    .poolBuild = parallelsStoragePoolBuild,   /* 0.10.0 */
> > +    .poolUndefine = parallelsStoragePoolUndefine,     /* 0.10.0 */
> > +    .poolCreate = parallelsStoragePoolStart,  /* 0.10.0 */
> > +    .poolDestroy = parallelsStoragePoolDestroy,       /* 0.10.0 */
> > +    .poolDelete = parallelsStoragePoolDelete, /* 0.10.0 */
> > +    .poolRefresh = parallelsStoragePoolRefresh,       /* 0.10.0 */
> > +    .poolGetInfo = parallelsStoragePoolGetInfo,       /* 0.10.0 */
> > +    .poolGetXMLDesc = parallelsStoragePoolGetXMLDesc, /* 0.10.0 */
> > +    .poolGetAutostart = parallelsStoragePoolGetAutostart, /* 0.10.0 */
> > +    .poolSetAutostart = parallelsStoragePoolSetAutostart, /* 0.10.0 */
> > +    .poolNumOfVolumes = parallelsStoragePoolNumVolumes, /* 0.10.0 */
> > +    .poolListVolumes = parallelsStoragePoolListVolumes, /* 0.10.0 */
> > +
> > +    .volLookupByName = parallelsStorageVolumeLookupByName, /* 0.10.0 */
> > +    .volLookupByKey = parallelsStorageVolumeLookupByKey, /* 0.10.0 */
> > +    .volLookupByPath = parallelsStorageVolumeLookupByPath, /* 0.10.0 */
> > +    .volCreateXML = parallelsStorageVolumeCreateXML,  /* 0.10.0 */
> > +    .volCreateXMLFrom = parallelsStorageVolumeCreateXMLFrom, /* 
> > 0.10.0 */
> > +    .volDelete = parallelsStorageVolumeDelete,        /* 0.10.0 */
> > +    .volGetInfo = parallelsStorageVolumeGetInfo,      /* 0.10.0 */
> > +    .volGetXMLDesc = parallelsStorageVolumeGetXMLDesc, /* 0.10.0 */
> > +    .volGetPath = parallelsStorageVolumeGetPath,      /* 0.10.0 */
> > +    .poolIsActive = parallelsStoragePoolIsActive,     /* 0.10.0 */
> > +    .poolIsPersistent = parallelsStoragePoolIsPersistent, /* 0.10.0 */
> > +};
> > +
> > +int
> > +parallelsStorageRegister(void)
> > +{
> > +    if (virRegisterStorageDriver(&parallelsStorageDriver) < 0)
> > +        return -1;
> > +
> > +    return 0;
> > +}
> > diff --git a/src/parallels/parallels_utils.c 
> > b/src/parallels/parallels_utils.c
> > index e4220e9..72178d9 100644
> > --- a/src/parallels/parallels_utils.c
> > +++ b/src/parallels/parallels_utils.c
> > @@ -30,6 +30,8 @@
> >
> >   #include "parallels_driver.h"
> >
> > +#define VIR_FROM_THIS VIR_FROM_PARALLELS
> > +
> >   static int
> >   parallelsDoCmdRun(char **outbuf, const char *binary, va_list list)
> >   {
> > @@ -105,3 +107,25 @@ parallelsCmdRun(const char *binary, ...)
> >
> >       return ret;
> >   }
> > +
> > +/*
> > + * Return new file path in malloced string created by
> > + * concatenating first and second function arguments.
> > + */
> > +char *
> > +parallelsAddFileExt(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) {
> > +        virReportOOMError();
> > +        return NULL;
> > +    }
> > +
> > +    if (!virStrcpy(new_path, path, len))
> > +        return NULL;
> > +    strcat(new_path, ext);
> > +
> > +    return new_path;
> > +}
>
> Peter


--
Dmitry Guryanov

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list