Re: [PATCH v2 3/5] storage: vstorage pool operations

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

 




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



[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