[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 12/19] storage: Introduce virStoragePoolObj{Get|Set}Autostart



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

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]