[libvirt] [PATCH 0/3] Add stateDir for storage driver and use for disk backend
https://bugzilla.redhat.com/show_bug.cgi?id=1138516 Using the network (and domain) for reference, add a 'stateDir' for the Storage Driver in order to save off pool and/or volume specific information into a live running area. This is useful for the disk backend where volume creation can define a specific name and target format type to use; however, that information cannot be encoded into the 'parted' data used to create the partition(s) on the disk. Thus the data is lost for refresh and restart/reload. The following patches will add the stateDir, then the infrastructure for storage volume xml to be saved off, and finally saving/using that data for the refresh, restart/reload path for the disk backend driver. John Ferlan (3): storage: Create a stateDir storage: Introduce virStorageVol{Save|Delete}Status storage: Adjustments to disk backend to use stateDir src/conf/storage_conf.c | 112 +++++++++++++++++++++++++++++++++---- src/conf/storage_conf.h | 13 ++++- src/libvirt_private.syms | 2 + src/parallels/parallels_storage.c | 5 +- src/storage/storage_backend_disk.c | 90 ++++++++++++++++++++++++++++- src/storage/storage_driver.c | 32 +++++++++-- 6 files changed, 232 insertions(+), 22 deletions(-) -- 2.1.0
https://bugzilla.redhat.com/show_bug.cgi?id=1138516 In order to allow storage backend drivers to save state between refreshes, restarts or reloads, create a stateDir in virStorageDriverState and virStoragePoolObj. At storage driver initialization generate either a /run/libvirt/storage or {virGetuserRuntimeDirectory()}/storage/run area to store specific state that may be needed. Propagate this to the pool->stateDir which will be the driver->stateDir plus the pool name. During pool load and libvirtd state initialize or state reload, create the path for the pool->stateDir. During pool delete, remove the entire status tree for the pool->stateDir. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 55 +++++++++++++++++++++++++++++++-------- src/conf/storage_conf.h | 7 +++-- src/parallels/parallels_storage.c | 5 ++-- src/storage/storage_driver.c | 32 +++++++++++++++++++---- 4 files changed, 79 insertions(+), 20 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index e9aaa8a..0901dca 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1,7 +1,7 @@ /* * storage_conf.c: config handling for storage driver * - * Copyright (C) 2006-2014 Red Hat, Inc. + * Copyright (C) 2006-2015 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -421,6 +421,7 @@ virStoragePoolObjFree(virStoragePoolObjPtr obj) VIR_FREE(obj->configFile); VIR_FREE(obj->autostartLink); + VIR_FREE(obj->stateDir); virMutexDestroy(&obj->lock); @@ -1802,7 +1803,8 @@ static virStoragePoolObjPtr virStoragePoolObjLoad(virStoragePoolObjListPtr pools, const char *file, const char *path, - const char *autostartLink) + const char *autostartLink, + const char *stateDir) { virStoragePoolDefPtr def; virStoragePoolObjPtr pool; @@ -1834,6 +1836,15 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools, virStoragePoolDefFree(def); return NULL; } + VIR_FREE(pool->stateDir); /* for driver reload */ + if (!(pool->stateDir = virFileBuildPath(stateDir, def->name, NULL))) { + virStoragePoolDefFree(def); + return NULL; + } + if (virFileMakePath(pool->stateDir) < 0) { + virStoragePoolDefFree(def); + return NULL; + } pool->autostart = virFileLinkPointsTo(pool->autostartLink, pool->configFile); @@ -1845,7 +1856,8 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools, int virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr pools, const char *configDir, - const char *autostartDir) + const char *autostartDir, + const char *stateDir) { DIR *dir; struct dirent *entry; @@ -1880,7 +1892,7 @@ virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr pools, } pool = virStoragePoolObjLoad(pools, entry->d_name, path, - autostartLink); + autostartLink, stateDir); if (pool) virStoragePoolObjUnlock(pool); @@ -1925,22 +1937,38 @@ virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, virReportSystemError(errno, _("cannot create config directory %s"), driver->configDir); - return -1; + goto error; } if (!(pool->configFile = virFileBuildPath(driver->configDir, - def->name, ".xml"))) { - return -1; - } + def->name, ".xml"))) + goto error; if (!(pool->autostartLink = virFileBuildPath(driver->autostartDir, - def->name, ".xml"))) { - VIR_FREE(pool->configFile); - return -1; + def->name, ".xml"))) + goto error; + } + + if (!pool->stateDir) { + if (!(pool->stateDir = virFileBuildPath(driver->stateDir, + def->name, NULL))) + goto error; + + if (virFileMakePath(pool->stateDir) < 0) { + virReportSystemError(errno, + _("cannot create pool state directory %s"), + pool->stateDir); + goto error; } } return virStoragePoolSaveConfig(pool->configFile, def); + + error: + VIR_FREE(pool->configFile); + VIR_FREE(pool->autostartLink); + VIR_FREE(pool->stateDir); + return -1; } int @@ -1959,6 +1987,11 @@ virStoragePoolObjDeleteDef(virStoragePoolObjPtr pool) return -1; } + if (virFileExists(pool->stateDir)) { + if (virFileDeleteTree(pool->stateDir) < 0) + return -1; + } + return 0; } diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 2c9eaed..064c78c 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -1,7 +1,7 @@ /* * storage_conf.h: config handling for storage driver * - * Copyright (C) 2006-2008, 2010-2014 Red Hat, Inc. + * Copyright (C) 2006-2008, 2010-2015 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -266,6 +266,7 @@ struct _virStoragePoolObj { char *configFile; char *autostartLink; + char *stateDir; int active; int autostart; unsigned int asyncjobs; @@ -293,6 +294,7 @@ struct _virStorageDriverState { char *configDir; char *autostartDir; + char *stateDir; bool privileged; }; @@ -315,7 +317,8 @@ virStoragePoolObjIsActive(virStoragePoolObjPtr pool) int virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr pools, const char *configDir, - const char *autostartDir); + const char *autostartDir, + const char *stateDir); virStoragePoolObjPtr virStoragePoolObjFindByUUID(virStoragePoolObjListPtr pools, diff --git a/src/parallels/parallels_storage.c b/src/parallels/parallels_storage.c index e916e0f..bde6ecf 100644 --- a/src/parallels/parallels_storage.c +++ b/src/parallels/parallels_storage.c @@ -2,7 +2,7 @@ * parallels_storage.c: core driver functions for managing * Parallels Cloud Server hosts * - * Copyright (C) 2013-2014 Red Hat, Inc. + * Copyright (C) 2013-2015 Red Hat, Inc. * Copyright (C) 2012 Parallels, Inc. * * This library is free software; you can redistribute it and/or @@ -424,7 +424,8 @@ static int parallelsLoadPools(virConnectPtr conn) if (virStoragePoolLoadAllConfigs(&privconn->pools, storageState->configDir, - storageState->autostartDir) < 0) { + storageState->autostartDir, + storageState->stateDir) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to load pool configs")); goto error; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 66dc994..d2ccf1b 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1,7 +1,7 @@ /* * storage_driver.c: core driver for storage APIs * - * Copyright (C) 2006-2014 Red Hat, Inc. + * Copyright (C) 2006-2015 Red Hat, Inc. * Copyright (C) 2006-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -146,7 +146,7 @@ storageDriverAutostart(void) /** * virStorageStartup: * - * Initialization function for the QEmu daemon + * Initialization function for the Storage Driver */ static int storageStateInitialize(bool privileged, @@ -154,6 +154,7 @@ storageStateInitialize(bool privileged, void *opaque ATTRIBUTE_UNUSED) { char *base = NULL; + char *rundir = NULL; if (VIR_ALLOC(driver) < 0) return -1; @@ -167,10 +168,18 @@ storageStateInitialize(bool privileged, if (privileged) { if (VIR_STRDUP(base, SYSCONFDIR "/libvirt") < 0) goto error; + if (virAsprintf(&driver->stateDir, + "%s/run/libvirt/storage", LOCALSTATEDIR) < 0) + goto error; } else { base = virGetUserConfigDirectory(); - if (!base) + rundir = virGetUserRuntimeDirectory(); + if (!(base && rundir)) + goto error; + + if (virAsprintf(&driver->stateDir, "%s/storage/run", rundir) < 0) goto error; + } driver->privileged = privileged; @@ -187,10 +196,19 @@ storageStateInitialize(bool privileged, goto error; VIR_FREE(base); + VIR_FREE(rundir); + + if (virFileMakePath(driver->stateDir) < 0) { + virReportSystemError(errno, + _("failed to create state directory '%s'"), + driver->stateDir); + goto error; + } if (virStoragePoolLoadAllConfigs(&driver->pools, driver->configDir, - driver->autostartDir) < 0) + driver->autostartDir, + driver->stateDir) < 0) goto error; storageDriverUnlock(); @@ -198,6 +216,7 @@ storageStateInitialize(bool privileged, error: VIR_FREE(base); + VIR_FREE(rundir); storageDriverUnlock(); storageStateCleanup(); return -1; @@ -234,7 +253,8 @@ storageStateReload(void) storageDriverLock(); virStoragePoolLoadAllConfigs(&driver->pools, driver->configDir, - driver->autostartDir); + driver->autostartDir, + driver->stateDir); storageDriverAutostart(); storageDriverUnlock(); @@ -260,6 +280,7 @@ storageStateCleanup(void) VIR_FREE(driver->configDir); VIR_FREE(driver->autostartDir); + VIR_FREE(driver->stateDir); storageDriverUnlock(); virMutexDestroy(&driver->lock); VIR_FREE(driver); @@ -743,6 +764,7 @@ storagePoolUndefine(virStoragePoolPtr obj) VIR_FREE(pool->configFile); VIR_FREE(pool->autostartLink); + VIR_FREE(pool->stateDir); VIR_INFO("Undefining storage pool '%s'", pool->def->name); virStoragePoolObjRemove(&driver->pools, pool); -- 2.1.0
https://bugzilla.redhat.com/show_bug.cgi?id=1138516 This patch introduces two generic storage_conf functions to handle saving and deleting the volume XML to/from the stateDir. The virStorageVolSaveStatus will format the volume XML into a file and save the file in pool->stateDir. The virStorageVolDeleteStatus will search for the file in the stateDir and remove it. These are useful for certain fields/data in the volume XML that cannot be determined or saved along with the volume or in some meta-data for the volume. Prior to a pool refresh operation the pool's volume list is cleared in order to be restored during the refresh. If there were fields that are not otherwise obtainable, the defaults will be used which may or may not be correct. Additionally, the libvirtd daemon reload/restart processing will be able to make use of the saved XML data and follow the same path with respect to regenerating the list of volumes in the pool and thus not having the same issue. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/storage_conf.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++ src/conf/storage_conf.h | 6 +++++ src/libvirt_private.syms | 2 ++ 3 files changed, 65 insertions(+) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 0901dca..9ea00e5 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1662,6 +1662,63 @@ virStorageVolDefFormat(virStoragePoolDefPtr pool, } +int +virStorageVolSaveStatus(virStoragePoolObjPtr pool, + virStorageVolDefPtr vol) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + char *xml = NULL; + char *statusFile = NULL; + int ret = -1; + + if (virAsprintf(&statusFile, "%s/%s.xml", pool->stateDir, vol->name) < 0) + goto cleanup; + + if (!(xml = virStorageVolDefFormat(pool->def, vol))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to generate XML")); + goto cleanup; + } + + virUUIDFormat(pool->def->uuid, uuidstr); + ret = virXMLSaveFile(statusFile, + virXMLPickShellSafeComment(vol->name, uuidstr), + "vol-edit", xml); + cleanup: + VIR_FREE(xml); + VIR_FREE(statusFile); + return ret; +} + + +int +virStorageVolDeleteStatus(virStoragePoolObjPtr pool, + virStorageVolDefPtr vol) +{ + char *statusFile = NULL; + int ret = -1; + + if (virAsprintf(&statusFile, "%s/%s.xml", pool->stateDir, vol->name) < 0) + goto cleanup; + + if (virFileExists(statusFile)) { + if (unlink(statusFile) < 0) { + virReportSystemError(errno, + _("cannot remove vol status file '%s'"), + statusFile); + goto cleanup; + } + } + + ret = 0; + + cleanup: + VIR_FREE(statusFile); + + return ret; +} + + virStoragePoolObjPtr virStoragePoolObjFindByUUID(virStoragePoolObjListPtr pools, const unsigned char *uuid) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 064c78c..9f25287 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -360,6 +360,12 @@ virStorageVolDefParseNode(virStoragePoolDefPtr pool, xmlNodePtr root); char *virStorageVolDefFormat(virStoragePoolDefPtr pool, virStorageVolDefPtr def); +int virStorageVolSaveStatus(virStoragePoolObjPtr pool, + virStorageVolDefPtr vol) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int virStorageVolDeleteStatus(virStoragePoolObjPtr pool, + virStorageVolDefPtr vol) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); virStoragePoolObjPtr virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7ceb54d..a84a293 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -784,6 +784,8 @@ virStorageVolDefFree; virStorageVolDefParseFile; virStorageVolDefParseNode; virStorageVolDefParseString; +virStorageVolDeleteStatus; +virStorageVolSaveStatus; virStorageVolTypeFromString; virStorageVolTypeToString; -- 2.1.0
https://bugzilla.redhat.com/show_bug.cgi?id=1138516 The disk backend uses 'parted' in order to create and delete partitions on the disk for use in the pool. At creation time, one can specify a specific "name" for the volume as well as a specific volume target format type. The name and volume target type "survive" only as long as the pool is not refreshed or the libvirtd not restarted/reloaded. The action immediately prior to the calling all the backend refreshPool API's is to clear out all the existing volumes from the pool. The theory being the refresh will be able to find all elements of the pool using some mechanism. The disk refreshPool backend will use the libvirt_parthelper utility to read the partitions found on the disk in order to regenerate the elements of the pool Unfortunately, since the "name" and target format type cannot be encoded, the data is now lost and the defaults are used (for the "name", the partition path is used and the default of 'none' is used for the target format type). This patch solves this by adding the ability to save the XML generated at create time into the stateDir and then use that during the refreshPool backend API call to restore the specific fields that are lost. Once the partition has been successfully created during the backend call to createVol (virStorageBackendDiskCreateVol) and the various XML fields updated, call the virStorageVolSaveStatus() API in order to save the XML for later use. Conversely, once the partition has been successfully removed during the backend call to deleteVol (virStorageBackendDiskDeleteVol) call the virStorageVolDeleteStatus() API in order to remove the XML file. The virStorageBackendDiskUpdateStatus called after reading the data about the partitions during the refreshPool will scan the pool->stateDir for XML files. For each found and successfully parsed file, look through the recently updated pool's volume list for a volume with the same key. If one is found with a different name than in use for the pool, then use the one from the stateDir and update the target format type and save the updated pool volume. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_disk.c | 90 +++++++++++++++++++++++++++++++++++++- 1 file changed, 88 insertions(+), 2 deletions(-) diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 3f97fd9..90b468c 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -1,7 +1,7 @@ /* * storage_backend_disk.c: storage backend for disk handling * - * Copyright (C) 2007-2014 Red Hat, Inc. + * Copyright (C) 2007-2015 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -333,6 +333,78 @@ virStorageBackendDiskReadGeometry(virStoragePoolObjPtr pool) } static int +virStorageBackendDiskUpdateStatus(virStoragePoolObjPtr pool) +{ + DIR *dir; + struct dirent *entry; + virStorageVolDefPtr voldef = NULL, poolvol = NULL; + int ret = -1; + + if (!(dir = opendir(pool->stateDir))) { + if (errno == ENOENT) + return 0; + + virReportSystemError(errno, _("Failed to open dir '%s'"), + pool->stateDir); + return -1; + } + + while ((ret = virDirRead(dir, &entry, pool->stateDir)) > 0) { + char *path; + + if (entry->d_name[0] == '.' || + !virFileHasSuffix(entry->d_name, ".xml")) + continue; + + if (!(path = virFileBuildPath(pool->stateDir, entry->d_name, NULL))) + continue; + + /* Unable to parse */ + voldef = virStorageVolDefParseFile(pool->def, path); + VIR_FREE(path); + if (!voldef) { + ret = -1; + goto cleanup; + } + + /* Search the current storage pool for the key from the file */ + if ((poolvol = virStorageVolDefFindByKey(pool, voldef->key))) { + + /* Swap name if necessary */ + if (STRNEQ(voldef->name, poolvol->name)) { + char *tmp = poolvol->name; + poolvol->name = voldef->name; + voldef->name = tmp; + } + + /* Same for target format type */ + poolvol->target.format = voldef->target.format; + + /* Update the saved file since we could have other changes + * as determined through refresh. + */ + if (virStorageVolSaveStatus(pool, poolvol) < 0) { + virStorageVolDefFree(voldef); + ret = -1; + goto cleanup; + } + } else { + /* If we don't find the key in our current pool, + * then remove stale or transient file + */ + unlink(entry->d_name); + } + + /* Free the voldef (poolvol is just a pointer into poolobj */ + virStorageVolDefFree(voldef); + } + + cleanup: + closedir(dir); + return ret; +} + +static int virStorageBackendDiskRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) { @@ -351,7 +423,13 @@ virStorageBackendDiskRefreshPool(virConnectPtr conn ATTRIBUTE_UNUSED, if (virStorageBackendDiskReadGeometry(pool) != 0) return -1; - return virStorageBackendDiskReadPartitions(pool, NULL); + if (virStorageBackendDiskReadPartitions(pool, NULL) < 0) + return -1; + + if (virStorageBackendDiskUpdateStatus(pool) < 0) + return -1; + + return 0; } @@ -690,6 +768,10 @@ virStorageBackendDiskCreateVol(virConnectPtr conn ATTRIBUTE_UNUSED, if (virStorageBackendDiskReadPartitions(pool, vol) < 0) goto cleanup; + /* Save the XML file in the stateDir for use by reload, restart, refresh */ + if (virStorageVolSaveStatus(pool, vol) < 0) + goto cleanup; + res = 0; cleanup: @@ -775,6 +857,10 @@ virStorageBackendDiskDeleteVol(virConnectPtr conn ATTRIBUTE_UNUSED, goto cleanup; } + /* Remove the XML status file from stateDir */ + if (virStorageVolDeleteStatus(pool, vol) < 0) + goto cleanup; + rc = 0; cleanup: VIR_FREE(devpath); -- 2.1.0
On Tue, Jan 13, 2015 at 04:12:32PM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1138516
The disk backend uses 'parted' in order to create and delete partitions on the disk for use in the pool. At creation time, one can specify a specific "name" for the volume as well as a specific volume target format type. The name and volume target type "survive" only as long as the pool is not refreshed or the libvirtd not restarted/reloaded.
The action immediately prior to the calling all the backend refreshPool API's is to clear out all the existing volumes from the pool. The theory being the refresh will be able to find all elements of the pool using some mechanism. The disk refreshPool backend will use the libvirt_parthelper utility to read the partitions found on the disk in order to regenerate the elements of the pool Unfortunately, since the "name" and target format type cannot be encoded, the data is now lost and the defaults are used (for the "name", the partition path is used and the default of 'none' is used for the target format type).
This patch solves this by adding the ability to save the XML generated at create time into the stateDir and then use that during the refreshPool backend API call to restore the specific fields that are lost.
I don't really think we should be adding a lookaside cache for this. We should simply not permit arbitrary user supplied names for disk based volumes. The names should be required to match the defined naming scheme for disk partitions. I could have sworn that we had enforced that already when I first wrote this, but perhaps that check got lost somewhere along the way. Likewise for the format - we should just rely on partition table format data Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 01/14/2015 04:46 AM, Daniel P. Berrange wrote:
On Tue, Jan 13, 2015 at 04:12:32PM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1138516
The disk backend uses 'parted' in order to create and delete partitions on the disk for use in the pool. At creation time, one can specify a specific "name" for the volume as well as a specific volume target format type. The name and volume target type "survive" only as long as the pool is not refreshed or the libvirtd not restarted/reloaded.
The action immediately prior to the calling all the backend refreshPool API's is to clear out all the existing volumes from the pool. The theory being the refresh will be able to find all elements of the pool using some mechanism. The disk refreshPool backend will use the libvirt_parthelper utility to read the partitions found on the disk in order to regenerate the elements of the pool Unfortunately, since the "name" and target format type cannot be encoded, the data is now lost and the defaults are used (for the "name", the partition path is used and the default of 'none' is used for the target format type).
This patch solves this by adding the ability to save the XML generated at create time into the stateDir and then use that during the refreshPool backend API call to restore the specific fields that are lost.
I don't really think we should be adding a lookaside cache for this.
Fair enough - although this may then turn into a documentation exercise. I'm OK with that option - if that is what is desired.
We should simply not permit arbitrary user supplied names for disk based volumes. The names should be required to match the defined naming scheme for disk partitions. I could have sworn that we had enforced that already when I first wrote this, but perhaps that check got lost somewhere along the way. Likewise for the format - we should just rely on partition table format data
I looked through some history and didn't see anywhere that supplied name must match partition name was enforced, but there's a lot of code motion and new features that may cloud the history. Perhaps it's the assumption true in so many other pools that 'name' is what was used to create the file, directory, lv, etc. There is code that fills in the name if not provided with the partition name (which occurs after Refresh, reload, restart). I'm not quite sure how one could enforce a name given that parted creates the partition name. One would have to know the device name of the pool and the numerical sequence that parted would use (easy perhaps know that perhaps partitions 1 & 2 exist that 3 will be next). However, there's not necessarily a guarantee of that is there? Reading the libvirt docs - the implication is that the volume "<name>" must only be unique across the pool, so the following is "valid": virsh vol-create-as disk-pool vol-linux --format linux 1G So without assuming/describing the naming scheme of the underlying parted how do we enforce that the name (vol-linux) is the same as what parted generates? Or how do we enforce that the name provided ends up being the same as the partition created? It's kind of a chicken/egg issue. Not much is done with the target.format in the backend even though it's described as providing the "partition type". When not provided on the command line like above it defaults to 'none'. Other than for dos label pools that would need to have extended partition before creating a logical partition, there's not much "use" for the field other than to perhaps store "something" for "someone" (until refresh, reload, restart loses it). I don't see changing the code to enforce incoming name matches. Changing the name to match the partition created seems to go against standard practices and for the disk backend the target.format really doesn't matter. I am open to try other suggestions, although outside of this particular test failing perhaps there isn't much demand... John
On Wed, Jan 14, 2015 at 07:48:26PM -0500, John Ferlan wrote:
On 01/14/2015 04:46 AM, Daniel P. Berrange wrote:
On Tue, Jan 13, 2015 at 04:12:32PM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1138516
The disk backend uses 'parted' in order to create and delete partitions on the disk for use in the pool. At creation time, one can specify a specific "name" for the volume as well as a specific volume target format type. The name and volume target type "survive" only as long as the pool is not refreshed or the libvirtd not restarted/reloaded.
The action immediately prior to the calling all the backend refreshPool API's is to clear out all the existing volumes from the pool. The theory being the refresh will be able to find all elements of the pool using some mechanism. The disk refreshPool backend will use the libvirt_parthelper utility to read the partitions found on the disk in order to regenerate the elements of the pool Unfortunately, since the "name" and target format type cannot be encoded, the data is now lost and the defaults are used (for the "name", the partition path is used and the default of 'none' is used for the target format type).
This patch solves this by adding the ability to save the XML generated at create time into the stateDir and then use that during the refreshPool backend API call to restore the specific fields that are lost.
I don't really think we should be adding a lookaside cache for this.
Fair enough - although this may then turn into a documentation exercise. I'm OK with that option - if that is what is desired.
We should simply not permit arbitrary user supplied names for disk based volumes. The names should be required to match the defined naming scheme for disk partitions. I could have sworn that we had enforced that already when I first wrote this, but perhaps that check got lost somewhere along the way. Likewise for the format - we should just rely on partition table format data
I looked through some history and didn't see anywhere that supplied name must match partition name was enforced, but there's a lot of code motion and new features that may cloud the history. Perhaps it's the assumption true in so many other pools that 'name' is what was used to create the file, directory, lv, etc. There is code that fills in the name if not provided with the partition name (which occurs after Refresh, reload, restart).
Looking back some more, it seems I did not ever enforce it - I just left it as something users had to set the "right way" by convention. ie I just expected callers to always use the right "sdaNNN" value
I'm not quite sure how one could enforce a name given that parted creates the partition name. One would have to know the device name of the pool and the numerical sequence that parted would use (easy perhaps know that perhaps partitions 1 & 2 exist that 3 will be next). However, there's not necessarily a guarantee of that is there?
For MS-DOS partition tables the partition entry number directly corresponds to the device name suffix. eg partition 3 corresponds to /dev/sda3 always and is happy to be sparse. eg if you create 3 partitions and then delete partition 2, you'll end up with /dev/sda1 and /dev/sda3 Testing with parted the same seems to be true of GPT tables too.
Reading the libvirt docs - the implication is that the volume "<name>" must only be unique across the pool, so the following is "valid":
virsh vol-create-as disk-pool vol-linux --format linux 1G
So without assuming/describing the naming scheme of the underlying parted how do we enforce that the name (vol-linux) is the same as what parted generates? Or how do we enforce that the name provided ends up being the same as the partition created? It's kind of a chicken/egg issue.
Well with disk partitions we know the device name eg the pool knows we're operating on '/dev/sda', so when we create it can we validate that the volume name is '/dev/sdaNNN' by kjust doing a string prefix match
Not much is done with the target.format in the backend even though it's described as providing the "partition type". When not provided on the command line like above it defaults to 'none'. Other than for dos label pools that would need to have extended partition before creating a logical partition, there's not much "use" for the field other than to perhaps store "something" for "someone" (until refresh, reload, restart loses it).
When you create a partition you can specify filesystem type for that partition, even though it is somewhat redundant as it is not directly connected to the filesystem that you actually mkfs. eg this list of types from fdisk Command (m for help): l 0 Empty 24 NEC DOS 81 Minix / old Lin bf Solaris 1 FAT12 27 Hidden NTFS Win 82 Linux swap / So c1 DRDOS/sec (FAT- 2 XENIX root 39 Plan 9 83 Linux c4 DRDOS/sec (FAT- 3 XENIX usr 3c PartitionMagic 84 OS/2 hidden C: c6 DRDOS/sec (FAT- 4 FAT16 <32M 40 Venix 80286 85 Linux extended c7 Syrinx 5 Extended 41 PPC PReP Boot 86 NTFS volume set da Non-FS data 6 FAT16 42 SFS 87 NTFS volume set db CP/M / CTOS / . 7 HPFS/NTFS/exFAT 4d QNX4.x 88 Linux plaintext de Dell Utility 8 AIX 4e QNX4.x 2nd part 8e Linux LVM df BootIt 9 AIX bootable 4f QNX4.x 3rd part 93 Amoeba e1 DOS access a OS/2 Boot Manag 50 OnTrack DM 94 Amoeba BBT e3 DOS R/O b W95 FAT32 51 OnTrack DM6 Aux 9f BSD/OS e4 SpeedStor c W95 FAT32 (LBA) 52 CP/M a0 IBM Thinkpad hi eb BeOS fs e W95 FAT16 (LBA) 53 OnTrack DM6 Aux a5 FreeBSD ee GPT f W95 Ext'd (LBA) 54 OnTrackDM6 a6 OpenBSD ef EFI (FAT-12/16/ 10 OPUS 55 EZ-Drive a7 NeXTSTEP f0 Linux/PA-RISC b 11 Hidden FAT12 56 Golden Bow a8 Darwin UFS f1 SpeedStor 12 Compaq diagnost 5c Priam Edisk a9 NetBSD f4 SpeedStor 14 Hidden FAT16 <3 61 SpeedStor ab Darwin boot f2 DOS secondary 16 Hidden FAT16 63 GNU HURD or Sys af HFS / HFS+ fb VMware VMFS 17 Hidden HPFS/NTF 64 Novell Netware b7 BSDI fs fc VMware VMKCORE 18 AST SmartSleep 65 Novell Netware b8 BSDI swap fd Linux raid auto 1b Hidden W95 FAT3 70 DiskSecure Mult bb Boot Wizard hid fe LANstep 1c Hidden W95 FAT3 75 PC/IX be Solaris boot ff BBT 1e Hidden W95 FAT1 80 Old Minix I did make a start on defining this for the XML: /* * XXX: these are basically partition types. * * fdisk has a bazillion partition ID types parted has * practically none, and splits the * info across 3 * different attributes. * * So this is a semi-generic set */ typedef enum { VIR_STORAGE_VOL_DISK_NONE = 0, VIR_STORAGE_VOL_DISK_LINUX, VIR_STORAGE_VOL_DISK_FAT16, VIR_STORAGE_VOL_DISK_FAT32, VIR_STORAGE_VOL_DISK_LINUX_SWAP, VIR_STORAGE_VOL_DISK_LINUX_LVM, VIR_STORAGE_VOL_DISK_LINUX_RAID, VIR_STORAGE_VOL_DISK_EXTENDED, VIR_STORAGE_VOL_DISK_LAST, } virStorageVolFormatDisk; VIR_ENUM_DECL(virStorageVolFormatDisk) but then storage_backend_disk never made use of this information I'm not sure how important this is, but if we do want to make use of 'format' for the disk pool, this partition types are what I'd map it too. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 01/15/2015 10:43 AM, Daniel P. Berrange wrote:
I looked through some history and didn't see anywhere that supplied name must match partition name was enforced, but there's a lot of code motion and new features that may cloud the history. Perhaps it's the assumption true in so many other pools that 'name' is what was used to create the file, directory, lv, etc. There is code that fills in the name if not provided with the partition name (which occurs after Refresh, reload, restart).
Looking back some more, it seems I did not ever enforce it - I just left it as something users had to set the "right way" by convention. ie I just expected callers to always use the right "sdaNNN" value
So then should this morph into a fix which causes a failure if a matching name wasn't passed? Or should we "be nice", "fix it" to whatever mkpart generates, and not fail? The check could be done in virStorageBackendDiskMakeDataVol which refresh, reload, restart already uses in order to assign partition name (eg, tmp) to vol->name. The downside of the fail case in that code is we'll need to delete the partition that just created - something that isn't already done in any of the failure cases. Trying to compare the name prior to creation against the assumed next partition # means we'll need to create an algorithm that finds all the partition numbers and figures out what the next one will be. They're not necessarily "in order". As for the target.format (<target...> <format type='linux'> </target>) I'm inclined to just let it be. Although it does have one use - since "extended" is a valid virStorageVolFormatDisk value, one could create an extended partition before all the primary ones are created. John
I'm not quite sure how one could enforce a name given that parted creates the partition name. One would have to know the device name of the pool and the numerical sequence that parted would use (easy perhaps know that perhaps partitions 1 & 2 exist that 3 will be next). However, there's not necessarily a guarantee of that is there?
For MS-DOS partition tables the partition entry number directly corresponds to the device name suffix. eg partition 3 corresponds to /dev/sda3 always and is happy to be sparse. eg if you create 3 partitions and then delete partition 2, you'll end up with /dev/sda1 and /dev/sda3
Testing with parted the same seems to be true of GPT tables too.
Reading the libvirt docs - the implication is that the volume "<name>" must only be unique across the pool, so the following is "valid":
virsh vol-create-as disk-pool vol-linux --format linux 1G
So without assuming/describing the naming scheme of the underlying parted how do we enforce that the name (vol-linux) is the same as what parted generates? Or how do we enforce that the name provided ends up being the same as the partition created? It's kind of a chicken/egg issue.
Well with disk partitions we know the device name
eg the pool knows we're operating on '/dev/sda', so when we create it can we validate that the volume name is '/dev/sdaNNN' by kjust doing a string prefix match
Not much is done with the target.format in the backend even though it's described as providing the "partition type". When not provided on the command line like above it defaults to 'none'. Other than for dos label pools that would need to have extended partition before creating a logical partition, there's not much "use" for the field other than to perhaps store "something" for "someone" (until refresh, reload, restart loses it).
When you create a partition you can specify filesystem type for that partition, even though it is somewhat redundant as it is not directly connected to the filesystem that you actually mkfs. eg this list of types from fdisk
Command (m for help): l
0 Empty 24 NEC DOS 81 Minix / old Lin bf Solaris 1 FAT12 27 Hidden NTFS Win 82 Linux swap / So c1 DRDOS/sec (FAT- 2 XENIX root 39 Plan 9 83 Linux c4 DRDOS/sec (FAT- 3 XENIX usr 3c PartitionMagic 84 OS/2 hidden C: c6 DRDOS/sec (FAT- 4 FAT16 <32M 40 Venix 80286 85 Linux extended c7 Syrinx 5 Extended 41 PPC PReP Boot 86 NTFS volume set da Non-FS data 6 FAT16 42 SFS 87 NTFS volume set db CP/M / CTOS / . 7 HPFS/NTFS/exFAT 4d QNX4.x 88 Linux plaintext de Dell Utility 8 AIX 4e QNX4.x 2nd part 8e Linux LVM df BootIt 9 AIX bootable 4f QNX4.x 3rd part 93 Amoeba e1 DOS access a OS/2 Boot Manag 50 OnTrack DM 94 Amoeba BBT e3 DOS R/O b W95 FAT32 51 OnTrack DM6 Aux 9f BSD/OS e4 SpeedStor c W95 FAT32 (LBA) 52 CP/M a0 IBM Thinkpad hi eb BeOS fs e W95 FAT16 (LBA) 53 OnTrack DM6 Aux a5 FreeBSD ee GPT f W95 Ext'd (LBA) 54 OnTrackDM6 a6 OpenBSD ef EFI (FAT-12/16/ 10 OPUS 55 EZ-Drive a7 NeXTSTEP f0 Linux/PA-RISC b 11 Hidden FAT12 56 Golden Bow a8 Darwin UFS f1 SpeedStor 12 Compaq diagnost 5c Priam Edisk a9 NetBSD f4 SpeedStor 14 Hidden FAT16 <3 61 SpeedStor ab Darwin boot f2 DOS secondary 16 Hidden FAT16 63 GNU HURD or Sys af HFS / HFS+ fb VMware VMFS 17 Hidden HPFS/NTF 64 Novell Netware b7 BSDI fs fc VMware VMKCORE 18 AST SmartSleep 65 Novell Netware b8 BSDI swap fd Linux raid auto 1b Hidden W95 FAT3 70 DiskSecure Mult bb Boot Wizard hid fe LANstep 1c Hidden W95 FAT3 75 PC/IX be Solaris boot ff BBT 1e Hidden W95 FAT1 80 Old Minix
I did make a start on defining this for the XML:
/* * XXX: these are basically partition types. * * fdisk has a bazillion partition ID types parted has * practically none, and splits the * info across 3 * different attributes. * * So this is a semi-generic set */ typedef enum { VIR_STORAGE_VOL_DISK_NONE = 0, VIR_STORAGE_VOL_DISK_LINUX, VIR_STORAGE_VOL_DISK_FAT16, VIR_STORAGE_VOL_DISK_FAT32, VIR_STORAGE_VOL_DISK_LINUX_SWAP, VIR_STORAGE_VOL_DISK_LINUX_LVM, VIR_STORAGE_VOL_DISK_LINUX_RAID, VIR_STORAGE_VOL_DISK_EXTENDED, VIR_STORAGE_VOL_DISK_LAST, } virStorageVolFormatDisk; VIR_ENUM_DECL(virStorageVolFormatDisk)
but then storage_backend_disk never made use of this information
I'm not sure how important this is, but if we do want to make use of 'format' for the disk pool, this partition types are what I'd map it too.
Regards, Daniel
participants (2)
-
Daniel P. Berrange -
John Ferlan