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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
