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

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



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 parallels com>
---

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


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