On 01/17/2017 09:10 AM, Olga Krishtal wrote:
> Added create/define/etc pool operations for vstorage backend.
>
> The vstorage storage pool looks like netfs ones. Due to this
> some of pool/volume functions were taken with no changes:
> refresh pool function.
Not exactly what I was expecting - perhaps I didn't explain well enough.
I was hoping you would create common API helpers and call from *_fs.c
and *_vstorage.c.
Of course, this is where that set of changes would start overlapping
with a what pkrempa is doing by creating a common storage_util.c
(currently the common place would be storage_backend.c) [1].
Since I'm watching [1] and I'd like for you to see progress here, I can
create the common functions once [1] is completed - which should be
soon. I'll mark places below with [2] for your reference.
>
> Signed-off-by: Olga Krishtal <okrishtal@xxxxxxxxxxxxx>
> ---
> src/storage/storage_backend_vstorage.c | 530 +++++++++++++++++++++++++++++++++
> 1 file changed, 530 insertions(+)
>
> diff --git a/src/storage/storage_backend_vstorage.c b/src/storage/storage_backend_vstorage.c
> index 3a57385..8332f4d 100644
> --- a/src/storage/storage_backend_vstorage.c
> +++ b/src/storage/storage_backend_vstorage.c
> @@ -6,11 +6,541 @@
> #include "storage_backend_vstorage.h"
> #include "virlog.h"
> #include "virstring.h"
> +#include <mntent.h>
> +#include <pwd.h>
> +#include <grp.h>
> +#include <sys/statvfs.h>
> +#include <sys/stat.h>
> +#include <fcntl.h>
[2] These 3 won't be necessary...
>
> #define VIR_FROM_THIS VIR_FROM_STORAGE
>
> +#define VIR_STORAGE_VOL_FS_OPEN_FLAGS (VIR_STORAGE_VOL_OPEN_DEFAULT | \
> + VIR_STORAGE_VOL_OPEN_DIR)
> +#define VIR_STORAGE_VOL_FS_PROBE_FLAGS (VIR_STORAGE_VOL_FS_OPEN_FLAGS | \
> + VIR_STORAGE_VOL_OPEN_NOERROR)
[2] These flags move to storage_backend.h near where
VIR_STORAGE_VOL_OPEN_DEFAULT is defined...
> +
> +
> +
Some strange spacing here. I think if you move the VIR_LOG_INIT up
betwen the #define's and have the right spacing it'll be OK.
> VIR_LOG_INIT("storage.storage_backend_vstorage");
Need at least an empty line between
> +/**
> + * @conn connection to report errors against
> + * @pool storage pool to build
> + * @flags controls the pool formatting behaviour
> + *
> + *
> + * If no flag is set, it only makes the directory;
> + * If VIR_STORAGE_POOL_BUILD_NO_OVERWRITE set, it determines if
> + * the directory exists and if yes - we use it. Otherwise - the new one
> + * will be created.
> + * If VIR_STORAGE_POOL_BUILD_OVERWRITE is set, the dircetory for pool
> + * will used but the content and permissions will be update
see [3]
> + *
> + * Returns 0 on success, -1 on error
> + */
> +
> +static int
> +virStorageBackendVzPoolBuild(virConnectPtr conn ATTRIBUTE_UNUSED,
> + virStoragePoolObjPtr pool,
> + unsigned int flags)
> +{
[2 - snip most into virStorageBackendBuildFileDirCommon()]
keep:
unsigned int dir_create_flags = 0;
> + int ret = -1;
> + char *parent = NULL;
> + char *p = NULL;
> + mode_t mode;
> + unsigned int dir_create_flags;
> +
> + virCheckFlags(VIR_STORAGE_POOL_BUILD_OVERWRITE |
> + VIR_STORAGE_POOL_BUILD_NO_OVERWRITE, ret);
> +
> + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_STORAGE_POOL_BUILD_OVERWRITE,
> + 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;
> + mode = pool->def->target.perms.mode;
> +
> + if (mode == (mode_t) -1 && !virFileExists(pool->def->target.path))
> + mode = VIR_STORAGE_DEFAULT_POOL_PERM_MODE;
> +
> + /* 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;
[2 end snip]
> +
> + /* Delete directory content if flag is set*/
should be "set */"
> + if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE)
> + if (virFileDeleteTree(pool->def->target.path))
> + goto error;
[3] This block doesn't make sense in light of the virDirCreate just
above *and* the intro comments.
If you want to support [NO_]OVERWRITE, then prior to calling the common
function we can play with flags, thus you end up with just:
if (flags & VIR_STORAGE_POOL_BUILD_OVERWRITE)
dir_create_flags |= VIR_DIR_CREATE_ALLOW_EXIST;
return virStorageBackendBuildFileDirCommon(pool, dir_create_flags);
The *_fs.c code would be
unsigned int dir_create_flags = VIR_DIR_CREATE_ALLOW_EXIST;
...
if (virStorageBackendBuildFileDirCommon(pool, dir_create_flags) < 0)
goto error;
...
and the common code would receive dir_create_flags
The comments would then need to be updated a bit... If you have a
suggestion or would prefer to just follow the *_fs.c model - I can
adjust that too - let me know.
> +
> + ret = 0;
> +
> + error:
> + VIR_FREE(parent);
> + return ret;
> +}
> +
> +static int
> +virStorageBackendVzPoolStart(virConnectPtr conn ATTRIBUTE_UNUSED,
> + virStoragePoolObjPtr pool)
> +{
> + int ret = -1;
> + virCommandPtr cmd = NULL;
> + char *grp_name = NULL;
> + char *usr_name = NULL;
> + char *mode = NULL;
> +
> + /* Check the permissions */
> + if (pool->def->target.perms.mode == (mode_t) - 1)
> + pool->def->target.perms.mode = VIR_STORAGE_DEFAULT_POOL_PERM_MODE;
> + if (pool->def->target.perms.uid == (uid_t) -1)
> + pool->def->target.perms.uid = geteuid();
> + if (pool->def->target.perms.gid == (gid_t) -1)
> + pool->def->target.perms.gid = getegid();
> +
> + /* Convert ids to names because vstorage uses names */
> +
> + grp_name = virGetGroupName(pool->def->target.perms.gid);
> + if (!grp_name)
> + return -1;
> +
> + usr_name = virGetUserName(pool->def->target.perms.uid);
> + if (!usr_name)
> + return -1;
Leak grp_name
> +
> + if (virAsprintf(&mode, "%o", pool->def->target.perms.mode) < 0)
> + return -1;
Leak grp_name, usr_name
Each of these changes to a goto cleanup;
> +
> + cmd = virCommandNewArgList(VSTORAGE_MOUNT,
> + "-c", pool->def->source.name,
> + pool->def->target.path,
> + "-m", mode,
> + "-g", grp_name, "-u", usr_name,
> + NULL);
> +
> + if (virCommandRun(cmd, NULL) < 0)
> + goto cleanup;
> + ret = 0;
> +
> + cleanup:
> + virCommandFree(cmd);
> + VIR_FREE(mode);
> + VIR_FREE(grp_name);
> + VIR_FREE(usr_name);
> + return ret;
> +}
> +
> +static int
> +virStorageBackendVzIsMounted(virStoragePoolObjPtr pool)
> +{
> + int ret = -1;
> + char *src = NULL;
Never used - I'll remove
> + FILE *mtab;
> + struct mntent ent;
> + char buf[1024];
> + char *cluster = NULL;
> +
> + if (virAsprintf(&cluster, "vstorage://%s", pool->def->source.name) < 0)
> + return -1;
> +
> + if ((mtab = fopen(_PATH_MOUNTED, "r")) == NULL) {
> + virReportSystemError(errno,
> + _("cannot read mount list '%s'"),
> + _PATH_MOUNTED);
> + goto cleanup;
> + }
> +
> + while ((getmntent_r(mtab, &ent, buf, sizeof(buf))) != NULL) {
> +
> + if (STREQ(ent.mnt_dir, pool->def->target.path) &&
> + STREQ(ent.mnt_fsname, cluster)) {
> + ret = 1;
> + goto cleanup;
> + }
> +
> + VIR_FREE(src);
> + }
> +
> + ret = 0;
> +
> + cleanup:
> + VIR_FORCE_FCLOSE(mtab);
> + VIR_FREE(src);
> + VIR_FREE(cluster);
> + return ret;
> +}
> +
> +static int
> +virStorageBackendVzUmount(virStoragePoolObjPtr pool)
> +{
> + virCommandPtr cmd = NULL;
> + int ret = -1;
> + int rc;
> +
> + /* Short-circuit if already unmounted */
> + if ((rc = virStorageBackendVzIsMounted(pool)) != 1)
> + return rc;
> +
[2] The following has commonality with _fs.c - use "return
virStorageBackendUmountCommon(pool);"
> + cmd = virCommandNewArgList(UMOUNT,
> + pool->def->target.path,
> + NULL);
> +
> + if (virCommandRun(cmd, NULL) < 0)
> + goto cleanup;
> +
> + ret = 0;
> + cleanup:
> + virCommandFree(cmd);
> + return ret;
> +}
> +
> +static int
> +virStorageBackendVzPoolStop(virConnectPtr conn ATTRIBUTE_UNUSED,
> + virStoragePoolObjPtr pool)
> +{
> + if (virStorageBackendVzUmount(pool) < 0)
> + return -1;
> +
> + return 0;
> +}
> +
> +/**
> + * @conn connection to report errors against
> + * @pool storage pool to delete
> + *
> + * Deletes vstorage based storage pool.
> + * At this moment vstorage is in umounted state - so we do not
> + * need to delete volumes first.
> + * Returns 0 on success, -1 on error
> + */
> +static int
> +virStorageBackendVzDelete(virConnectPtr conn ATTRIBUTE_UNUSED,
> + virStoragePoolObjPtr pool,
> + unsigned int flags)
> +{
> + virCheckFlags(0, -1);
[2] This could be a return virStorageBackendDeleteDirCommon(pool);
> +
> + if (rmdir(pool->def->target.path) < 0) {
> + virReportSystemError(errno,
> + _("failed to remove pool '%s'"),
> + pool->def->target.path);
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Check wether the cluster is mounted
whether
> + */
> +static int
> +virStorageBackendVzCheck(virStoragePoolObjPtr pool,
> + bool *isActive)
> +{
> + int ret = -1;
> + *isActive = false;
> + if ((ret = virStorageBackendVzIsMounted(pool)) != 0) {
> + if (ret < 0)
> + return -1;
> + *isActive = true;
> + }
> +
> + return 0;
> +}
> +
[2 - start] - This whole hunk moves to common module
> +/* All the underlying functions were directly copied from
> + * filesystem backend with no changes.
> + */
> +
> +static int
> +virStorageBackendProbeTarget(virStorageSourcePtr target,
> + virStorageEncryptionPtr *encryption)
> +{
> + int backingStoreFormat;
> + int fd = -1;
> + int ret = -1;
> + int rc;
> + virStorageSourcePtr meta = NULL;
> + struct stat sb;
> +
> + if (encryption)
> + *encryption = NULL;
> +
> + if ((rc = virStorageBackendVolOpen(target->path, &sb,
> + VIR_STORAGE_VOL_FS_PROBE_FLAGS)) < 0)
> + return rc; /* Take care to propagate rc, it is not always -1 */
> + fd = rc;
> +
> + if (virStorageBackendUpdateVolTargetInfoFD(target, fd, &sb) < 0)
> + goto cleanup;
> +
> + if (S_ISDIR(sb.st_mode)) {
> + if (virStorageBackendIsPloopDir(target->path)) {
> + if (virStorageBackendRedoPloopUpdate(target, &sb, &fd,
> + VIR_STORAGE_VOL_FS_PROBE_FLAGS) < 0)
> + goto cleanup;
> + } else {
> + target->format = VIR_STORAGE_FILE_DIR;
> + ret = 0;
> + goto cleanup;
> + }
> + }
> +
> + if (!(meta = virStorageFileGetMetadataFromFD(target->path,
> + fd,
> + VIR_STORAGE_FILE_AUTO,
> + &backingStoreFormat)))
> + goto cleanup;
> +
> + if (meta->backingStoreRaw) {
> + if (!(target->backingStore = virStorageSourceNewFromBacking(meta)))
> + goto cleanup;
> +
> + target->backingStore->format = backingStoreFormat;
> +
> + /* XXX: Remote storage doesn't play nicely with volumes backed by
> + * remote storage. To avoid trouble, just fake the backing store is RAW
> + * and put the string from the metadata as the path of the target. */
> + if (!virStorageSourceIsLocalStorage(target->backingStore)) {
> + virStorageSourceFree(target->backingStore);
> +
> + if (VIR_ALLOC(target->backingStore) < 0)
> + goto cleanup;
> +
> + target->backingStore->type = VIR_STORAGE_TYPE_NETWORK;
> + target->backingStore->path = meta->backingStoreRaw;
> + meta->backingStoreRaw = NULL;
> + target->backingStore->format = VIR_STORAGE_FILE_RAW;
> + }
> +
> + if (target->backingStore->format == VIR_STORAGE_FILE_AUTO) {
> + if ((rc = virStorageFileProbeFormat(target->backingStore->path,
> + -1, -1)) < 0) {
> + /* If the backing file is currently unavailable or is
> + * accessed via remote protocol only log an error, fake the
> + * format as RAW and continue. Returning -1 here would
> + * disable the whole storage pool, making it unavailable for
> + * even maintenance. */
> + target->backingStore->format = VIR_STORAGE_FILE_RAW;
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("cannot probe backing volume format: %s"),
> + target->backingStore->path);
> + } else {
> + target->backingStore->format = rc;
> + }
> + }
> + }
> +
> + target->format = meta->format;
> +
> + /* Default to success below this point */
> + ret = 0;
> +
> + if (meta->capacity)
> + target->capacity = meta->capacity;
> +
> + if (encryption && meta->encryption) {
> + *encryption = meta->encryption;
> + meta->encryption = NULL;
> +
> + /* XXX ideally we'd fill in secret UUID here
> + * but we cannot guarantee 'conn' is non-NULL
> + * at this point in time :-( So we only fill
> + * in secrets when someone first queries a vol
> + */
> + }
> +
> + virBitmapFree(target->features);
> + target->features = meta->features;
> + meta->features = NULL;
> +
> + if (meta->compat) {
> + VIR_FREE(target->compat);
> + target->compat = meta->compat;
> + meta->compat = NULL;
> + }
> +
> + cleanup:
> + VIR_FORCE_CLOSE(fd);
> + virStorageSourceFree(meta);
> + return ret;
> +
> +}
[2] - end
> +
> +/**
> + * The same as for fs/dir storage pools
> + * Iterate over the pool's directory and enumerate all disk images
> + * within it. This is non-recursive.
> + */
Scratch the comment
> +
> +static int
> +virStorageBackendVzRefresh(virConnectPtr conn ATTRIBUTE_UNUSED,
> + virStoragePoolObjPtr pool)
> +{
[2] all the code moves:
return virStorageBackendRefreshFileDirCommon(pool);
> + DIR *dir;
> + struct dirent *ent;
> + struct statvfs sb;
> + struct stat statbuf;
> + virStorageVolDefPtr vol = NULL;
> + virStorageSourcePtr target = NULL;
> + int direrr;
> + int fd = -1, ret = -1;
> +
> + if (virDirOpen(&dir, pool->def->target.path) < 0)
> + goto cleanup;
> +
> + while ((direrr = virDirRead(dir, &ent, pool->def->target.path)) > 0) {
> + int err;
> +
> + if (virStringHasControlChars(ent->d_name)) {
> + VIR_WARN("Ignoring file with control characters under '%s'",
> + pool->def->target.path);
> + continue;
> + }
> +
> + if (VIR_ALLOC(vol) < 0)
> + goto cleanup;
> +
> + if (VIR_STRDUP(vol->name, ent->d_name) < 0)
> + goto cleanup;
> +
> + vol->type = VIR_STORAGE_VOL_FILE;
> + vol->target.format = VIR_STORAGE_FILE_RAW; /* Real value is filled in during probe */
> + if (virAsprintf(&vol->target.path, "%s/%s",
> + pool->def->target.path,
> + vol->name) == -1)
> + goto cleanup;
> +
> + if (VIR_STRDUP(vol->key, vol->target.path) < 0)
> + goto cleanup;
> +
> + if ((err = virStorageBackendProbeTarget(&vol->target,
> + &vol->target.encryption)) < 0) {
> + if (err == -2) {
> + /* Silently ignore non-regular files,
> + * eg 'lost+found', dangling symbolic link */
> + virStorageVolDefFree(vol);
> + vol = NULL;
> + continue;
> + } else if (err == -3) {
> + /* The backing file is currently unavailable, its format is not
> + * explicitly specified, the probe to auto detect the format
> + * failed: continue with faked RAW format, since AUTO will
> + * break virStorageVolTargetDefFormat() generating the line
> + * <format type='...'/>. */
> + } else {
> + goto cleanup;
> + }
> + }
> +
> + /* directory based volume */
> + if (vol->target.format == VIR_STORAGE_FILE_DIR)
> + vol->type = VIR_STORAGE_VOL_DIR;
> +
> + if (vol->target.format == VIR_STORAGE_FILE_PLOOP)
> + vol->type = VIR_STORAGE_VOL_PLOOP;
> +
> + if (vol->target.backingStore) {
> + ignore_value(virStorageBackendUpdateVolTargetInfo(vol->target.backingStore,
> + false,
> + VIR_STORAGE_VOL_OPEN_DEFAULT, 0));
NB: I had a patch here today - this is why cut/copy/paste is not
preferred...
> + /* If this failed, the backing file is currently unavailable,
> + * the capacity, allocation, owner, group and mode are unknown.
> + * An error message was raised, but we just continue. */
> + }
> +
> + if (VIR_APPEND_ELEMENT(pool->volumes.objs, pool->volumes.count, vol) < 0)
> + goto cleanup;
> + }
> + if (direrr < 0)
> + goto cleanup;
> + VIR_DIR_CLOSE(dir);
> + vol = NULL;
> +
> + if (VIR_ALLOC(target))
> + goto cleanup;
> +
> + if ((fd = open(pool->def->target.path, O_RDONLY)) < 0) {
> + virReportSystemError(errno,
> + _("cannot open path '%s'"),
> + pool->def->target.path);
> + goto cleanup;
> + }
> +
> + if (fstat(fd, &statbuf) < 0) {
> + virReportSystemError(errno,
> + _("cannot stat path '%s'"),
> + pool->def->target.path);
> + goto cleanup;
> + }
> +
> + if (virStorageBackendUpdateVolTargetInfoFD(target, fd, &statbuf) < 0)
> + goto cleanup;
> +
> + /* VolTargetInfoFD doesn't update capacity correctly for the pool case */
> + if (statvfs(pool->def->target.path, &sb) < 0) {
> + virReportSystemError(errno,
> + _("cannot statvfs path '%s'"),
> + pool->def->target.path);
> + goto cleanup;
> + }
> +
> + pool->def->capacity = ((unsigned long long)sb.f_frsize *
> + (unsigned long long)sb.f_blocks);
> + pool->def->available = ((unsigned long long)sb.f_bfree *
> + (unsigned long long)sb.f_frsize);
> + pool->def->allocation = pool->def->capacity - pool->def->available;
> +
> + pool->def->target.perms.mode = target->perms->mode;
> + pool->def->target.perms.uid = target->perms->uid;
> + pool->def->target.perms.gid = target->perms->gid;
> + VIR_FREE(pool->def->target.perms.label);
> + if (VIR_STRDUP(pool->def->target.perms.label, target->perms->label) < 0)
> + goto cleanup;
> +
> + ret = 0;
> + cleanup:
> + VIR_DIR_CLOSE(dir);
> + VIR_FORCE_CLOSE(fd);
> + virStorageVolDefFree(vol);
> + virStorageSourceFree(target);
> + if (ret < 0)
> + virStoragePoolObjClearVols(pool);
> + return ret;
> +}
[2 - end]
Once this is all done - this file is much shorter and you gain the
benefit of any adjustments to the common code - which will happen...
and this code wouldn't be updated...
I'll make the updates though - it won't happen overnight - should be by
the end of the week.
John
>
> virStorageBackend virStorageBackendVstorage = {
> .type = VIR_STORAGE_POOL_VSTORAGE,
> +
> + .buildPool = virStorageBackendVzPoolBuild,
> + .startPool = virStorageBackendVzPoolStart,
> + .stopPool = virStorageBackendVzPoolStop,
> + .deletePool = virStorageBackendVzDelete,
> + .refreshPool = virStorageBackendVzRefresh,
> + .checkPool = virStorageBackendVzCheck,
> };
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
