The checkPool callback should be used when updating states of all pools during storageStateInitialize, not storageDriverAutostart, otherwise we can't start a domain which mounts a volume after daemons restarted. This is because qemuProcessReconnect is called earlier than storageDriverAutostart which marks the pool as active, so that the domain can mount a volume from this pool successfully. In order to fix this, conn attribute has to be discarded from the callback, because storageStateInitialize doesn't have a connection yet. (it's not used anyway) Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733 --- src/storage/storage_backend.h | 3 +-- src/storage/storage_backend_fs.c | 3 +-- src/storage/storage_backend_iscsi.c | 3 +-- src/storage/storage_backend_logical.c | 3 +-- src/storage/storage_backend_mpath.c | 3 +-- src/storage/storage_backend_scsi.c | 3 +-- src/storage/storage_backend_zfs.c | 3 +-- src/storage/storage_driver.c | 2 +- 8 files changed, 8 insertions(+), 15 deletions(-) diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 9f1db60..fd2451c 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -34,8 +34,7 @@ typedef char * (*virStorageBackendFindPoolSources)(virConnectPtr conn, const char *srcSpec, unsigned int flags); -typedef int (*virStorageBackendCheckPool)(virConnectPtr conn, - virStoragePoolObjPtr pool, +typedef int (*virStorageBackendCheckPool)(virStoragePoolObjPtr pool, bool *active); typedef int (*virStorageBackendStartPool)(virConnectPtr conn, virStoragePoolObjPtr pool); diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 35385db..d4d65bc 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -533,8 +533,7 @@ virStorageBackendFileSystemUnmount(virStoragePoolObjPtr pool) static int -virStorageBackendFileSystemCheck(virConnectPtr conn ATTRIBUTE_UNUSED, - virStoragePoolObjPtr pool, +virStorageBackendFileSystemCheck(virStoragePoolObjPtr pool, bool *isActive) { if (pool->def->type == VIR_STORAGE_POOL_DIR) { diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 079c767..497a71b 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -236,8 +236,7 @@ virStorageBackendISCSIFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, } static int -virStorageBackendISCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, - virStoragePoolObjPtr pool, +virStorageBackendISCSICheckPool(virStoragePoolObjPtr pool, bool *isActive) { char *session = NULL; diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 7ba8ded..11c5683 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -479,8 +479,7 @@ virStorageBackendLogicalFindPoolSources(virConnectPtr conn ATTRIBUTE_UNUSED, static int -virStorageBackendLogicalCheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, - virStoragePoolObjPtr pool, +virStorageBackendLogicalCheckPool(virStoragePoolObjPtr pool, bool *isActive) { *isActive = virFileExists(pool->def->target.path); diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index 44bcd60..971408a 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -245,8 +245,7 @@ virStorageBackendGetMaps(virStoragePoolObjPtr pool) } static int -virStorageBackendMpathCheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, - virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, +virStorageBackendMpathCheckPool(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, bool *isActive) { *isActive = virFileExists("/dev/mpath"); diff --git a/src/storage/storage_backend_scsi.c b/src/storage/storage_backend_scsi.c index 58e7e6d..66e0846 100644 --- a/src/storage/storage_backend_scsi.c +++ b/src/storage/storage_backend_scsi.c @@ -844,8 +844,7 @@ deleteVport(virConnectPtr conn, static int -virStorageBackendSCSICheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, - virStoragePoolObjPtr pool, +virStorageBackendSCSICheckPool(virStoragePoolObjPtr pool, bool *isActive) { char *path = NULL; diff --git a/src/storage/storage_backend_zfs.c b/src/storage/storage_backend_zfs.c index 9482706..cb2662a 100644 --- a/src/storage/storage_backend_zfs.c +++ b/src/storage/storage_backend_zfs.c @@ -41,8 +41,7 @@ VIR_LOG_INIT("storage.storage_backend_zfs"); static int -virStorageBackendZFSCheckPool(virConnectPtr conn ATTRIBUTE_UNUSED, - virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, +virStorageBackendZFSCheckPool(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, bool *isActive) { char *devpath; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index b95506f..64ea770 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -100,7 +100,7 @@ storageDriverAutostart(void) } if (backend->checkPool && - backend->checkPool(conn, pool, &started) < 0) { + backend->checkPool(pool, &started) < 0) { virErrorPtr err = virGetLastError(); VIR_ERROR(_("Failed to initialize storage pool '%s': %s"), pool->def->name, err ? err->message : -- 1.9.3

Before introducing necessary changes to storage_driver.c, first prepare our structures for storage state XML support. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733 --- src/conf/storage_conf.h | 1 + src/storage/storage_driver.c | 1 + 2 files changed, 2 insertions(+) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 4584075..8ccc947 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -293,6 +293,7 @@ struct _virStorageDriverState { char *configDir; char *autostartDir; + char *stateDir; bool privileged; }; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 64ea770..e088ffa 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -261,6 +261,7 @@ storageStateCleanup(void) VIR_FREE(driver->configDir); VIR_FREE(driver->autostartDir); + VIR_FREE(driver->stateDir); storageDriverUnlock(); virMutexDestroy(&driver->lock); VIR_FREE(driver); -- 1.9.3

This patch introduces virStoragePoolSaveStatus to properly format the status XML. It also adds virStoragePoolDefFormatBuf function, which alike in the network driver, formats the whole storage pool definition into a buffer that might have been previously modified by virStoragePoolSaveStatus to insert <poolstatus> element. The original virStoragePoolDefFormat function had to be modified accordingly to use virBuffer. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733 --- src/conf/storage_conf.c | 139 ++++++++++++++++++++++++++++++++++------------- src/conf/storage_conf.h | 6 +- src/libvirt_private.syms | 1 + 3 files changed, 107 insertions(+), 39 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index b070448..5d984f3 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1150,76 +1150,90 @@ virStoragePoolSourceFormat(virBufferPtr buf, } -char * -virStoragePoolDefFormat(virStoragePoolDefPtr def) +int +virStoragePoolDefFormatBuf(virBufferPtr buf, + virStoragePoolDefPtr def) { virStoragePoolOptionsPtr options; - virBuffer buf = VIR_BUFFER_INITIALIZER; - const char *type; char uuid[VIR_UUID_STRING_BUFLEN]; + const char *type; options = virStoragePoolOptionsForPoolType(def->type); if (options == NULL) - return NULL; + goto error; type = virStoragePoolTypeToString(def->type); if (!type) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("unexpected pool type")); - goto cleanup; + goto error; } - virBufferAsprintf(&buf, "<pool type='%s'>\n", type); - virBufferAdjustIndent(&buf, 2); - virBufferEscapeString(&buf, "<name>%s</name>\n", def->name); + virBufferAsprintf(buf, "<pool type='%s'>\n", type); + virBufferAdjustIndent(buf, 2); + virBufferEscapeString(buf, "<name>%s</name>\n", def->name); virUUIDFormat(def->uuid, uuid); - virBufferAsprintf(&buf, "<uuid>%s</uuid>\n", uuid); + virBufferAsprintf(buf, "<uuid>%s</uuid>\n", uuid); - virBufferAsprintf(&buf, "<capacity unit='bytes'>%llu</capacity>\n", + virBufferAsprintf(buf, "<capacity unit='bytes'>%llu</capacity>\n", def->capacity); - virBufferAsprintf(&buf, "<allocation unit='bytes'>%llu</allocation>\n", + virBufferAsprintf(buf, "<allocation unit='bytes'>%llu</allocation>\n", def->allocation); - virBufferAsprintf(&buf, "<available unit='bytes'>%llu</available>\n", + virBufferAsprintf(buf, "<available unit='bytes'>%llu</available>\n", def->available); - if (virStoragePoolSourceFormat(&buf, options, &def->source) < 0) - goto cleanup; + if (virStoragePoolSourceFormat(buf, options, &def->source) < 0) + goto error; /* RBD, Sheepdog, and Gluster devices are not local block devs nor * files, so they don't have a target */ if (def->type != VIR_STORAGE_POOL_RBD && def->type != VIR_STORAGE_POOL_SHEEPDOG && def->type != VIR_STORAGE_POOL_GLUSTER) { - virBufferAddLit(&buf, "<target>\n"); - virBufferAdjustIndent(&buf, 2); + virBufferAddLit(buf, "<target>\n"); + virBufferAdjustIndent(buf, 2); - virBufferEscapeString(&buf, "<path>%s</path>\n", def->target.path); + virBufferEscapeString(buf, "<path>%s</path>\n", def->target.path); - virBufferAddLit(&buf, "<permissions>\n"); - virBufferAdjustIndent(&buf, 2); - virBufferAsprintf(&buf, "<mode>0%o</mode>\n", + virBufferAddLit(buf, "<permissions>\n"); + virBufferAdjustIndent(buf, 2); + virBufferAsprintf(buf, "<mode>0%o</mode>\n", def->target.perms.mode); - virBufferAsprintf(&buf, "<owner>%d</owner>\n", + virBufferAsprintf(buf, "<owner>%d</owner>\n", (int) def->target.perms.uid); - virBufferAsprintf(&buf, "<group>%d</group>\n", + virBufferAsprintf(buf, "<group>%d</group>\n", (int) def->target.perms.gid); - virBufferEscapeString(&buf, "<label>%s</label>\n", + virBufferEscapeString(buf, "<label>%s</label>\n", def->target.perms.label); - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</permissions>\n"); - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</target>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</permissions>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</target>\n"); } - virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</pool>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</pool>\n"); + + return 0; + + error: + return -1; +} + +char * +virStoragePoolDefFormat(virStoragePoolDefPtr def) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + + if (virStoragePoolDefFormatBuf(&buf, def) < 0) + goto error; if (virBufferCheckError(&buf) < 0) - goto cleanup; + goto error; return virBufferContentAndReset(&buf); - cleanup: + error: virBufferFreeAndReset(&buf); return NULL; } @@ -1899,11 +1913,60 @@ virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr pools, return ret; } + +static int virStoragePoolSaveXML(const char *configFile, + virStoragePoolDefPtr def, + const char *xml) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + int ret = -1; + + virUUIDFormat(def->uuid, uuidstr); + ret = virXMLSaveFile(configFile, + virXMLPickShellSafeComment(def->name, uuidstr), + "pool-edit", xml); + + return ret; +} + + +int virStoragePoolSaveStatus(const char *stateFile, + virStoragePoolDefPtr def) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + int ret = -1; + char *xml; + + virBufferAddLit(&buf, "<poolstatus>\n"); + virBufferAdjustIndent(&buf, 2); + + if (virStoragePoolDefFormatBuf(&buf, def) < 0) + goto error; + + virBufferAdjustIndent(&buf, -2); + virBufferAddLit(&buf, "</poolstatus>\n"); + + if (virBufferCheckError(&buf) < 0) + goto error; + + if (!(xml = virBufferContentAndReset(&buf))) + goto error; + + if (virStoragePoolSaveXML(stateFile, def, xml)) + goto error; + + ret = 0; + + error: + VIR_FREE(xml); + return ret; +} + + int virStoragePoolSaveConfig(const char *configFile, virStoragePoolDefPtr def) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; char *xml; int ret = -1; @@ -1913,12 +1976,12 @@ virStoragePoolSaveConfig(const char *configFile, return -1; } - virUUIDFormat(def->uuid, uuidstr); - ret = virXMLSaveFile(configFile, - virXMLPickShellSafeComment(def->name, uuidstr), - "pool-edit", xml); - VIR_FREE(xml); + if (virStoragePoolSaveXML(configFile, def, xml)) + goto cleanup; + ret = 0; + cleanup: + VIR_FREE(xml); return ret; } diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 8ccc947..99b2f4a 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -345,6 +345,8 @@ virStoragePoolDefPtr virStoragePoolDefParseFile(const char *filename); virStoragePoolDefPtr virStoragePoolDefParseNode(xmlDocPtr xml, xmlNodePtr root); char *virStoragePoolDefFormat(virStoragePoolDefPtr def); +int virStoragePoolDefFormatBuf(virBufferPtr buf, + virStoragePoolDefPtr def); typedef enum { /* do not require volume capacity at all */ @@ -372,7 +374,9 @@ virStoragePoolObjPtr virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, virStoragePoolDefPtr def); -int virStoragePoolSaveConfig(const char *configDir, +int virStoragePoolSaveStatus(const char *stateFile, + virStoragePoolDefPtr def); +int virStoragePoolSaveConfig(const char *configFile, virStoragePoolDefPtr def); int virStoragePoolObjSaveDef(virStorageDriverStatePtr driver, virStoragePoolObjPtr pool, diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 33222f0..689a08f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -811,6 +811,7 @@ virStoragePoolObjRemove; virStoragePoolObjSaveDef; virStoragePoolObjUnlock; virStoragePoolSaveConfig; +virStoragePoolSaveStatus; virStoragePoolSourceAdapterTypeFromString; virStoragePoolSourceAdapterTypeToString; virStoragePoolSourceClear; -- 1.9.3

On 03/24/2015 06:06 AM, Erik Skultety wrote: Could we fix the comment above this too! s/QEmu daemon/Storage Driver/ Braver than I was - although I prefer your method because it's clearer. I think if this is merged into patch 2 - it'll be easier to follow John

On 03/24/2015 06:06 AM, Erik Skultety wrote: s/status/state ? s/status/state ? s/status/state Coverity found - if we jump to cleanup here, def is leaked. Add the def = NULL here since success ObjAssignDef consumes it Then add here: virStoragePoolDefFree(def); What about storageStateReload ? IOW the other place where virStoragePoolLoadAllConfigs is called. John

On 03/24/2015 06:06 AM, Erik Skultety wrote: s/update/checkPool s/have/need move the checkPool block into storageDriverInitialize. This ensures activating pools earlier allowing domains using volumes from a pool to be able to find the volume during qemuProcessReconnect ... [e.g. picking up the commit message from 1/7 into here...] I think you're trying to indicate prior to this function, the virStoragePoolLoadAllState and virStoragePoolLoadAllConfigs functions have generated a list all the existing configs (active or inactive)... The purpose of this function is to take any that were deemed active and ensure that the checkPool function is called along with the refreshPool function in order to ensure that the volumes within the pool are present when the domain reconnection occurs. s/update/checkPool This one I'm struggling with... For file based pools there's mostly truth to the claim in the commit message; however, iSCSI and SCSI pools are a bit different. Perhaps along the same issues as rbd and even gluster. I guess I just have to research a bit more of any edge conditions and make sure the changes actually "work" for my iSCSI config... My fc_host config is a bit trickier since I'm sharing the host with someone else who I have to coordinate with (so I'll wait for the v2 patches before going down that path). Redundant check with DriverAutostart and since that's the only place that calls us.... I think dropping the VIR_ERROR is fine... In fact I think: if ((backend = virStorageBackendForType(pool->def->type)) == NULL || !backend->checkPool) { virStoragepoolObjUnlock(pool); continue; } Then you can assume that whatever follows has a 'checkPool' function See I understand why patch 1 removes the conn; however, I'm still not convinced we can do that - let's see if anyone else says anything. Could add a : if (!active) { virStoragePoolObjUnlock(pool); continue; } then the rest is just inline... (ahem) I assume this only matters is active true... John

Once we introduced virStoragePoolSaveStatus function, create a status XML every time a pool is created (virStoragePoolCreate, storageDriverAutostart) Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733 --- src/storage/storage_driver.c | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 2899521..4ce3d34 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -136,6 +136,7 @@ static void storageDriverAutostart(void) { size_t i; + char *stateFile = NULL; virConnectPtr conn = NULL; /* XXX Remove hardcoding of QEMU URI */ @@ -183,6 +184,12 @@ storageDriverAutostart(void) virStoragePoolObjUnlock(pool); continue; } + + if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, ".xml"))) + continue; + + ignore_value(virStoragePoolSaveStatus(stateFile, pool->def)); pool->active = 1; } virStoragePoolObjUnlock(pool); @@ -812,6 +819,7 @@ storagePoolCreate(virStoragePoolPtr obj, virStoragePoolObjPtr pool; virStorageBackendPtr backend; int ret = -1; + char *stateFile = NULL; virCheckFlags(0, -1); @@ -840,11 +848,21 @@ storagePoolCreate(virStoragePoolPtr obj, goto cleanup; } + /* save pool state */ + if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, ".xml"))) + goto cleanup; + + if ((ret = virStoragePoolSaveStatus(stateFile, + pool->def)) < 0) + goto cleanup; + VIR_INFO("Starting up storage pool '%s'", pool->def->name); pool->active = 1; ret = 0; cleanup: + VIR_FREE(stateFile); virStoragePoolObjUnlock(pool); return ret; } @@ -889,6 +907,7 @@ storagePoolDestroy(virStoragePoolPtr obj) { virStoragePoolObjPtr pool; virStorageBackendPtr backend; + char *stateFile = NULL; int ret = -1; storageDriverLock(); @@ -937,6 +956,15 @@ storagePoolDestroy(virStoragePoolPtr obj) pool->def = pool->newDef; pool->newDef = NULL; } + + if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, + ".xml"))) + goto cleanup; + + unlink(stateFile); + VIR_FREE(stateFile); + ret = 0; cleanup: @@ -952,6 +980,7 @@ storagePoolDelete(virStoragePoolPtr obj, { virStoragePoolObjPtr pool; virStorageBackendPtr backend; + char *stateFile = NULL; int ret = -1; if (!(pool = virStoragePoolObjFromStoragePool(obj))) @@ -985,6 +1014,14 @@ storagePoolDelete(virStoragePoolPtr obj, if (backend->deletePool(obj->conn, pool, flags) < 0) goto cleanup; VIR_INFO("Deleting storage pool '%s'", pool->def->name); + + if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, + ".xml"))) + goto cleanup; + + unlink(stateFile); + VIR_FREE(stateFile); ret = 0; cleanup: -- 1.9.3