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

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(a)parallels.com&gt; > --- Comments inline.

> 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.

> + > + 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.

..... > + > + 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?

> + > + 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.

> + 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