[libvirt] [PATCH RFC v3 03/15] storage pools: refactoring of fs backend
John Ferlan
jferlan at redhat.com
Wed Dec 7 23:44:16 UTC 2016
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 at virtuozzo.com>
> ---
> 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__ */
>
More information about the libvir-list
mailing list