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

Storage state driver directories initialization needs to be modified to become more generic. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733 --- src/storage/storage_driver.c | 52 ++++++++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 23 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index e088ffa..9bd93d2 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -154,54 +154,60 @@ storageStateInitialize(bool privileged, virStateInhibitCallback callback ATTRIBUTE_UNUSED, void *opaque ATTRIBUTE_UNUSED) { - char *base = NULL; + int ret = -1; + char *configdir = NULL; + char *rundir = NULL; if (VIR_ALLOC(driver) < 0) - return -1; + return ret; if (virMutexInit(&driver->lock) < 0) { VIR_FREE(driver); - return -1; + return ret; } storageDriverLock(); if (privileged) { - if (VIR_STRDUP(base, SYSCONFDIR "/libvirt") < 0) + if (VIR_STRDUP(driver->configDir, + SYSCONFDIR "/libvirt/storage") < 0 || + VIR_STRDUP(driver->autostartDir, + SYSCONFDIR "/libvirt/storage/autostart") < 0 || + VIR_STRDUP(driver->stateDir, + LOCALSTATEDIR "/run/libvirt/storage") < 0) goto error; } else { - base = virGetUserConfigDirectory(); - if (!base) + configdir = virGetUserConfigDirectory(); + rundir = virGetUserRuntimeDirectory(); + if (!(configdir && rundir)) + goto error; + + if ((virAsprintf(&driver->configDir, + "%s/storage", configdir) < 0) || + (virAsprintf(&driver->autostartDir, + "%s/storage", configdir) < 0) || + (virAsprintf(&driver->stateDir, + "%s/storage/run", rundir) < 0)) goto error; } driver->privileged = privileged; - /* - * Configuration paths are either $USER_CONFIG_HOME/libvirt/storage/... - * (session) or /etc/libvirt/storage/... (system). - */ - if (virAsprintf(&driver->configDir, - "%s/storage", base) == -1) - goto error; - - if (virAsprintf(&driver->autostartDir, - "%s/storage/autostart", base) == -1) - goto error; - - VIR_FREE(base); - if (virStoragePoolLoadAllConfigs(&driver->pools, driver->configDir, driver->autostartDir) < 0) goto error; storageDriverUnlock(); - return 0; + + ret = 0; + cleanup: + VIR_FREE(configdir); + VIR_FREE(rundir); + return ret; error: - VIR_FREE(base); storageDriverUnlock(); storageStateCleanup(); - return -1; + goto cleanup; } /** -- 1.9.3

The update was originally part of the storageDriverAutostart function, but the pools have to be checked earlier during initialization phase, so the 'checkPool' block has been moved to storagePoolUpdateAllState. Prior to this update virStoragePoolLoadAllConfigs and virStoragePoolLoadAllState functions gather all existing pool in the system, so first it is necessary to filter out the ones that are inactive (only config XML found), then determine storage backends for the rest of the pools and run checkPool on each one of them to update their 'active' property. After update, pools have to be refreshed, otherwise the list of volumes stays empty. Once again we need the connection, but all the storage backends ignore this argument except for RBD, however RBD doesn't support 'checkPool' callback, therefore it is safe to pass connection as NULL pointer. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733 --- src/storage/storage_driver.c | 73 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 61 insertions(+), 12 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index d09acce..2899521 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -75,6 +75,64 @@ static void storageDriverUnlock(void) } static void +storagePoolUpdateAllState(void) +{ + size_t i; + bool active = false; + + for (i = 0; i < driver->pools.count; i++) { + virStoragePoolObjPtr pool = driver->pools.objs[i]; + virStorageBackendPtr backend; + + virStoragePoolObjLock(pool); + if (!virStoragePoolObjIsActive(pool)) { + virStoragePoolObjUnlock(pool); + continue; + } + + if ((backend = virStorageBackendForType(pool->def->type)) == NULL) { + VIR_ERROR(_("Missing backend %d"), pool->def->type); + virStoragePoolObjUnlock(pool); + continue; + } + + /* Backends which do not support 'checkPool' are considered + * inactive by default. + */ + if (backend->checkPool && + backend->checkPool(pool, &active) < 0) { + virErrorPtr err = virGetLastError(); + VIR_ERROR(_("Failed to initialize storage pool '%s': %s"), + pool->def->name, err ? err->message : + _("no error message found")); + virStoragePoolObjUnlock(pool); + continue; + } + + /* We can pass NULL as connection, most backends do not use + * it anyway, but if they do and fail, we want to log error and + * continue with other pools. + */ + if (active) { + virStoragePoolObjClearVols(pool); + if (backend->refreshPool(NULL, pool) < 0) { + virErrorPtr err = virGetLastError(); + if (backend->stopPool) + backend->stopPool(NULL, pool); + VIR_ERROR(_("Failed to restart storage pool '%s': %s"), + pool->def->name, err ? err->message : + _("no error message found")); + virStoragePoolObjUnlock(pool); + continue; + } + } + + pool->active = active; + virStoragePoolObjUnlock(pool); + } +} + +static void storageDriverAutostart(void) { size_t i; @@ -99,18 +157,7 @@ storageDriverAutostart(void) continue; } - if (backend->checkPool && - backend->checkPool(pool, &started) < 0) { - virErrorPtr err = virGetLastError(); - VIR_ERROR(_("Failed to initialize storage pool '%s': %s"), - pool->def->name, err ? err->message : - _("no error message found")); - virStoragePoolObjUnlock(pool); - continue; - } - - if (!started && - pool->autostart && + if (pool->autostart && !virStoragePoolObjIsActive(pool)) { if (backend->startPool && backend->startPool(conn, pool) < 0) { @@ -207,6 +254,8 @@ storageStateInitialize(bool privileged, driver->autostartDir) < 0) goto error; + storagePoolUpdateAllState(); + storageDriverUnlock(); ret = 0; -- 1.9.3

On 03/24/2015 06:06 AM, Erik Skultety wrote:

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

I had started down this path before for a different bug/issue: http://www.redhat.com/archives/libvir-list/2015-January/msg00465.html but abandoned the stateDir because it was felt it wasn't necessary. I recall running into a few "interesting" issues with stateDir including fixing one issue seen during testing that didn't hit the list. Good news is I still have the patches in a branch somewhere if you're interested. 1 & 2 are on list... The 3rd one in the archive was a solution to the particular problem - that was rejected and a different solution was pushed. In any case, I do suggest looking at 1 & 2, plus I can send you an adjustment to 1 to resolve some condition I ran into, but cannot recall the details. John

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);

On 03/24/2015 06:06 AM, Erik Skultety wrote:

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(-)

I'm not convinced we can remove 'conn'. Something about backend or backward compatibility. Since it's not used by any backend anyway, there's (more or less) no harm in keeping it. Perhaps others can pipe in on this... John

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 :

On 03/24/2015 06:06 AM, Erik Skultety wrote:

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(-)

In a bit of bikeshedding - this patch does a couple of things and could be split into a couple of patches... Ironically you bundled things together here, but separated them for the stateDir changes (patches 2, 4, & 7). first one just creates DefFormatBuf and has DefFormat call it second one creates the virStoragePoolSaveXML, has the config code use it third one creates virStoragePoolSaveStatus which use the new API's

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

static int - only this function cares; otherwise we have to ensure the 'buf' and 'def' arguments aren't NULL in the .h file... This way we control having non NULL values being presented.

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

static int virStoragePoolSaveXML(const char *path, (since it won't be the "configFile" only...)

+{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + int ret = -1; + + virUUIDFormat(def->uuid, uuidstr); + ret = virXMLSaveFile(configFile,

s/configFile/path

+ virXMLPickShellSafeComment(def->name, uuidstr), + "pool-edit", xml); + + return ret; +} + + +int virStoragePoolSaveStatus(const char *stateFile, + virStoragePoolDefPtr def)

int virStoragePoolSaveStatus(...

+{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + int ret = -1; + char *xml; + + virBufferAddLit(&buf, "<poolstatus>\n");

poolstate ?

+ virBufferAdjustIndent(&buf, 2); + + if (virStoragePoolDefFormatBuf(&buf, def) < 0) + goto error; + + virBufferAdjustIndent(&buf, -2); + virBufferAddLit(&buf, "</poolstatus>\n");

poolstate ?

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

Unnecessary if static int Obvious if this gets split into 3 patches, then there's a few changes to the following stuff changes too John

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;

On 03/24/2015 06:06 AM, Erik Skultety wrote:

These functions operate exactly the same as virStoragePoolLoadAllConfigs.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733 --- src/conf/storage_conf.c | 90 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/storage_conf.h | 7 ++++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 11 ++++++ 4 files changed, 109 insertions(+)

diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 5d984f3..b158e30 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1863,6 +1863,96 @@ virStoragePoolObjLoad(virStoragePoolObjListPtr pools, }

+virStoragePoolObjPtr +virStoragePoolLoadState(virStoragePoolObjListPtr pools, + const char *stateDir, + const char *name) +{ + char *stateFile = NULL; + virStoragePoolDefPtr def = NULL; + virStoragePoolObjPtr pool = NULL; + xmlDocPtr xml = NULL; + xmlXPathContextPtr ctxt = NULL; + xmlNodePtr node = NULL; + + if (!(stateFile = virFileBuildPath(stateDir, name, ".xml"))) + goto cleanup; + + if (!(xml = virXMLParseCtxt(stateFile, NULL, _("(pool status)"), &ctxt)))

s/status/state ?

+ goto cleanup; + + if (!(node = virXPathNode("//pool", ctxt))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not find any 'pool' element in status file"));

s/status/state ?

+ goto cleanup; + } + + ctxt->node = node; + if (!(def = virStoragePoolDefParseXML(ctxt))) + goto cleanup; + + if (!STREQ(name, def->name)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Storage pool status file '%s' does not match "

s/status/state

+ "pool name '%s'"), + stateFile, def->name);

Coverity found - if we jump to cleanup here, def is leaked.

+ goto cleanup; + } + + /* create the object */ + if (!(pool = virStoragePoolObjAssignDef(pools, def))) + goto cleanup;

Add the def = NULL here since success ObjAssignDef consumes it

+ + /* XXX: future handling of some additional usefull status data, + * for now, if a status file for a pool exists, the pool will be marked + * as active + */ + + pool->active = 1; + + cleanup: + VIR_FREE(stateFile); + xmlFree(xml); + xmlXPathFreeContext(ctxt);

Then add here: virStoragePoolDefFree(def);

+ return pool; +} + + +int +virStoragePoolLoadAllState(virStoragePoolObjListPtr pools, + const char *stateDir) +{ + DIR *dir; + struct dirent *entry; + int ret = -1; + + if (!(dir = opendir(stateDir))) { + if (errno == ENOENT) + return 0; + + virReportSystemError(errno, _("Failed to open dir '%s'"), stateDir); + return -1; + } + + while ((ret = virDirRead(dir, &entry, stateDir)) > 0) { + virStoragePoolObjPtr pool; + + if (entry->d_name[0] == '.') + continue; + + if (!virFileStripSuffix(entry->d_name, ".xml")) + continue; + + if (!(pool = virStoragePoolLoadState(pools, stateDir, entry->d_name))) + continue; + virStoragePoolObjUnlock(pool); + } + + closedir(dir); + return ret; +} + + int virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr pools, const char *configDir, diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 99b2f4a..1f84504 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -318,6 +318,13 @@ int virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr pools, const char *configDir, const char *autostartDir);

+int virStoragePoolLoadAllState(virStoragePoolObjListPtr pools, + const char *stateDir); + +virStoragePoolObjPtr +virStoragePoolLoadState(virStoragePoolObjListPtr pools, + const char *stateDir, + const char *name); virStoragePoolObjPtr virStoragePoolObjFindByUUID(virStoragePoolObjListPtr pools, const unsigned char *uuid); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 689a08f..9bc8de8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -798,6 +798,7 @@ virStoragePoolFormatFileSystemNetTypeToString; virStoragePoolFormatFileSystemTypeToString; virStoragePoolGetVhbaSCSIHostParent; virStoragePoolLoadAllConfigs; +virStoragePoolLoadAllState; virStoragePoolObjAssignDef; virStoragePoolObjClearVols; virStoragePoolObjDeleteDef; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 9bd93d2..d09acce 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -191,6 +191,17 @@ storageStateInitialize(bool privileged, } driver->privileged = privileged;

+ if (virFileMakePath(driver->stateDir) < 0) { + virReportError(errno, + _("cannot create directory %s"), + driver->stateDir); + goto error; + } + + if (virStoragePoolLoadAllState(&driver->pools, + driver->stateDir) < 0) + goto error; + if (virStoragePoolLoadAllConfigs(&driver->pools, driver->configDir, driver->autostartDir) < 0)

What about storageStateReload ? IOW the other place where virStoragePoolLoadAllConfigs is called. John

On 03/24/2015 06:06 AM, Erik Skultety wrote:

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

I still think this could be merged into 2 and 4... Commit message should indicate we're not creating a stateFile for transient storage pools which use the PoolCreateXML... Deleting the file occurs at PoolDestroy or PoolDelete time

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));

The 'stateFile' is leaked multiple times... Strange Coverity didn't pick up on it!

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);

Ironically we already started... So just because we cannot create the pool state file, we're failing startup, but the pool is left running on failure... You'll need to stop the pool... maybe even move to in between start and refresh... Both failures need to stop the pool, but you could combine into 1 if stmt.

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)

Ironically the VIR_INFO above here says "Shutting down...", but in reality it's already stopped.

pool->def = pool->newDef; pool->newDef = NULL; } + + if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, + ".xml")))

The args are unaligned. Futhermore, Coverity points out the pool = NULL a few lines back when pool->configFile == NULL, thus we'd have a bad dereference. We've already stopped the pool - so failing and leaving a stopped pool is unusual. So the question comes - do we attempt to delete the status file first and if any part of that fails, we cause failure or do we silently fail.

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

args are unaligned Again I'll point at the wrong tense used in the VIR_INFO message... We're not deleting - we deleted! Also like the destroy path - do we attempt the state file deletion first, then fail if that fails or do we silently just accept that we cannot delete the file. John

+ goto cleanup; + + unlink(stateFile); + VIR_FREE(stateFile); ret = 0;

cleanup: