[libvirt] [PATCH 0/6] Add support for storage pool state XML
Patches add necessary changes to add support for storage pool state XMLs and thus resolve https://bugzilla.redhat.com/show_bug.cgi?id=1177733 Erik Skultety (6): conf: Introduce virStoragePoolDefFormatBuf conf: Introduce virStoragePoolSaveXML conf: Introduce virStoragePoolSaveState storage: Add support for storage pool state XML conf: Introduce virStoragePoolLoadAllState && virStoragePoolLoadState storage: Introduce storagePoolUpdateAllState function src/conf/storage_conf.c | 233 ++++++++++++++++++++++++++++++++++++------- src/conf/storage_conf.h | 12 ++- src/libvirt_private.syms | 2 + src/storage/storage_driver.c | 188 +++++++++++++++++++++++++++------- 4 files changed, 358 insertions(+), 77 deletions(-) -- 1.9.3
When modifying config/status XML, it might be handy to include some additional XML elements (e.g. <poolstatus>). In order to do so, introduce new formatting function virStoragePoolDefFormatBuf and make virStoragePoolDefFormat call it. --- src/conf/storage_conf.c | 78 +++++++++++++++++++++++++++++-------------------- 1 file changed, 46 insertions(+), 32 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index b070448..a8e9876 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1150,76 +1150,90 @@ virStoragePoolSourceFormat(virBufferPtr buf, } -char * -virStoragePoolDefFormat(virStoragePoolDefPtr def) +static 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; } -- 1.9.3
On Thu, Apr 02, 2015 at 12:10:35PM +0200, Erik Skultety wrote:
When modifying config/status XML, it might be handy to include some additional XML elements (e.g. <poolstatus>). In order to do so, introduce new formatting function virStoragePoolDefFormatBuf and make virStoragePoolDefFormat call it. --- src/conf/storage_conf.c | 78 +++++++++++++++++++++++++++++-------------------- 1 file changed, 46 insertions(+), 32 deletions(-)
ACK with one nit:
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index b070448..a8e9876 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1150,76 +1150,90 @@ virStoragePoolSourceFormat(virBufferPtr buf,
--- 8< ---
- virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</pool>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</pool>\n"); + + return 0; + + error: + return -1;
The error label is not necessary - just return -1; directly (unless you plan to add some cleanup code there in the near future). Jan
ACK with one nit:
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index b070448..a8e9876 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1150,76 +1150,90 @@ virStoragePoolSourceFormat(virBufferPtr buf,
--- 8< ---
- virBufferAdjustIndent(&buf, -2); - virBufferAddLit(&buf, "</pool>\n"); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</pool>\n"); + + return 0; + + error: + return -1;
The error label is not necessary - just return -1; directly (unless you plan to add some cleanup code there in the near future).
Jan
Thanks, now pushed. Erik
Make XML definition saving more generic by moving the common code into virStoragePoolSaveXML and leave case specific code to PoolSave{Status,Config,...} functions. --- src/conf/storage_conf.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index a8e9876..73b937e 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1913,11 +1913,25 @@ virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr pools, return ret; } + +static int virStoragePoolSaveXML(const char *path, + virStoragePoolDefPtr def, + const char *xml) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + int ret = -1; + + virUUIDFormat(def->uuid, uuidstr); + ret = virXMLSaveFile(path, + virXMLPickShellSafeComment(def->name, uuidstr), + "pool-edit", xml); + + return ret; +} int virStoragePoolSaveConfig(const char *configFile, virStoragePoolDefPtr def) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; char *xml; int ret = -1; @@ -1927,12 +1941,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; } -- 1.9.3
On Thu, Apr 02, 2015 at 12:10:36 +0200, Erik Skultety wrote:
Make XML definition saving more generic by moving the common code into virStoragePoolSaveXML and leave case specific code to PoolSave{Status,Config,...} functions. --- src/conf/storage_conf.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index a8e9876..73b937e 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1913,11 +1913,25 @@ virStoragePoolLoadAllConfigs(virStoragePoolObjListPtr pools, return ret; }
+ +static int virStoragePoolSaveXML(const char *path, + virStoragePoolDefPtr def, + const char *xml) +{ + char uuidstr[VIR_UUID_STRING_BUFLEN]; + int ret = -1; + + virUUIDFormat(def->uuid, uuidstr); + ret = virXMLSaveFile(path, + virXMLPickShellSafeComment(def->name, uuidstr), + "pool-edit", xml); + + return ret; +} int
Missing empty lines between functions.
virStoragePoolSaveConfig(const char *configFile, virStoragePoolDefPtr def) { - char uuidstr[VIR_UUID_STRING_BUFLEN]; char *xml; int ret = -1;
@@ -1927,12 +1941,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; }
ACK if you add at least one new line between virStoragePoolSaveConfig and virStoragePoolSaveXML Peter
Properly format storage pool state XML. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733 --- src/conf/storage_conf.c | 35 +++++++++++++++++++++++++++++++++++ src/conf/storage_conf.h | 4 +++- src/libvirt_private.syms | 1 + 3 files changed, 39 insertions(+), 1 deletion(-) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 73b937e..ee564f2 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1928,6 +1928,41 @@ static int virStoragePoolSaveXML(const char *path, return ret; } + + +int virStoragePoolSaveState(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) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 4584075..da378b7 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -371,7 +371,9 @@ virStoragePoolObjPtr virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, virStoragePoolDefPtr def); -int virStoragePoolSaveConfig(const char *configDir, +int virStoragePoolSaveState(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 9e71b1a..56acb01 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -813,6 +813,7 @@ virStoragePoolObjRemove; virStoragePoolObjSaveDef; virStoragePoolObjUnlock; virStoragePoolSaveConfig; +virStoragePoolSaveState; virStoragePoolSourceAdapterTypeFromString; virStoragePoolSourceAdapterTypeToString; virStoragePoolSourceClear; -- 1.9.3
On 04/02/2015 06:10 AM, Erik Skultety wrote:
Properly format storage pool state XML.
Your previous patch 3/7 had a better commit message: Introduce virStoragePoolSaveStatus to properly format the status XML in the same manner as virStoragePoolDefFormat, except for adding a <poolstatus> ... </poolstatus> around the definition. This is similar to virNetworkObjFormat used to save the live/active network information.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733
By itself - no it doesn't solve the bug (same for 4 and 5)
--- src/conf/storage_conf.c | 35 +++++++++++++++++++++++++++++++++++ src/conf/storage_conf.h | 4 +++- src/libvirt_private.syms | 1 + 3 files changed, 39 insertions(+), 1 deletion(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 73b937e..ee564f2 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1928,6 +1928,41 @@ static int virStoragePoolSaveXML(const char *path,
return ret; } + + +int virStoragePoolSaveState(const char *stateFile, + virStoragePoolDefPtr def)
Again it's int virStorage... ACK with those adjustments. John FYI: Coverity is happy with all 6 patches...
+{ + 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) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 4584075..da378b7 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -371,7 +371,9 @@ virStoragePoolObjPtr virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, virStoragePoolDefPtr def);
-int virStoragePoolSaveConfig(const char *configDir, +int virStoragePoolSaveState(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 9e71b1a..56acb01 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -813,6 +813,7 @@ virStoragePoolObjRemove; virStoragePoolObjSaveDef; virStoragePoolObjUnlock; virStoragePoolSaveConfig; +virStoragePoolSaveState; virStoragePoolSourceAdapterTypeFromString; virStoragePoolSourceAdapterTypeToString; virStoragePoolSourceClear;
Your previous patch 3/7 had a better commit message:
Introduce virStoragePoolSaveStatus to properly format the status XML in the same manner as virStoragePoolDefFormat, except for adding a <poolstatus> ... </poolstatus> around the definition. This is similar to virNetworkObjFormat used to save the live/active network information.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733
By itself - no it doesn't solve the bug (same for 4 and 5)
--- src/conf/storage_conf.c | 35 +++++++++++++++++++++++++++++++++++ src/conf/storage_conf.h | 4 +++- src/libvirt_private.syms | 1 + 3 files changed, 39 insertions(+), 1 deletion(-)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 73b937e..ee564f2 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1928,6 +1928,41 @@ static int virStoragePoolSaveXML(const char *path,
return ret; } + + +int virStoragePoolSaveState(const char *stateFile, + virStoragePoolDefPtr def)
Again it's int virStorage...
ACK with those adjustments.
John
FYI: Coverity is happy with all 6 patches...
Thank you, I changed the commit message, fixed the issues and pushed with Jan's little note in mind (I'll push that one as trivial). Erik
On Thu, Apr 02, 2015 at 12:10:37PM +0200, Erik Skultety wrote:
Properly format storage pool state XML.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733 --- src/conf/storage_conf.c | 35 +++++++++++++++++++++++++++++++++++ src/conf/storage_conf.h | 4 +++- src/libvirt_private.syms | 1 + 3 files changed, 39 insertions(+), 1 deletion(-)
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index 4584075..da378b7 100644 --- a/src/conf/storage_conf.h +++ b/src/conf/storage_conf.h @@ -371,7 +371,9 @@ virStoragePoolObjPtr virStoragePoolObjAssignDef(virStoragePoolObjListPtr pools, virStoragePoolDefPtr def);
-int virStoragePoolSaveConfig(const char *configDir,
This change from configDir
+int virStoragePoolSaveState(const char *stateFile, + virStoragePoolDefPtr def); +int virStoragePoolSaveConfig(const char *configFile,
to configFile should be in a separate commit. (You can just push it under the trivial rule) Jan
This change from configDir
+int virStoragePoolSaveState(const char *stateFile, + virStoragePoolDefPtr def); +int virStoragePoolSaveConfig(const char *configFile,
to configFile should be in a separate commit.
(You can just push it under the trivial rule)
Jan
Thanks for notice, I sent a patch to list and pushed it as trivial. Erik
This patch introduces new virStorageDriverState element stateDir. Also adds necessary changes to storageStateInitialize, so that directories initialization becomes more generic. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733 --- src/conf/storage_conf.h | 1 + src/storage/storage_driver.c | 109 +++++++++++++++++++++++++++++++------------ 2 files changed, 81 insertions(+), 29 deletions(-) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index da378b7..8d43019 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..0180fd7 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -78,6 +78,7 @@ static void storageDriverAutostart(void) { size_t i; + char *stateFile = NULL; virConnectPtr conn = NULL; /* XXX Remove hardcoding of QEMU URI */ @@ -136,7 +137,14 @@ storageDriverAutostart(void) virStoragePoolObjUnlock(pool); continue; } + + if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, ".xml"))) + continue; + + ignore_value(virStoragePoolSaveState(stateFile, pool->def)); pool->active = 1; + VIR_FREE(stateFile); } virStoragePoolObjUnlock(pool); } @@ -147,61 +155,67 @@ storageDriverAutostart(void) /** * virStorageStartup: * - * Initialization function for the QEmu daemon + * Initialization function for the Storage Driver */ static int 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; } /** @@ -261,6 +275,7 @@ storageStateCleanup(void) VIR_FREE(driver->configDir); VIR_FREE(driver->autostartDir); + VIR_FREE(driver->stateDir); storageDriverUnlock(); virMutexDestroy(&driver->lock); VIR_FREE(driver); @@ -579,6 +594,7 @@ storagePoolCreateXML(virConnectPtr conn, virStoragePoolObjPtr pool = NULL; virStoragePoolPtr ret = NULL; virStorageBackendPtr backend; + char *stateFile; virCheckFlags(0, NULL); @@ -609,7 +625,12 @@ storagePoolCreateXML(virConnectPtr conn, goto cleanup; } - if (backend->refreshPool(conn, pool) < 0) { + if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, ".xml"))) + goto cleanup; + + if (virStoragePoolSaveState(stateFile, pool->def) < 0 || + backend->refreshPool(conn, pool) < 0) { if (backend->stopPool) backend->stopPool(conn, pool); virStoragePoolObjRemove(&driver->pools, pool); @@ -745,6 +766,7 @@ storagePoolCreate(virStoragePoolPtr obj, virStoragePoolObjPtr pool; virStorageBackendPtr backend; int ret = -1; + char *stateFile = NULL; virCheckFlags(0, -1); @@ -763,21 +785,28 @@ storagePoolCreate(virStoragePoolPtr obj, pool->def->name); goto cleanup; } + + VIR_INFO("Starting up storage pool '%s'", pool->def->name); if (backend->startPool && backend->startPool(obj->conn, pool) < 0) goto cleanup; - if (backend->refreshPool(obj->conn, pool) < 0) { + if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, ".xml"))) + goto cleanup; + + if (virStoragePoolSaveState(stateFile, pool->def) || + backend->refreshPool(obj->conn, pool) < 0) { if (backend->stopPool) backend->stopPool(obj->conn, pool); 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; } @@ -822,6 +851,7 @@ storagePoolDestroy(virStoragePoolPtr obj) { virStoragePoolObjPtr pool; virStorageBackendPtr backend; + char *stateFile = NULL; int ret = -1; storageDriverLock(); @@ -840,12 +870,22 @@ storagePoolDestroy(virStoragePoolPtr obj) if ((backend = virStorageBackendForType(pool->def->type)) == NULL) goto cleanup; + VIR_INFO("Destroying storage pool '%s'", pool->def->name); + if (!virStoragePoolObjIsActive(pool)) { virReportError(VIR_ERR_OPERATION_INVALID, _("storage pool '%s' is not active"), pool->def->name); goto cleanup; } + if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, + ".xml"))) + goto cleanup; + + unlink(stateFile); + VIR_FREE(stateFile); + if (pool->asyncjobs > 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("pool '%s' has asynchronous jobs running."), @@ -860,7 +900,6 @@ storagePoolDestroy(virStoragePoolPtr obj) virStoragePoolObjClearVols(pool); pool->active = 0; - VIR_INFO("Shutting down storage pool '%s'", pool->def->name); if (pool->configFile == NULL) { virStoragePoolObjRemove(&driver->pools, pool); @@ -870,6 +909,7 @@ storagePoolDestroy(virStoragePoolPtr obj) pool->def = pool->newDef; pool->newDef = NULL; } + ret = 0; cleanup: @@ -885,6 +925,7 @@ storagePoolDelete(virStoragePoolPtr obj, { virStoragePoolObjPtr pool; virStorageBackendPtr backend; + char *stateFile = NULL; int ret = -1; if (!(pool = virStoragePoolObjFromStoragePool(obj))) @@ -896,6 +937,8 @@ storagePoolDelete(virStoragePoolPtr obj, if ((backend = virStorageBackendForType(pool->def->type)) == NULL) goto cleanup; + VIR_INFO("Deleting storage pool '%s'", pool->def->name); + if (virStoragePoolObjIsActive(pool)) { virReportError(VIR_ERR_OPERATION_INVALID, _("storage pool '%s' is still active"), @@ -903,6 +946,14 @@ storagePoolDelete(virStoragePoolPtr obj, goto cleanup; } + if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, + ".xml"))) + goto cleanup; + + unlink(stateFile); + VIR_FREE(stateFile); + if (pool->asyncjobs > 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("pool '%s' has asynchronous jobs running."), @@ -917,7 +968,7 @@ storagePoolDelete(virStoragePoolPtr obj, } if (backend->deletePool(obj->conn, pool, flags) < 0) goto cleanup; - VIR_INFO("Deleting storage pool '%s'", pool->def->name); + ret = 0; cleanup: -- 1.9.3
On 04/02/2015 06:10 AM, Erik Skultety wrote:
This patch introduces new virStorageDriverState element stateDir. Also adds necessary changes to storageStateInitialize, so that directories initialization becomes more generic.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733
^^ don't forget to remove...
--- src/conf/storage_conf.h | 1 + src/storage/storage_driver.c | 109 +++++++++++++++++++++++++++++++------------ 2 files changed, 81 insertions(+), 29 deletions(-)
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index da378b7..8d43019 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..0180fd7 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -78,6 +78,7 @@ static void storageDriverAutostart(void) { size_t i; + char *stateFile = NULL; virConnectPtr conn = NULL;
/* XXX Remove hardcoding of QEMU URI */ @@ -136,7 +137,14 @@ storageDriverAutostart(void) virStoragePoolObjUnlock(pool); continue; } + + if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, ".xml"))) + continue; + + ignore_value(virStoragePoolSaveState(stateFile, pool->def)); pool->active = 1; + VIR_FREE(stateFile);
I didn't quite pick up on this the first time, but with everything together now it was more obvious... If we cannot build the stateFile path, then pool->active = 1 never happens, but the pool is started... So it seems we'll have to stop the pool here as well and tell why (rather than continue)... Perhaps follow how you did things in storagePoolCreate[XML] with the extra caveats noted below...
} virStoragePoolObjUnlock(pool); } @@ -147,61 +155,67 @@ storageDriverAutostart(void) /** * virStorageStartup: * - * Initialization function for the QEmu daemon + * Initialization function for the Storage Driver
Yay! thanks ;-)
*/ static int 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; }
/** @@ -261,6 +275,7 @@ storageStateCleanup(void)
VIR_FREE(driver->configDir); VIR_FREE(driver->autostartDir); + VIR_FREE(driver->stateDir); storageDriverUnlock(); virMutexDestroy(&driver->lock); VIR_FREE(driver); @@ -579,6 +594,7 @@ storagePoolCreateXML(virConnectPtr conn, virStoragePoolObjPtr pool = NULL; virStoragePoolPtr ret = NULL; virStorageBackendPtr backend; + char *stateFile;
virCheckFlags(0, NULL);
@@ -609,7 +625,12 @@ storagePoolCreateXML(virConnectPtr conn, goto cleanup; }
- if (backend->refreshPool(conn, pool) < 0) { + if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, ".xml"))) + goto cleanup;
At this point the pool is started, so rather than goto cleanup, we'll need to stop the pool
+ + if (virStoragePoolSaveState(stateFile, pool->def) < 0 || + backend->refreshPool(conn, pool) < 0) { if (backend->stopPool) backend->stopPool(conn, pool); virStoragePoolObjRemove(&driver->pools, pool); @@ -745,6 +766,7 @@ storagePoolCreate(virStoragePoolPtr obj, virStoragePoolObjPtr pool; virStorageBackendPtr backend; int ret = -1; + char *stateFile = NULL;
virCheckFlags(0, -1);
@@ -763,21 +785,28 @@ storagePoolCreate(virStoragePoolPtr obj, pool->def->name); goto cleanup; } + + VIR_INFO("Starting up storage pool '%s'", pool->def->name); if (backend->startPool && backend->startPool(obj->conn, pool) < 0) goto cleanup;
- if (backend->refreshPool(obj->conn, pool) < 0) { + if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, ".xml"))) + goto cleanup;
Same start and stop on goto cleanup issue.
+ + if (virStoragePoolSaveState(stateFile, pool->def) || + backend->refreshPool(obj->conn, pool) < 0) { if (backend->stopPool) backend->stopPool(obj->conn, pool); 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; } @@ -822,6 +851,7 @@ storagePoolDestroy(virStoragePoolPtr obj) { virStoragePoolObjPtr pool; virStorageBackendPtr backend; + char *stateFile = NULL; int ret = -1;
storageDriverLock(); @@ -840,12 +870,22 @@ storagePoolDestroy(virStoragePoolPtr obj) if ((backend = virStorageBackendForType(pool->def->type)) == NULL) goto cleanup;
+ VIR_INFO("Destroying storage pool '%s'", pool->def->name); + if (!virStoragePoolObjIsActive(pool)) { virReportError(VIR_ERR_OPERATION_INVALID, _("storage pool '%s' is not active"), pool->def->name); goto cleanup; }
+ if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, + ".xml"))) + goto cleanup; + + unlink(stateFile); + VIR_FREE(stateFile); +
Perhaps should occur after the next check
if (pool->asyncjobs > 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("pool '%s' has asynchronous jobs running."), @@ -860,7 +900,6 @@ storagePoolDestroy(virStoragePoolPtr obj) virStoragePoolObjClearVols(pool);
pool->active = 0; - VIR_INFO("Shutting down storage pool '%s'", pool->def->name);
if (pool->configFile == NULL) { virStoragePoolObjRemove(&driver->pools, pool); @@ -870,6 +909,7 @@ storagePoolDestroy(virStoragePoolPtr obj) pool->def = pool->newDef; pool->newDef = NULL; } + ret = 0;
cleanup: @@ -885,6 +925,7 @@ storagePoolDelete(virStoragePoolPtr obj, { virStoragePoolObjPtr pool; virStorageBackendPtr backend; + char *stateFile = NULL; int ret = -1;
if (!(pool = virStoragePoolObjFromStoragePool(obj))) @@ -896,6 +937,8 @@ storagePoolDelete(virStoragePoolPtr obj, if ((backend = virStorageBackendForType(pool->def->type)) == NULL) goto cleanup;
+ VIR_INFO("Deleting storage pool '%s'", pool->def->name); + if (virStoragePoolObjIsActive(pool)) { virReportError(VIR_ERR_OPERATION_INVALID, _("storage pool '%s' is still active"), @@ -903,6 +946,14 @@ storagePoolDelete(virStoragePoolPtr obj, goto cleanup; }
+ if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, + ".xml"))) + goto cleanup; + + unlink(stateFile); + VIR_FREE(stateFile); +
Perhaps should occur after the next check... I think these are easily resolvable, but a v3 (sic) should be seen John
if (pool->asyncjobs > 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("pool '%s' has asynchronous jobs running."), @@ -917,7 +968,7 @@ storagePoolDelete(virStoragePoolPtr obj, } if (backend->deletePool(obj->conn, pool, flags) < 0) goto cleanup; - VIR_INFO("Deleting storage pool '%s'", pool->def->name); + ret = 0;
cleanup:
This patch introduces new virStorageDriverState element stateDir. Also adds necessary changes to storageStateInitialize, so that directories initialization becomes more generic. --- src/conf/storage_conf.h | 1 + src/storage/storage_driver.c | 108 +++++++++++++++++++++++++++++++------------ 2 files changed, 79 insertions(+), 30 deletions(-) diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index da378b7..8d43019 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..b350ee6 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -78,6 +78,7 @@ static void storageDriverAutostart(void) { size_t i; + char *stateFile = NULL; virConnectPtr conn = NULL; /* XXX Remove hardcoding of QEMU URI */ @@ -126,7 +127,11 @@ storageDriverAutostart(void) if (started) { virStoragePoolObjClearVols(pool); - if (backend->refreshPool(conn, pool) < 0) { + stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, ".xml"); + if (!stateFile || + virStoragePoolSaveState(stateFile, pool->def) < 0 || + backend->refreshPool(conn, pool) < 0) { virErrorPtr err = virGetLastError(); if (backend->stopPool) backend->stopPool(conn, pool); @@ -136,7 +141,9 @@ storageDriverAutostart(void) virStoragePoolObjUnlock(pool); continue; } + pool->active = 1; + VIR_FREE(stateFile); } virStoragePoolObjUnlock(pool); } @@ -147,61 +154,67 @@ storageDriverAutostart(void) /** * virStorageStartup: * - * Initialization function for the QEmu daemon + * Initialization function for the Storage Driver */ static int 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; } /** @@ -261,6 +274,7 @@ storageStateCleanup(void) VIR_FREE(driver->configDir); VIR_FREE(driver->autostartDir); + VIR_FREE(driver->stateDir); storageDriverUnlock(); virMutexDestroy(&driver->lock); VIR_FREE(driver); @@ -579,6 +593,7 @@ storagePoolCreateXML(virConnectPtr conn, virStoragePoolObjPtr pool = NULL; virStoragePoolPtr ret = NULL; virStorageBackendPtr backend; + char *stateFile; virCheckFlags(0, NULL); @@ -609,7 +624,11 @@ storagePoolCreateXML(virConnectPtr conn, goto cleanup; } - if (backend->refreshPool(conn, pool) < 0) { + stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, ".xml"); + + if (!stateFile || virStoragePoolSaveState(stateFile, pool->def) < 0 || + backend->refreshPool(conn, pool) < 0) { if (backend->stopPool) backend->stopPool(conn, pool); virStoragePoolObjRemove(&driver->pools, pool); @@ -745,6 +764,7 @@ storagePoolCreate(virStoragePoolPtr obj, virStoragePoolObjPtr pool; virStorageBackendPtr backend; int ret = -1; + char *stateFile = NULL; virCheckFlags(0, -1); @@ -763,21 +783,27 @@ storagePoolCreate(virStoragePoolPtr obj, pool->def->name); goto cleanup; } + + VIR_INFO("Starting up storage pool '%s'", pool->def->name); if (backend->startPool && backend->startPool(obj->conn, pool) < 0) goto cleanup; - if (backend->refreshPool(obj->conn, pool) < 0) { + stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, ".xml"); + + if (!stateFile || virStoragePoolSaveState(stateFile, pool->def) < 0 || + backend->refreshPool(obj->conn, pool) < 0) { if (backend->stopPool) backend->stopPool(obj->conn, pool); 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; } @@ -822,6 +848,7 @@ storagePoolDestroy(virStoragePoolPtr obj) { virStoragePoolObjPtr pool; virStorageBackendPtr backend; + char *stateFile = NULL; int ret = -1; storageDriverLock(); @@ -840,6 +867,8 @@ storagePoolDestroy(virStoragePoolPtr obj) if ((backend = virStorageBackendForType(pool->def->type)) == NULL) goto cleanup; + VIR_INFO("Destroying storage pool '%s'", pool->def->name); + if (!virStoragePoolObjIsActive(pool)) { virReportError(VIR_ERR_OPERATION_INVALID, _("storage pool '%s' is not active"), pool->def->name); @@ -853,6 +882,14 @@ storagePoolDestroy(virStoragePoolPtr obj) goto cleanup; } + if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, + ".xml"))) + goto cleanup; + + unlink(stateFile); + VIR_FREE(stateFile); + if (backend->stopPool && backend->stopPool(obj->conn, pool) < 0) goto cleanup; @@ -860,7 +897,6 @@ storagePoolDestroy(virStoragePoolPtr obj) virStoragePoolObjClearVols(pool); pool->active = 0; - VIR_INFO("Shutting down storage pool '%s'", pool->def->name); if (pool->configFile == NULL) { virStoragePoolObjRemove(&driver->pools, pool); @@ -870,6 +906,7 @@ storagePoolDestroy(virStoragePoolPtr obj) pool->def = pool->newDef; pool->newDef = NULL; } + ret = 0; cleanup: @@ -885,6 +922,7 @@ storagePoolDelete(virStoragePoolPtr obj, { virStoragePoolObjPtr pool; virStorageBackendPtr backend; + char *stateFile = NULL; int ret = -1; if (!(pool = virStoragePoolObjFromStoragePool(obj))) @@ -896,6 +934,8 @@ storagePoolDelete(virStoragePoolPtr obj, if ((backend = virStorageBackendForType(pool->def->type)) == NULL) goto cleanup; + VIR_INFO("Deleting storage pool '%s'", pool->def->name); + if (virStoragePoolObjIsActive(pool)) { virReportError(VIR_ERR_OPERATION_INVALID, _("storage pool '%s' is still active"), @@ -910,6 +950,14 @@ storagePoolDelete(virStoragePoolPtr obj, goto cleanup; } + if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, + ".xml"))) + goto cleanup; + + unlink(stateFile); + VIR_FREE(stateFile); + if (!backend->deletePool) { virReportError(VIR_ERR_NO_SUPPORT, "%s", _("pool does not support pool deletion")); @@ -917,7 +965,7 @@ storagePoolDelete(virStoragePoolPtr obj, } if (backend->deletePool(obj->conn, pool, flags) < 0) goto cleanup; - VIR_INFO("Deleting storage pool '%s'", pool->def->name); + ret = 0; cleanup: -- 1.9.3
On 04/03/2015 07:03 AM, Erik Skultety wrote:
This patch introduces new virStorageDriverState element stateDir. Also adds necessary changes to storageStateInitialize, so that directories initialization becomes more generic. --- src/conf/storage_conf.h | 1 + src/storage/storage_driver.c | 108 +++++++++++++++++++++++++++++++------------ 2 files changed, 79 insertions(+), 30 deletions(-)
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index da378b7..8d43019 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..b350ee6 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -78,6 +78,7 @@ static void storageDriverAutostart(void) { size_t i; + char *stateFile = NULL; virConnectPtr conn = NULL;
/* XXX Remove hardcoding of QEMU URI */ @@ -126,7 +127,11 @@ storageDriverAutostart(void)
if (started) { virStoragePoolObjClearVols(pool); - if (backend->refreshPool(conn, pool) < 0) { + stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, ".xml"); + if (!stateFile || + virStoragePoolSaveState(stateFile, pool->def) < 0 || + backend->refreshPool(conn, pool) < 0) { virErrorPtr err = virGetLastError(); if (backend->stopPool) backend->stopPool(conn, pool); @@ -136,7 +141,9 @@ storageDriverAutostart(void) virStoragePoolObjUnlock(pool);
Add a VIR_FREE(stateFile); before continue
continue; } + pool->active = 1; + VIR_FREE(stateFile); } virStoragePoolObjUnlock(pool); } @@ -147,61 +154,67 @@ storageDriverAutostart(void) /** * virStorageStartup: * - * Initialization function for the QEmu daemon + * Initialization function for the Storage Driver */ static int 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; }
/** @@ -261,6 +274,7 @@ storageStateCleanup(void)
VIR_FREE(driver->configDir); VIR_FREE(driver->autostartDir); + VIR_FREE(driver->stateDir); storageDriverUnlock(); virMutexDestroy(&driver->lock); VIR_FREE(driver); @@ -579,6 +593,7 @@ storagePoolCreateXML(virConnectPtr conn, virStoragePoolObjPtr pool = NULL; virStoragePoolPtr ret = NULL; virStorageBackendPtr backend; + char *stateFile;
s/;/ = NULL;
virCheckFlags(0, NULL);
@@ -609,7 +624,11 @@ storagePoolCreateXML(virConnectPtr conn, goto cleanup; }
- if (backend->refreshPool(conn, pool) < 0) { + stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, ".xml"); + + if (!stateFile || virStoragePoolSaveState(stateFile, pool->def) < 0 || + backend->refreshPool(conn, pool) < 0) { if (backend->stopPool) backend->stopPool(conn, pool); virStoragePoolObjRemove(&driver->pools, pool);
Then in the cleanup: VIR_FREE(stateFile); ACK - with the adjustments John
@@ -745,6 +764,7 @@ storagePoolCreate(virStoragePoolPtr obj, virStoragePoolObjPtr pool; virStorageBackendPtr backend; int ret = -1; + char *stateFile = NULL;
virCheckFlags(0, -1);
@@ -763,21 +783,27 @@ storagePoolCreate(virStoragePoolPtr obj, pool->def->name); goto cleanup; } + + VIR_INFO("Starting up storage pool '%s'", pool->def->name); if (backend->startPool && backend->startPool(obj->conn, pool) < 0) goto cleanup;
- if (backend->refreshPool(obj->conn, pool) < 0) { + stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, ".xml"); + + if (!stateFile || virStoragePoolSaveState(stateFile, pool->def) < 0 || + backend->refreshPool(obj->conn, pool) < 0) { if (backend->stopPool) backend->stopPool(obj->conn, pool); 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; } @@ -822,6 +848,7 @@ storagePoolDestroy(virStoragePoolPtr obj) { virStoragePoolObjPtr pool; virStorageBackendPtr backend; + char *stateFile = NULL; int ret = -1;
storageDriverLock(); @@ -840,6 +867,8 @@ storagePoolDestroy(virStoragePoolPtr obj) if ((backend = virStorageBackendForType(pool->def->type)) == NULL) goto cleanup;
+ VIR_INFO("Destroying storage pool '%s'", pool->def->name); + if (!virStoragePoolObjIsActive(pool)) { virReportError(VIR_ERR_OPERATION_INVALID, _("storage pool '%s' is not active"), pool->def->name); @@ -853,6 +882,14 @@ storagePoolDestroy(virStoragePoolPtr obj) goto cleanup; }
+ if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, + ".xml"))) + goto cleanup; + + unlink(stateFile); + VIR_FREE(stateFile); + if (backend->stopPool && backend->stopPool(obj->conn, pool) < 0) goto cleanup; @@ -860,7 +897,6 @@ storagePoolDestroy(virStoragePoolPtr obj) virStoragePoolObjClearVols(pool);
pool->active = 0; - VIR_INFO("Shutting down storage pool '%s'", pool->def->name);
if (pool->configFile == NULL) { virStoragePoolObjRemove(&driver->pools, pool); @@ -870,6 +906,7 @@ storagePoolDestroy(virStoragePoolPtr obj) pool->def = pool->newDef; pool->newDef = NULL; } + ret = 0;
cleanup: @@ -885,6 +922,7 @@ storagePoolDelete(virStoragePoolPtr obj, { virStoragePoolObjPtr pool; virStorageBackendPtr backend; + char *stateFile = NULL; int ret = -1;
if (!(pool = virStoragePoolObjFromStoragePool(obj))) @@ -896,6 +934,8 @@ storagePoolDelete(virStoragePoolPtr obj, if ((backend = virStorageBackendForType(pool->def->type)) == NULL) goto cleanup;
+ VIR_INFO("Deleting storage pool '%s'", pool->def->name); + if (virStoragePoolObjIsActive(pool)) { virReportError(VIR_ERR_OPERATION_INVALID, _("storage pool '%s' is still active"), @@ -910,6 +950,14 @@ storagePoolDelete(virStoragePoolPtr obj, goto cleanup; }
+ if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, + ".xml"))) + goto cleanup; + + unlink(stateFile); + VIR_FREE(stateFile); + if (!backend->deletePool) { virReportError(VIR_ERR_NO_SUPPORT, "%s", _("pool does not support pool deletion")); @@ -917,7 +965,7 @@ storagePoolDelete(virStoragePoolPtr obj, } if (backend->deletePool(obj->conn, pool, flags) < 0) goto cleanup; - VIR_INFO("Deleting storage pool '%s'", pool->def->name); + ret = 0;
cleanup:
Thanks, fixed and pushed with remaining ACKed patches 5 and 6. Erik On 04/07/2015 03:55 PM, John Ferlan wrote:
On 04/03/2015 07:03 AM, Erik Skultety wrote:
This patch introduces new virStorageDriverState element stateDir. Also adds necessary changes to storageStateInitialize, so that directories initialization becomes more generic. --- src/conf/storage_conf.h | 1 + src/storage/storage_driver.c | 108 +++++++++++++++++++++++++++++++------------ 2 files changed, 79 insertions(+), 30 deletions(-)
diff --git a/src/conf/storage_conf.h b/src/conf/storage_conf.h index da378b7..8d43019 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..b350ee6 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -78,6 +78,7 @@ static void storageDriverAutostart(void) { size_t i; + char *stateFile = NULL; virConnectPtr conn = NULL;
/* XXX Remove hardcoding of QEMU URI */ @@ -126,7 +127,11 @@ storageDriverAutostart(void)
if (started) { virStoragePoolObjClearVols(pool); - if (backend->refreshPool(conn, pool) < 0) { + stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, ".xml"); + if (!stateFile || + virStoragePoolSaveState(stateFile, pool->def) < 0 || + backend->refreshPool(conn, pool) < 0) { virErrorPtr err = virGetLastError(); if (backend->stopPool) backend->stopPool(conn, pool); @@ -136,7 +141,9 @@ storageDriverAutostart(void) virStoragePoolObjUnlock(pool);
Add a VIR_FREE(stateFile); before continue
continue; } + pool->active = 1; + VIR_FREE(stateFile); } virStoragePoolObjUnlock(pool); } @@ -147,61 +154,67 @@ storageDriverAutostart(void) /** * virStorageStartup: * - * Initialization function for the QEmu daemon + * Initialization function for the Storage Driver */ static int 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; }
/** @@ -261,6 +274,7 @@ storageStateCleanup(void)
VIR_FREE(driver->configDir); VIR_FREE(driver->autostartDir); + VIR_FREE(driver->stateDir); storageDriverUnlock(); virMutexDestroy(&driver->lock); VIR_FREE(driver); @@ -579,6 +593,7 @@ storagePoolCreateXML(virConnectPtr conn, virStoragePoolObjPtr pool = NULL; virStoragePoolPtr ret = NULL; virStorageBackendPtr backend; + char *stateFile;
s/;/ = NULL;
virCheckFlags(0, NULL);
@@ -609,7 +624,11 @@ storagePoolCreateXML(virConnectPtr conn, goto cleanup; }
- if (backend->refreshPool(conn, pool) < 0) { + stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, ".xml"); + + if (!stateFile || virStoragePoolSaveState(stateFile, pool->def) < 0 || + backend->refreshPool(conn, pool) < 0) { if (backend->stopPool) backend->stopPool(conn, pool); virStoragePoolObjRemove(&driver->pools, pool);
Then in the cleanup:
VIR_FREE(stateFile);
ACK - with the adjustments
John
@@ -745,6 +764,7 @@ storagePoolCreate(virStoragePoolPtr obj, virStoragePoolObjPtr pool; virStorageBackendPtr backend; int ret = -1; + char *stateFile = NULL;
virCheckFlags(0, -1);
@@ -763,21 +783,27 @@ storagePoolCreate(virStoragePoolPtr obj, pool->def->name); goto cleanup; } + + VIR_INFO("Starting up storage pool '%s'", pool->def->name); if (backend->startPool && backend->startPool(obj->conn, pool) < 0) goto cleanup;
- if (backend->refreshPool(obj->conn, pool) < 0) { + stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, ".xml"); + + if (!stateFile || virStoragePoolSaveState(stateFile, pool->def) < 0 || + backend->refreshPool(obj->conn, pool) < 0) { if (backend->stopPool) backend->stopPool(obj->conn, pool); 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; } @@ -822,6 +848,7 @@ storagePoolDestroy(virStoragePoolPtr obj) { virStoragePoolObjPtr pool; virStorageBackendPtr backend; + char *stateFile = NULL; int ret = -1;
storageDriverLock(); @@ -840,6 +867,8 @@ storagePoolDestroy(virStoragePoolPtr obj) if ((backend = virStorageBackendForType(pool->def->type)) == NULL) goto cleanup;
+ VIR_INFO("Destroying storage pool '%s'", pool->def->name); + if (!virStoragePoolObjIsActive(pool)) { virReportError(VIR_ERR_OPERATION_INVALID, _("storage pool '%s' is not active"), pool->def->name); @@ -853,6 +882,14 @@ storagePoolDestroy(virStoragePoolPtr obj) goto cleanup; }
+ if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, + ".xml"))) + goto cleanup; + + unlink(stateFile); + VIR_FREE(stateFile); + if (backend->stopPool && backend->stopPool(obj->conn, pool) < 0) goto cleanup; @@ -860,7 +897,6 @@ storagePoolDestroy(virStoragePoolPtr obj) virStoragePoolObjClearVols(pool);
pool->active = 0; - VIR_INFO("Shutting down storage pool '%s'", pool->def->name);
if (pool->configFile == NULL) { virStoragePoolObjRemove(&driver->pools, pool); @@ -870,6 +906,7 @@ storagePoolDestroy(virStoragePoolPtr obj) pool->def = pool->newDef; pool->newDef = NULL; } + ret = 0;
cleanup: @@ -885,6 +922,7 @@ storagePoolDelete(virStoragePoolPtr obj, { virStoragePoolObjPtr pool; virStorageBackendPtr backend; + char *stateFile = NULL; int ret = -1;
if (!(pool = virStoragePoolObjFromStoragePool(obj))) @@ -896,6 +934,8 @@ storagePoolDelete(virStoragePoolPtr obj, if ((backend = virStorageBackendForType(pool->def->type)) == NULL) goto cleanup;
+ VIR_INFO("Deleting storage pool '%s'", pool->def->name); + if (virStoragePoolObjIsActive(pool)) { virReportError(VIR_ERR_OPERATION_INVALID, _("storage pool '%s' is still active"), @@ -910,6 +950,14 @@ storagePoolDelete(virStoragePoolPtr obj, goto cleanup; }
+ if (!(stateFile = virFileBuildPath(driver->stateDir, + pool->def->name, + ".xml"))) + goto cleanup; + + unlink(stateFile); + VIR_FREE(stateFile); + if (!backend->deletePool) { virReportError(VIR_ERR_NO_SUPPORT, "%s", _("pool does not support pool deletion")); @@ -917,7 +965,7 @@ storagePoolDelete(virStoragePoolPtr obj, } if (backend->deletePool(obj->conn, pool, flags) < 0) goto cleanup; - VIR_INFO("Deleting storage pool '%s'", pool->def->name); + ret = 0;
cleanup:
These functions operate exactly the same as their network equivalents virNetworkLoadAllState, virNetworkLoadState. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733 --- src/conf/storage_conf.c | 94 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/storage_conf.h | 7 ++++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 13 ++++++ 4 files changed, 115 insertions(+) diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index ee564f2..e7d6e6b 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1863,6 +1863,100 @@ 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 error; + + if (!(xml = virXMLParseCtxt(stateFile, NULL, _("(pool state)"), &ctxt))) + goto error; + + if (!(node = virXPathNode("//pool", ctxt))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not find any 'pool' element in state file")); + goto error; + } + + ctxt->node = node; + if (!(def = virStoragePoolDefParseXML(ctxt))) + goto error; + + if (!STREQ(name, def->name)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Storage pool state file '%s' does not match " + "pool name '%s'"), + stateFile, def->name); + goto error; + } + + /* create the object */ + if (!(pool = virStoragePoolObjAssignDef(pools, def))) + goto error; + + /* 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); + return pool; + + error: + virStoragePoolDefFree(def); + goto cleanup; +} + + +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 8d43019..7471006 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 56acb01..6b95dea 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -800,6 +800,7 @@ virStoragePoolFormatFileSystemNetTypeToString; virStoragePoolFormatFileSystemTypeToString; virStoragePoolGetVhbaSCSIHostParent; virStoragePoolLoadAllConfigs; +virStoragePoolLoadAllState; virStoragePoolObjAssignDef; virStoragePoolObjClearVols; virStoragePoolObjDeleteDef; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 0180fd7..36c05b3 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -199,6 +199,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) @@ -247,6 +258,8 @@ storageStateReload(void) return -1; storageDriverLock(); + virStoragePoolLoadAllState(&driver->pools, + driver->stateDir); virStoragePoolLoadAllConfigs(&driver->pools, driver->configDir, driver->autostartDir); -- 1.9.3
On 04/02/2015 06:10 AM, Erik Skultety wrote:
These functions operate exactly the same as their network equivalents virNetworkLoadAllState, virNetworkLoadState.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733
^^ Don't forget to remove...
--- src/conf/storage_conf.c | 94 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/storage_conf.h | 7 ++++ src/libvirt_private.syms | 1 + src/storage/storage_driver.c | 13 ++++++ 4 files changed, 115 insertions(+)
diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index ee564f2..e7d6e6b 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -1863,6 +1863,100 @@ 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 error; + + if (!(xml = virXMLParseCtxt(stateFile, NULL, _("(pool state)"), &ctxt))) + goto error; + + if (!(node = virXPathNode("//pool", ctxt))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Could not find any 'pool' element in state file")); + goto error; + } + + ctxt->node = node; + if (!(def = virStoragePoolDefParseXML(ctxt))) + goto error; + + if (!STREQ(name, def->name)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("Storage pool state file '%s' does not match " + "pool name '%s'"), + stateFile, def->name); + goto error; + } + + /* create the object */ + if (!(pool = virStoragePoolObjAssignDef(pools, def))) + goto error; + + /* XXX: future handling of some additional usefull status data,
s/usefull/useful ACK with the slight adjustments. If you want to post a v3 "inline"/as a response to patch 4 - that's fine John
+ * 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); + return pool; + + error: + virStoragePoolDefFree(def); + goto cleanup; +} + + +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 8d43019..7471006 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 56acb01..6b95dea 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -800,6 +800,7 @@ virStoragePoolFormatFileSystemNetTypeToString; virStoragePoolFormatFileSystemTypeToString; virStoragePoolGetVhbaSCSIHostParent; virStoragePoolLoadAllConfigs; +virStoragePoolLoadAllState; virStoragePoolObjAssignDef; virStoragePoolObjClearVols; virStoragePoolObjDeleteDef; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 0180fd7..36c05b3 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -199,6 +199,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) @@ -247,6 +258,8 @@ storageStateReload(void) return -1;
storageDriverLock(); + virStoragePoolLoadAllState(&driver->pools, + driver->stateDir); virStoragePoolLoadAllConfigs(&driver->pools, driver->configDir, driver->autostartDir);
ACK with the slight adjustments.
If you want to post a v3 "inline"/as a response to patch 4 - that's fine
John
Thank you, I sent v3 of patch 4/6, fixed the slight adjustments in 5 and 6. I'll push them once I have an ACK for 4/6. Erik
The 'checkPool' callback was originally part of the storageDriverAutostart function, but the pools need to be checked earlier during initialization phase, otherwise we can't start a domain which mountsa volume after daemon's restarted. This is because qemuProcessReconnect is called earlier than storageDriverAutostart. Therefore the 'checkPool' logic has been moved to storagePoolUpdateAllState which is called inside storageDriverInitialize. We also need a valid 'conn' reference to be able to execute 'refreshPool' during initialization phase. Though it isn't available until storageDriverAutostart all of our storage backends do ignore 'conn' pointer, except for RBD, but RBD doesn't support 'checkPool' callback, so it's safe to pass conn = NULL in this case. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733 --- src/storage/storage_driver.c | 74 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 61 insertions(+), 13 deletions(-) diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 36c05b3..12e94ad 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; @@ -95,23 +153,11 @@ storageDriverAutostart(void) virStoragePoolObjLock(pool); if ((backend = virStorageBackendForType(pool->def->type)) == NULL) { - VIR_ERROR(_("Missing backend %d"), pool->def->type); virStoragePoolObjUnlock(pool); 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) { @@ -215,6 +261,8 @@ storageStateInitialize(bool privileged, driver->autostartDir) < 0) goto error; + storagePoolUpdateAllState(); + storageDriverUnlock(); ret = 0; -- 1.9.3
On 04/02/2015 06:10 AM, Erik Skultety wrote:
The 'checkPool' callback was originally part of the storageDriverAutostart function, but the pools need to be checked earlier during initialization phase, otherwise we can't start a domain which mountsa volume after daemon's
s/mountsa/mounts a/ s/after daemon's/after the libvirtd daemon/
restarted. This is because qemuProcessReconnect is called earlier than storageDriverAutostart. Therefore the 'checkPool' logic has been moved to storagePoolUpdateAllState which is called inside storageDriverInitialize.
We also need a valid 'conn' reference to be able to execute 'refreshPool' during initialization phase. Though it isn't available until storageDriverAutostart all of our storage backends do ignore 'conn' pointer, except for RBD, but RBD doesn't support 'checkPool' callback, so it's safe to pass conn = NULL in this case.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1177733
yes, this does belong in this one Also, as I'm going through my list of backlog bugzillas, I think that https://bugzilla.redhat.com/show_bug.cgi?id=1125805 can be duplicated to this bz.
--- src/storage/storage_driver.c | 74 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 61 insertions(+), 13 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 36c05b3..12e94ad 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;
Instead of initializing it here - do it in the code...
+ + 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. + */ active = false;
So here's my reasoning for resetting active each time through the pool... What happens if pool [1] has a check and is determined to be active... then pool [2] doesn't have a check function, but active is still true from [1] - thus the refreshPool will happen. ACK with that adjustment. John
+ 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; @@ -95,23 +153,11 @@ storageDriverAutostart(void)
virStoragePoolObjLock(pool); if ((backend = virStorageBackendForType(pool->def->type)) == NULL) { - VIR_ERROR(_("Missing backend %d"), pool->def->type); virStoragePoolObjUnlock(pool); 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) { @@ -215,6 +261,8 @@ storageStateInitialize(bool privileged, driver->autostartDir) < 0) goto error;
+ storagePoolUpdateAllState(); + storageDriverUnlock();
ret = 0;
On 04/02/2015 12:10 PM, Erik Skultety wrote:
Patches add necessary changes to add support for storage pool state XMLs and thus resolve https://bugzilla.redhat.com/show_bug.cgi?id=1177733
Erik Skultety (6): conf: Introduce virStoragePoolDefFormatBuf conf: Introduce virStoragePoolSaveXML conf: Introduce virStoragePoolSaveState storage: Add support for storage pool state XML conf: Introduce virStoragePoolLoadAllState && virStoragePoolLoadState storage: Introduce storagePoolUpdateAllState function
src/conf/storage_conf.c | 233 ++++++++++++++++++++++++++++++++++++------- src/conf/storage_conf.h | 12 ++- src/libvirt_private.syms | 2 + src/storage/storage_driver.c | 188 +++++++++++++++++++++++++++------- 4 files changed, 358 insertions(+), 77 deletions(-)
These are v2 patches, forgot the --subject-prefix option with format-patch. Erik
participants (4)
-
Erik Skultety -
John Ferlan -
Ján Tomko -
Peter Krempa