On Tue, May 09, 2017 at 11:30:19AM -0400, John Ferlan wrote:
Rather than have the logic in the storage driver, move it to virstorageobj.
NB: virStoragePoolObjGetAutostart can take liberties with not needing the same if...else construct. Also pass a NULL for testStoragePoolSetAutostart to avoid the autostartLink setup using the autostartDir for the test driver.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/virstorageobj.c | 57 ++++++++++++++++++++++++++++++++++++++++++++ src/conf/virstorageobj.h | 8 +++++++ src/libvirt_private.syms | 2 ++ src/storage/storage_driver.c | 41 +++---------------------------- src/test/test_driver.c | 13 ++-------- 5 files changed, 72 insertions(+), 49 deletions(-)
diff --git a/src/conf/virstorageobj.c b/src/conf/virstorageobj.c index 0079472..9ce3840 100644 --- a/src/conf/virstorageobj.c +++ b/src/conf/virstorageobj.c @@ -111,6 +111,63 @@ virStoragePoolObjSetActive(virStoragePoolObjPtr obj, }
+int +virStoragePoolObjGetAutostart(virStoragePoolObjPtr obj) +{ + if (!obj->configFile) + return 0; + + return obj->autostart; +}
The getter is correct.
+ + +int +virStoragePoolObjSetAutostart(virStoragePoolObjPtr obj, + const char *autostartDir, + int autostart) +{ + obj->autostart = autostart; +
However, the setter does a lot more than it should be doing.
+ if (!obj->configFile) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("pool has no config file")); + return -1; + } + + autostart = (autostart != 0); + + if (obj->autostart != autostart) { + if (autostart && autostartDir) { + if (virFileMakePath(autostartDir) < 0) { + virReportSystemError(errno, + _("cannot create autostart directory %s"), + autostartDir); + return -1; + }
The autostartDir is owned by the storage driver so it is up to the storage driver to create the directory and create/remove the symlink.
+ + if (symlink(obj->configFile, obj->autostartLink) < 0) { + virReportSystemError(errno, + _("Failed to create symlink '%s' to '%s'"), + obj->autostartLink, obj->configFile); + return -1; + } + } else { + if (unlink(obj->autostartLink) < 0 && + errno != ENOENT && errno != ENOTDIR) { + virReportSystemError(errno, + _("Failed to delete symlink '%s'"), + obj->autostartLink); + return -1; + } + } + + obj->autostart = autostart;
This is duplicated the beginning of the function. The one at the beginning of the function is wrong, the @autostart should be changed only if we actually creates the symlink. I see that you've probably added it at the beginning of the function to reuse it in the test driver but that's wrong, it may cause issues for real storage driver.
+ } + + return 0; +} + + unsigned int virStoragePoolObjGetAsyncjobs(virStoragePoolObjPtr obj) { diff --git a/src/conf/virstorageobj.h b/src/conf/virstorageobj.h index d47b233..3b6e395 100644 --- a/src/conf/virstorageobj.h +++ b/src/conf/virstorageobj.h @@ -93,6 +93,14 @@ void virStoragePoolObjSetActive(virStoragePoolObjPtr obj, bool active);
+int +virStoragePoolObjGetAutostart(virStoragePoolObjPtr obj); + +int +virStoragePoolObjSetAutostart(virStoragePoolObjPtr obj, + const char *autostartDir, + int autostart); + unsigned int virStoragePoolObjGetAsyncjobs(virStoragePoolObjPtr obj);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index edd3174..e8b4eda 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1014,6 +1014,7 @@ virStoragePoolObjDeleteDef; virStoragePoolObjFindByName; virStoragePoolObjFindByUUID; virStoragePoolObjGetAsyncjobs; +virStoragePoolObjGetAutostart; virStoragePoolObjGetConfigFile; virStoragePoolObjGetDef; virStoragePoolObjGetNames; @@ -1031,6 +1032,7 @@ virStoragePoolObjNumOfVolumes; virStoragePoolObjRemove; virStoragePoolObjSaveDef; virStoragePoolObjSetActive; +virStoragePoolObjSetAutostart; virStoragePoolObjSetConfigFile; virStoragePoolObjSourceFindDuplicate; virStoragePoolObjStealNewDef; diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 6289314..c4e4e7b 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1250,11 +1250,7 @@ storagePoolGetAutostart(virStoragePoolPtr pool, if (virStoragePoolGetAutostartEnsureACL(pool->conn, obj->def) < 0) goto cleanup;
- if (!obj->configFile) { - *autostart = 0; - } else { - *autostart = obj->autostart; - } + *autostart = virStoragePoolObjGetAutostart(obj);
ret = 0;
@@ -1277,39 +1273,8 @@ storagePoolSetAutostart(virStoragePoolPtr pool, if (virStoragePoolSetAutostartEnsureACL(pool->conn, obj->def) < 0) goto cleanup;
- if (!obj->configFile) { - virReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("pool has no config file")); - } - - autostart = (autostart != 0); - - if (obj->autostart != autostart) { - if (autostart) { - if (virFileMakePath(driver->autostartDir) < 0) { - virReportSystemError(errno, - _("cannot create autostart directory %s"), - driver->autostartDir); - goto cleanup; - } - - if (symlink(obj->configFile, obj->autostartLink) < 0) { - virReportSystemError(errno, - _("Failed to create symlink '%s' to '%s'"), - obj->autostartLink, obj->configFile); - goto cleanup; - } - } else { - if (unlink(obj->autostartLink) < 0 && - errno != ENOENT && errno != ENOTDIR) { - virReportSystemError(errno, - _("Failed to delete symlink '%s'"), - obj->autostartLink); - goto cleanup; - } - } - obj->autostart = autostart;
I would leave the symlink creation logic here, to do that we need to have additional accessor API virStoragePoolObjGetAutostartLink(). Pavel
- } + if (virStoragePoolObjSetAutostart(obj, driver->autostartDir, autostart) < 0) + goto cleanup;
ret = 0;
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 68f1412..d68a18d 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -4690,11 +4690,7 @@ testStoragePoolGetAutostart(virStoragePoolPtr pool, if (!(obj = testStoragePoolObjFindByName(privconn, pool->name))) return -1;
- if (!obj->configFile) { - *autostart = 0; - } else { - *autostart = obj->autostart; - } + *autostart = virStoragePoolObjGetAutostart(obj);
virStoragePoolObjUnlock(obj); return 0; @@ -4712,14 +4708,9 @@ testStoragePoolSetAutostart(virStoragePoolPtr pool, if (!(obj = testStoragePoolObjFindByName(privconn, pool->name))) return -1;
- if (!obj->configFile) { - virReportError(VIR_ERR_INVALID_ARG, - "%s", _("pool has no config file")); + if (virStoragePoolObjSetAutostart(obj, NULL, autostart) < 0) goto cleanup; - }
- autostart = (autostart != 0); - obj->autostart = autostart; ret = 0;
cleanup: -- 2.9.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list