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

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(a)virtuozzo.com&gt; > --- > 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, > }; >