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:

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:

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 think patch 3 could go ahead of this - introducing the save function first so it's ready to be used by following patches. While this being separate is nice/simple - it seems patch 4 & 7 should be merged with this 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:

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:

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: