Re: [PATCH RFC v3 03/15] storage pools: refactoring of fs backend

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

 




On 12/02/2016 10:38 AM, Olga Krishtal wrote:
> The fs backend for storage pools works a lot with
> directories and etc. The same is true for filesystem pools with
> directory backend. In order to avoid rewriting the same code once again
> patch moves this code to virpoolcommon.c.
> 

I would just say moving this code to virpool.c in order to be shared by
future patches which will be adding a new file system storage driver
that will share the code.

> Signed-off-by: Olga Krishtal <okrishtal@xxxxxxxxxxxxx>
> ---
>  po/POTFILES.in                   |  1 +
>  src/libvirt_private.syms         |  3 ++
>  src/storage/storage_backend.h    |  1 -
>  src/storage/storage_backend_fs.c | 74 +++-------------------------------
>  src/util/virpoolcommon.c         | 87 ++++++++++++++++++++++++++++++++++++++++
>  src/util/virpoolcommon.h         |  7 ++++
>  6 files changed, 104 insertions(+), 69 deletions(-)
> 
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index 1469240..29bc45c 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -229,6 +229,7 @@ src/util/virpci.c
>  src/util/virperf.c
>  src/util/virpidfile.c
>  src/util/virpolkit.c
> +src/util/virpoolcommon.c

Should this have changed in the previous patch?

>  src/util/virportallocator.c
>  src/util/virprocess.c
>  src/util/virqemu.c
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index a061799..67ebe2a 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2709,6 +2709,9 @@ virStoragePoolSourceFree;
>  virStoragePoolSourceDeviceClear;
>  virStoragePoolSourceClear;
>  virStoragePoolSourceListNewSource;
> +virDirPoolDelete;
> +virDirItemCreate;
> +virDirPoolBuild;

Names would all need to start with virPool (see below for my suggestions).

>  
>  # Let emacs know we want case-insensitive sorting
>  # Local Variables:
> diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
> index 28e1a65..a08a725 100644
> --- a/src/storage/storage_backend.h
> +++ b/src/storage/storage_backend.h
> @@ -214,7 +214,6 @@ int virStorageBackendVolOpen(const char *path, struct stat *sb,
>      ATTRIBUTE_RETURN_CHECK
>      ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
>  
> -# define VIR_STORAGE_DEFAULT_POOL_PERM_MODE 0755

Keep the above


>  # define VIR_STORAGE_DEFAULT_VOL_PERM_MODE  0600
>  
>  int virStorageBackendUpdateVolInfo(virStorageVolDefPtr vol,
> diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
> index 6c8bae2..c21fbc8 100644
> --- a/src/storage/storage_backend_fs.c
> +++ b/src/storage/storage_backend_fs.c
> @@ -45,6 +45,7 @@
>  #include "storage_backend_fs.h"
>  #include "storage_conf.h"
>  #include "virstoragefile.h"
> +#include "virpoolcommon.h"
>  #include "vircommand.h"
>  #include "viralloc.h"
>  #include "virxml.h"
> @@ -809,11 +810,7 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED,
>                                   unsigned int flags)
>  {
>      int ret = -1;
> -    char *parent = NULL;
> -    char *p = NULL;
> -    mode_t mode;
>      bool needs_create_as_uid;
> -    unsigned int dir_create_flags;
>  
>      virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE |
>                    VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret);
> @@ -822,45 +819,9 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED,
>                               VIR_STORAGE_POOL_BUILD_NO_OVERWRITE,
>                               error);
>  
> -    if (VIR_STRDUP(parent, pool->def->target.path) < 0)
> -        goto error;
> -    if (!(p = strrchr(parent, '/'))) {
> -        virReportError(VIR_ERR_INVALID_ARG,
> -                       _("path '%s' is not absolute"),
> -                       pool->def->target.path);
> -        goto error;
> -    }
> -
> -    if (p != parent) {
> -        /* assure all directories in the path prior to the final dir
> -         * exist, with default uid/gid/mode. */
> -        *p = '\0';
> -        if (virFileMakePath(parent) < 0) {
> -            virReportSystemError(errno, _("cannot create path '%s'"),
> -                                 parent);
> -            goto error;
> -        }
> -    }
> -
> -    dir_create_flags = VIR_DIR_CREATE_ALLOW_EXIST;
>      needs_create_as_uid = (pool->def->type == VIR_STORAGE_POOL_NETFS);
> -    mode = pool->def->target.perms.mode;
> -
> -    if (mode == (mode_t) -1 &&
> -        (needs_create_as_uid || !virFileExists(pool->def->target.path)))
> -        mode = VIR_STORAGE_DEFAULT_POOL_PERM_MODE;
> -    if (needs_create_as_uid)
> -        dir_create_flags |= VIR_DIR_CREATE_AS_UID;
> -
> -    /* Now create the final dir in the path with the uid/gid/mode
> -     * requested in the config. If the dir already exists, just set
> -     * the perms. */
> -    if (virDirCreate(pool->def->target.path,
> -                     mode,
> -                     pool->def->target.perms.uid,
> -                     pool->def->target.perms.gid,
> -                     dir_create_flags) < 0)
> -        goto error;
> +    if (virDirPoolBuild(pool->def, needs_create_as_uid) < 0)

alter the first parameter to be "&pool->def->target", then pass the
default perms VIR_STORAGE_DEFAULT_POOL_PERM_MODE as a 2nd param

> +        return ret;
>  
>      if (flags != 0) {
>          ret = virStorageBackendMakeFileSystem(pool, flags);
> @@ -869,7 +830,6 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED,
>      }
>  
>   error:
> -    VIR_FREE(parent);
>      return ret;
>  }
>  
> @@ -1054,14 +1014,8 @@ virStorageBackendFileSystemDelete(virConnectPtr conn ATTRIBUTE_UNUSED,
>  
>      /* XXX delete all vols first ? */
>  
> -    if (rmdir(pool->def->target.path) < 0) {
> -        virReportSystemError(errno,
> -                             _("failed to remove pool '%s'"),
> -                             pool->def->target.path);
> -        return -1;
> -    }
> +    return virDirPoolDelete(pool->def->target.path);

Perhaps pass the "pool->def->target" since it contains both "path" and
"permissions"...

>  
> -    return 0;
>  }
>  
>  
> @@ -1084,27 +1038,11 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn ATTRIBUTE_UNUSED,
>      else
>          vol->type = VIR_STORAGE_VOL_FILE;
>  
> -    /* Volumes within a directory pools are not recursive; do not
> -     * allow escape to ../ or a subdir */
> -    if (strchr(vol->name, '/')) {
> -        virReportError(VIR_ERR_OPERATION_INVALID,
> -                       _("volume name '%s' cannot contain '/'"), vol->name);
> -        return -1;
> -    }
> -
>      VIR_FREE(vol->target.path);
> -    if (virAsprintf(&vol->target.path, "%s/%s",
> -                    pool->def->target.path,
> -                    vol->name) == -1)
> +    if (virDirItemCreate(vol->name, &vol->target.path,
> +                         pool->def->target.path) < 0)

    if (!(vol->target.path =
          virPoolTargetPathCreate(pool->def->target.path,
                                  vol->name)))

[it'll make sense later]

>          return -1;
>  
> -    if (virFileExists(vol->target.path)) {
> -        virReportError(VIR_ERR_OPERATION_INVALID,
> -                       _("volume target path '%s' already exists"),
> -                       vol->target.path);
> -        return -1;
> -    }
> -
>      VIR_FREE(vol->key);
>      return VIR_STRDUP(vol->key, vol->target.path);
>  }
> diff --git a/src/util/virpoolcommon.c b/src/util/virpoolcommon.c
> index 3ee6251..f002939 100644
> --- a/src/util/virpoolcommon.c
> +++ b/src/util/virpoolcommon.c
> @@ -123,3 +123,90 @@ virStoragePoolSourceListNewSource(virPoolSourceListPtr list)
>  
>      return source;
>  }
> +
> +int virDirPoolBuild(virPoolDefPtr pooldef, bool needs_create_as_uid)

int
virPoolBuildDir(const virPoolTarget *target,
                mode_t default_mode,
                bool needs_create_as_uuid)


target then be referenced as "target->$FIELD"

> +{
> +    int ret = -1;
> +    char *parent = NULL;
> +    char *p = NULL;
> +    mode_t mode;
> +    unsigned int dir_create_flags;
> +
> +    if (VIR_STRDUP(parent, pooldef->target.path) < 0)
> +        goto error;
> +    if (!(p = strrchr(parent, '/'))) {
> +        virReportError(VIR_ERR_INVALID_ARG,
> +                       _("path '%s' is not absolute"),
> +                       pooldef->target.path);
> +        goto error;
> +    }
> +
> +    if (p != parent) {
> +        /* assure all directories in the path prior to the final dir
> +         * exist, with default uid/gid/mode. */
> +        *p = '\0';
> +        if (virFileMakePath(parent) < 0) {
> +            virReportSystemError(errno, _("cannot create path '%s'"),
> +                                 parent);
> +            goto error;
> +        }
> +    }
> +
> +    dir_create_flags = VIR_DIR_CREATE_ALLOW_EXIST;
> +    mode = pooldef->target.perms.mode;
> +
> +    if (mode == (mode_t) -1 &&
> +        (needs_create_as_uid || !virFileExists(pooldef->target.path)))
> +        mode = VIR_STORAGE_DEFAULT_POOL_PERM_MODE;

The default should be passed.

> +    if (needs_create_as_uid)
> +        dir_create_flags |= VIR_DIR_CREATE_AS_UID;
> +
> +    /* Now create the final dir in the path with the uid/gid/mode
> +     * requested in the config. If the dir already exists, just set
> +     * the perms. */
> +    if (virDirCreate(pooldef->target.path,
> +                     mode,
> +                     pooldef->target.perms.uid,
> +                     pooldef->target.perms.gid,
> +                     dir_create_flags) < 0)
> +        goto error;
> +     ret = 0;
> + error:
> +    VIR_FREE(parent);
> +    return ret;
> +}
> +
> +int virDirPoolDelete(char *path)

virPoolDeleteDir taking pool as a parameter...

> +{
> +    if (rmdir(path) < 0) {

Hmm... me wonders if this should be virFileRemove instead...  In any
case, it could take a 'const virPoolTarget *target' and do the magic
from there.

> +        virReportSystemError(errno,
> +                             _("failed to remove pool '%s'"),
> +                             path);
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +int virDirItemCreate(char *name, char **itempath, char *poolpath)

char *
virPoolTargetPathCreate(char *poolpath, char *name)

> +{

    char *itempath;

> +
> +    if (strchr(name, '/')) {
> +        virReportError(VIR_ERR_OPERATION_INVALID,
> +                       _("name '%s' cannot contain '/'"), name);
> +        return -1;

return NULL;

> +    }
> +
> +    if (virAsprintf(itempath, "%s/%s",
> +                    poolpath, name) == -1)

    if (!(itempath = virFileBuildPath(poolpath, name, NULL)))
        return NULL;

> +        return -1;
> +
> +    if (virFileExists(*itempath)) {

s/*//

> +        virReportError(VIR_ERR_OPERATION_INVALID,
> +                       _("target path '%s' already exists"),
> +                       *itempath);

s/*//
> +        return -1;

    VIR_FREE(itempath);

(which will set itempath = NULL and...)

    return itempath;

will either be valid or NULL


> +    }
> +
> +    return 0;
> +}
> diff --git a/src/util/virpoolcommon.h b/src/util/virpoolcommon.h
> index c1c607f..37d642c 100644
> --- a/src/util/virpoolcommon.h
> +++ b/src/util/virpoolcommon.h
> @@ -25,6 +25,9 @@
>  # include "virthread.h"
>  # include "virpci.h"
>  
> +
> +# define VIR_STORAGE_DEFAULT_POOL_PERM_MODE 0755
> +

This should be passed and not set here as it's block storage driver
specific.

John

>  /*
>   * For remote pools, info on how to reach the host
>   */
> @@ -179,4 +182,8 @@ void virStoragePoolSourceFree(virPoolSourcePtr source);
>  void virStoragePoolDefFree(virPoolDefPtr def);
>  virPoolSourcePtr
>  virStoragePoolSourceListNewSource(virPoolSourceListPtr list);
> +/*Common functions fot directory backend*/
> +int virDirPoolBuild(virPoolDefPtr pooldef, bool is_ntfs);
> +int virDirPoolDelete(char *path);
> +int virDirItemCreate(char *name, char **itempath, char *poolname);
>  # endif /* __VIR_POOL_COMMON_H__ */
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux