Gluster storage works on a similar principle to NFS where it takes the uid and gid of the actual process and uses it to access the storage volume on the remote server. This introduces a need to chown storage files on gluster via native API. --- src/storage/storage_backend.h | 6 ++++++ src/storage/storage_backend_fs.c | 12 ++++++++++++ src/storage/storage_backend_gluster.c | 12 ++++++++++++ src/storage/storage_driver.c | 28 ++++++++++++++++++++++++++++ src/storage/storage_driver.h | 1 + 5 files changed, 59 insertions(+) diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index e48da5b..a93df5d 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -244,6 +244,11 @@ typedef int (*virStorageFileBackendAccess)(virStorageSourcePtr src, int mode); +typedef int +(*virStorageFileBackendChown)(virStorageSourcePtr src, + uid_t uid, + gid_t gid); + virStorageFileBackendPtr virStorageFileBackendForType(int type, int protocol); virStorageFileBackendPtr virStorageFileBackendForTypeInternal(int type, int protocol, @@ -269,6 +274,7 @@ struct _virStorageFileBackend { virStorageFileBackendUnlink storageFileUnlink; virStorageFileBackendStat storageFileStat; virStorageFileBackendAccess storageFileAccess; + virStorageFileBackendChown storageFileChown; }; #endif /* __VIR_STORAGE_BACKEND_H__ */ diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 40cc1f4..2af5ab5 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1455,6 +1455,15 @@ virStorageFileBackendFileAccess(virStorageSourcePtr src, } +static int +virStorageFileBackendFileChown(virStorageSourcePtr src, + uid_t uid, + gid_t gid) +{ + return chown(src->path, uid, gid); +} + + virStorageFileBackend virStorageFileBackendFile = { .type = VIR_STORAGE_TYPE_FILE, @@ -1465,6 +1474,7 @@ virStorageFileBackend virStorageFileBackendFile = { .storageFileStat = virStorageFileBackendFileStat, .storageFileReadHeader = virStorageFileBackendFileReadHeader, .storageFileAccess = virStorageFileBackendFileAccess, + .storageFileChown = virStorageFileBackendFileChown, .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, }; @@ -1479,6 +1489,7 @@ virStorageFileBackend virStorageFileBackendBlock = { .storageFileStat = virStorageFileBackendFileStat, .storageFileReadHeader = virStorageFileBackendFileReadHeader, .storageFileAccess = virStorageFileBackendFileAccess, + .storageFileChown = virStorageFileBackendFileChown, .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, }; @@ -1491,6 +1502,7 @@ virStorageFileBackend virStorageFileBackendDir = { .backendDeinit = virStorageFileBackendFileDeinit, .storageFileAccess = virStorageFileBackendFileAccess, + .storageFileChown = virStorageFileBackendFileChown, .storageFileGetUniqueIdentifier = virStorageFileBackendFileGetUniqueIdentifier, }; diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 0f8f0f3..052f58d 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -762,6 +762,17 @@ virStorageFileBackendGlusterGetUniqueIdentifier(virStorageSourcePtr src) } +static int +virStorageFileBackendGlusterChown(virStorageSourcePtr src, + uid_t uid, + gid_t gid) +{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + + return glfs_chown(priv->vol, src->path, uid, gid); +} + + virStorageFileBackend virStorageFileBackendGluster = { .type = VIR_STORAGE_TYPE_NETWORK, .protocol = VIR_STORAGE_NET_PROTOCOL_GLUSTER, @@ -773,6 +784,7 @@ virStorageFileBackend virStorageFileBackendGluster = { .storageFileStat = virStorageFileBackendGlusterStat, .storageFileReadHeader = virStorageFileBackendGlusterReadHeader, .storageFileAccess = virStorageFileBackendGlusterAccess, + .storageFileChown = virStorageFileBackendGlusterChown, .storageFileGetUniqueIdentifier = virStorageFileBackendGlusterGetUniqueIdentifier, diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 3830867..efd79b9 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -2613,6 +2613,34 @@ virStorageFileAccess(virStorageSourcePtr src, } +/** + * virStorageFileChown: Change owner of a storage file + * + * @src: storage file to change owner of + * @uid: new owner id + * @gid: new group id + * + * Returns 0 on success, -1 on error and sets errno. No libvirt + * error is reported. Returns -2 if the operation isn't supported + * by libvirt storage backend. + */ +int +virStorageFileChown(virStorageSourcePtr src, + uid_t uid, + gid_t gid) +{ + if (!virStorageFileIsInitialized(src) || + !src->drv->backend->storageFileChown) { + errno = ENOSYS; + return -2; + } + + VIR_DEBUG("chown of storage file %p to %d:%d", src, uid, gid); + + return src->drv->backend->storageFileChown(src, uid, gid); +} + + /* Recursive workhorse for virStorageFileGetMetadata. */ static int virStorageFileGetMetadataRecurse(virStorageSourcePtr src, diff --git a/src/storage/storage_driver.h b/src/storage/storage_driver.h index bd9e9ed..eefd766 100644 --- a/src/storage/storage_driver.h +++ b/src/storage/storage_driver.h @@ -43,6 +43,7 @@ ssize_t virStorageFileReadHeader(virStorageSourcePtr src, char **buf); const char *virStorageFileGetUniqueIdentifier(virStorageSourcePtr src); int virStorageFileAccess(virStorageSourcePtr src, int mode); +int virStorageFileChown(virStorageSourcePtr src, uid_t uid, gid_t gid); int virStorageFileGetMetadata(virStorageSourcePtr src, uid_t uid, gid_t gid, -- 2.0.0

When restoring security labels in the dac driver the code would resolve the file path and use the resolved one to be chown-ed. The setting code doesn't do that. Remove the unnecessary code. --- src/security/security_dac.c | 19 +------------------ 1 file changed, 1 insertion(+), 18 deletions(-) diff --git a/src/security/security_dac.c b/src/security/security_dac.c index 4d2a9d6..cdb2735 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -264,27 +264,10 @@ virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid) static int virSecurityDACRestoreSecurityFileLabel(const char *path) { - struct stat buf; - int rc = -1; - char *newpath = NULL; - VIR_INFO("Restoring DAC user and group on '%s'", path); - if (virFileResolveLink(path, &newpath) < 0) { - virReportSystemError(errno, - _("cannot resolve symlink %s"), path); - goto err; - } - - if (stat(newpath, &buf) != 0) - goto err; - /* XXX record previous ownership */ - rc = virSecurityDACSetOwnership(newpath, 0, 0); - - err: - VIR_FREE(newpath); - return rc; + return virSecurityDACSetOwnership(path, 0, 0); } -- 2.0.0

On 07/22/2014 05:20 AM, Peter Krempa wrote: ACK Although I'm still not sure I completely follow how or what role the cfg->user and cfg->group 'play'.... or if there needs to be a relationship with the chownCallback. John

On 07/22/2014 05:20 AM, Peter Krempa wrote: This is the 'src' option and 'path' is NULL Thus rc == -1, path == NULL Maybe a "if !src && !path" return 0 should be before the VIR_INFO... Maybe with a VIR_DEBUG that'll help someone some day figure out why they aren't getting what they were expecting. Just a thought... Reusing a passed parameter is where things get dicey for me. I think we can get here with path == NULL and rc == -1, resulting in subsequent usage of '%s' for path to have 'nil', right? I know this just follows the previous code, but in the big picture - how does this "play" in with future patches where fs & gluster will seemingly go though this path? ACK - in general with the 'path' issue above understood. For this code, I'm mostly curious. John

Use the storage driver to chown remote images. --- src/qemu/qemu_driver.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a5a9e0f..c0e6485 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -327,6 +327,52 @@ qemuAutostartDomains(virQEMUDriverPtr driver) virObjectUnref(cfg); } + +static int +qemuSecurityChownCallback(virStorageSourcePtr src, + uid_t uid, + gid_t gid) +{ + struct stat sb; + int save_errno = 0; + int ret = -1; + + if (!virStorageFileSupportsSecurityDriver(src)) + return 0; + + if (virStorageSourceIsLocalStorage(src)) { + /* use direct chmod for local files so that the file doesn't + * need to be initialized */ + if (stat(src->path, &sb) >= 0) { + if (sb.st_uid == uid && + sb.st_gid == gid) { + /* It's alright, there's nothing to change anyway. */ + return 0; + } + } + + return chown(src->path, uid, gid); + } + + /* storage file init reports errors, return -2 on failure */ + if (virStorageFileInit(src) < 0) + return -2; + + if (virStorageFileChown(src, uid, gid) < 0) { + save_errno = errno; + goto cleanup; + } + + ret = 0; + + cleanup: + virStorageFileDeinit(src); + errno = save_errno; + + return ret; +} + + static int qemuSecurityInit(virQEMUDriverPtr driver) { @@ -375,7 +421,7 @@ qemuSecurityInit(virQEMUDriverPtr driver) cfg->securityDefaultConfined, cfg->securityRequireConfined, cfg->dynamicOwnership, - NULL))) + qemuSecurityChownCallback))) goto error; if (!stack) { if (!(stack = virSecurityManagerNewStack(mgr))) -- 2.0.0

Add backends for this frontend function so that we can use it in the snapshot creation code. --- src/storage/storage_backend_fs.c | 17 +++++++++++++++++ src/storage/storage_backend_gluster.c | 15 +++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 2af5ab5..378c553 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -1387,6 +1387,22 @@ virStorageFileBackendFileInit(virStorageSourcePtr src) static int +virStorageFileBackendFileCreate(virStorageSourcePtr src) +{ + int fd = -1; + + if ((fd = virFileOpenAs(src->path, O_WRONLY | O_TRUNC | O_CREAT, 0, + src->drv->uid, src->drv->gid, 0)) < 0) { + errno = -fd; + return -1; + } + + VIR_FORCE_CLOSE(fd); + return 0; +} + + +static int virStorageFileBackendFileUnlink(virStorageSourcePtr src) { return unlink(src->path); @@ -1470,6 +1486,7 @@ virStorageFileBackend virStorageFileBackendFile = { .backendInit = virStorageFileBackendFileInit, .backendDeinit = virStorageFileBackendFileDeinit, + .storageFileCreate = virStorageFileBackendFileCreate, .storageFileUnlink = virStorageFileBackendFileUnlink, .storageFileStat = virStorageFileBackendFileStat, .storageFileReadHeader = virStorageFileBackendFileReadHeader, diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index 052f58d..38d02ac 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -634,6 +634,20 @@ virStorageFileBackendGlusterInit(virStorageSourcePtr src) static int +virStorageFileBackendGlusterCreate(virStorageSourcePtr src) +{ + virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; + glfs_fd_t *fd = NULL; + + if (!(fd = glfs_open(priv->vol, src->path, O_CREAT | O_TRUNC | O_WRONLY))) + return -1; + + ignore_value(glfs_close(fd)); + return 0; +} + + +static int virStorageFileBackendGlusterUnlink(virStorageSourcePtr src) { virStorageFileBackendGlusterPrivPtr priv = src->drv->priv; @@ -780,6 +794,7 @@ virStorageFileBackend virStorageFileBackendGluster = { .backendInit = virStorageFileBackendGlusterInit, .backendDeinit = virStorageFileBackendGlusterDeinit, + .storageFileCreate = virStorageFileBackendGlusterCreate, .storageFileUnlink = virStorageFileBackendGlusterUnlink, .storageFileStat = virStorageFileBackendGlusterStat, .storageFileReadHeader = virStorageFileBackendGlusterReadHeader, -- 2.0.0

Move the last operation done on local files to the storage driver API. --- src/qemu/qemu_driver.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c0e6485..f0cf1c6 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -12855,7 +12855,6 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, char *source = NULL; const char *formatStr = NULL; int ret = -1; - int fd = -1; bool need_unlink = false; if (snap->snapshot != VIR_DOMAIN_SNAPSHOT_LOCATION_EXTERNAL) { @@ -12873,7 +12872,7 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, if (virStorageSourceInitChainElement(newDiskSrc, disk->src, false) < 0) goto cleanup; - if (virStorageFileInit(newDiskSrc) < 0) + if (qemuDomainStorageFileInit(driver, vm, newDiskSrc) < 0) goto cleanup; if (qemuGetDriveSourceString(newDiskSrc, NULL, &source) < 0) @@ -12889,15 +12888,13 @@ qemuDomainSnapshotCreateSingleDiskActive(virQEMUDriverPtr driver, } /* pre-create the image file so that we can label it before handing it to qemu */ - /* XXX we should switch to storage driver based pre-creation of the image */ - if (virStorageSourceIsLocalStorage(newDiskSrc)) { - if (!reuse && newDiskSrc->type != VIR_STORAGE_TYPE_BLOCK) { - fd = qemuOpenFile(driver, vm, source, O_WRONLY | O_TRUNC | O_CREAT, - &need_unlink, NULL); - if (fd < 0) - goto cleanup; - VIR_FORCE_CLOSE(fd); + if (!reuse && newDiskSrc->type != VIR_STORAGE_TYPE_BLOCK) { + if (virStorageFileCreate(newDiskSrc) < 0) { + virReportSystemError(errno, _("failed to create image file '%s'"), + source); + goto cleanup; } + need_unlink = true; } /* set correct security, cgroup and locking options on the new image */ -- 2.0.0