[libvirt] [PATCH v2 3/5] storage: vstorage pool operations
Olga Krishtal
okrishtal at virtuozzo.com
Thu Jan 19 14:26:59 UTC 2017
On 19/01/17 00:04, John Ferlan wrote:
>
> 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.
I will follow changes
>
>> Signed-off-by: Olga Krishtal <okrishtal at virtuozzo.com>
>> ---
>> 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.
>
follow the *_fs.c model seems more reasonable
>> +
>> + 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,
>> };
>>
--
Best regards,
Olga
More information about the libvir-list
mailing list