[libvirt] [PATCH 0/2] rework virFileCreate into virFileOperation w/hook function
(This patchset requires that the virFork() patchset I send a couple hours ago be applied first, otherwise it will fail to apply). virFileCreate just didn't have what it takes to get the job done. It turns out that there are other things that must be done to files as the qemu (or whatever) user, so this patchset adds a hook function that gets called during the child process. That solves a problem I found with creating raw storage volumes on root-squash NFS, and will also be used for an upcoming domain-save-on-root-squash-nfs patch. I'm now beginning to think that virFileCreate/virFileOperation is just too narrow in scope, and it's getting too many options. Possibly it will be better to just make a simpler virCallasUID() function that gets a pointer to a function to execute in the child process as an argument and does nothing but fork/setuid/call the function. That function would then contain *all* of the file operations, including creating/opening/closing the file. But that's too large of a change to contemplate so soon before a release, and this functionality is necessary for two important bug fixes (mentioned above), so...
It turns out it is also useful to be able to perform other operations on a file created while running as a different uid (eg, write things to that file), and possibly to do this to a file that already exists. This patch adds an optional hook function to the renamed (for more accuracy of purpose) virFileOperation; the hook will be called after the file has been opened (possibly created) and gid/mode checked/set, before closing it. As with the other operations on the file, if the VIR_FILE_OP_AS_UID flag is set, this hook function will be called in the context of a child process forked from the process that called virFileOperation. The implication here is that, while all data in memory is available to this hook function, any modification to that data will not be seen by the caller - the only indication in memory of what happened in the hook will be the return value (which the hook should set to 0 on success, or one of the standard errno values on failure). Another piece of making the function more flexible was to add an "openflags" argument. This arg should contain exactly the flags to be passed to open(2), eg O_RDWR | O_EXCL, etc. In the process of adding the hook to virFileOperation, I also realized that the bits to fix up file owner/group/mode settings after creation were being done in the parent process, which could fail, so I moved them to the child process where they should be. util/util.c|h: rename and rework virFileCreate-->virFileOperation, and redo flags in virDirCreate. storage/storage_backend.c, storage/storage_backend_fs.c: update the calls to virFileOperation/virDirCreate to reflect changes in the API, but don't yet take advantage of the hook. --- src/storage/storage_backend.c | 11 ++- src/storage/storage_backend_fs.c | 7 +- src/util/util.c | 156 ++++++++++++++++++++------------------ src/util/util.h | 19 ++++- 4 files changed, 106 insertions(+), 87 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index a12ddc7..07c116a 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -290,10 +290,13 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, return -1; } - if ((createstat = virFileCreate(vol->target.path, vol->target.perms.mode, - vol->target.perms.uid, vol->target.perms.gid, - (pool->def->type == VIR_STORAGE_POOL_NETFS - ? VIR_FILE_CREATE_AS_UID : 0))) < 0) { + if ((createstat = virFileOperation(vol->target.path, O_RDWR | O_CREAT | O_EXCL, + vol->target.perms.mode, + vol->target.perms.uid, vol->target.perms.gid, + NULL, NULL, + VIR_FILE_OP_FORCE_PERMS | + (pool->def->type == VIR_STORAGE_POOL_NETFS + ? VIR_FILE_OP_AS_UID : 0))) < 0) { virReportSystemError(createstat, _("cannot create path '%s'"), vol->target.path); diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index bbd5787..8279d78 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -533,9 +533,9 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, pool->def->target.perms.mode, pool->def->target.perms.uid, pool->def->target.perms.gid, - VIR_FILE_CREATE_ALLOW_EXIST | + VIR_DIR_CREATE_FORCE_PERMS | VIR_DIR_CREATE_ALLOW_EXIST | (pool->def->type == VIR_STORAGE_POOL_NETFS - ? VIR_FILE_CREATE_AS_UID : 0)) != 0)) { + ? VIR_DIR_CREATE_AS_UID : 0)) != 0)) { virReportSystemError(err, _("cannot create path '%s'"), pool->def->target.path); goto error; @@ -779,8 +779,9 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED, if ((err = virDirCreate(vol->target.path, vol->target.perms.mode, vol->target.perms.uid, vol->target.perms.gid, + VIR_DIR_CREATE_FORCE_PERMS | (pool->def->type == VIR_STORAGE_POOL_NETFS - ? VIR_FILE_CREATE_AS_UID : 0))) != 0) { + ? VIR_DIR_CREATE_AS_UID : 0))) != 0) { virReportSystemError(err, _("cannot create path '%s'"), vol->target.path); return -1; diff --git a/src/util/util.c b/src/util/util.c index c3b4084..d3bbfe1 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1212,15 +1212,15 @@ int virFileExists(const char *path) } -static int virFileCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid, - unsigned int flags) { - int open_flags = O_RDWR | O_CREAT | ((flags & VIR_FILE_CREATE_ALLOW_EXIST) - ? 0 : O_EXCL); +static int virFileOperationNoFork(const char *path, int openflags, mode_t mode, + uid_t uid, gid_t gid, + virFileOperationHook hook, void *hookdata, + unsigned int flags) { int fd = -1; int ret = 0; struct stat st; - if ((fd = open(path, open_flags, mode)) < 0) { + if ((fd = open(path, openflags, mode)) < 0) { ret = errno; virReportSystemError(errno, _("failed to create file '%s'"), path); @@ -1238,13 +1238,17 @@ static int virFileCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t g path, uid, gid); goto error; } - if (fchmod(fd, mode) < 0) { + if ((flags & VIR_FILE_OP_FORCE_PERMS) + && (fchmod(fd, mode) < 0)) { ret = errno; virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), path, mode); goto error; } + if ((hook) && ((ret = hook(fd, hookdata)) != 0)) { + goto error; + } if (close(fd) < 0) { ret = errno; virReportSystemError(errno, _("failed to close new file '%s'"), @@ -1259,13 +1263,13 @@ error: return ret; } -static int virDirCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid, +static int virDirCreateNoFork(const char *path, mode_t mode, uid_t uid, gid_t gid, unsigned int flags) { int ret = 0; struct stat st; if ((mkdir(path, mode) < 0) - && !((errno == EEXIST) && (flags & VIR_FILE_CREATE_ALLOW_EXIST))) + && !((errno == EEXIST) && (flags & VIR_DIR_CREATE_ALLOW_EXIST))) { ret = errno; virReportSystemError(errno, _("failed to create directory '%s'"), @@ -1285,7 +1289,8 @@ static int virDirCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gi path, uid, gid); goto error; } - if (chmod(path, mode) < 0) { + if ((flags & VIR_DIR_CREATE_FORCE_PERMS) + && (chmod(path, mode) < 0)) { ret = errno; virReportSystemError(errno, _("cannot set mode of '%s' to %04o"), @@ -1297,18 +1302,20 @@ error: } #ifndef WIN32 -int virFileCreate(const char *path, mode_t mode, - uid_t uid, gid_t gid, unsigned int flags) { +int virFileOperation(const char *path, int openflags, mode_t mode, + uid_t uid, gid_t gid, + virFileOperationHook hook, void *hookdata, + unsigned int flags) { struct stat st; pid_t pid; int waitret, status, ret = 0; int fd; - if ((!(flags & VIR_FILE_CREATE_AS_UID)) + if ((!(flags & VIR_FILE_OP_AS_UID)) || (getuid() != 0) - || ((uid == 0) && (gid == 0)) - || ((flags & VIR_FILE_CREATE_ALLOW_EXIST) && (stat(path, &st) >= 0))) { - return virFileCreateSimple(path, mode, uid, gid, flags); + || ((uid == 0) && (gid == 0))) { + return virFileOperationNoFork(path, openflags, mode, uid, gid, + hook, hookdata, flags); } /* parent is running as root, but caller requested that the @@ -1338,34 +1345,8 @@ int virFileCreate(const char *path, mode_t mode, if (!WIFEXITED(status) || (ret == EACCES)) { /* fall back to the simpler method, which works better in * some cases */ - return virFileCreateSimple(path, mode, uid, gid, flags); - } - if (ret != 0) { - goto parenterror; - } - - /* check if group was set properly by creating after - * setgid. If not, try doing it with chown */ - if (stat(path, &st) == -1) { - ret = errno; - virReportSystemError(errno, - _("stat of '%s' failed"), - path); - goto parenterror; - } - if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) { - ret = errno; - virReportSystemError(errno, - _("cannot chown '%s' to group %u"), - path, gid); - goto parenterror; - } - if (chmod(path, mode) < 0) { - ret = errno; - virReportSystemError(errno, - _("cannot set mode of '%s' to %04o"), - path, mode); - goto parenterror; + return virFileOperationNoFork(path, openflags, mode, uid, gid, + hook, hookdata, flags); } parenterror: return ret; @@ -1395,7 +1376,7 @@ parenterror: uid, path); goto childerror; } - if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) { + if ((fd = open(path, openflags, mode)) < 0) { ret = errno; if (ret != EACCES) { /* in case of EACCES, the parent will retry */ @@ -1405,6 +1386,29 @@ parenterror: } goto childerror; } + if (fstat(fd, &st) == -1) { + ret = errno; + virReportSystemError(errno, _("stat of '%s' failed"), path); + goto childerror; + } + if ((st.st_gid != gid) + && (fchown(fd, -1, gid) < 0)) { + ret = errno; + virReportSystemError(errno, _("cannot chown '%s' to (%u, %u)"), + path, uid, gid); + goto childerror; + } + if ((flags & VIR_FILE_OP_FORCE_PERMS) + && (fchmod(fd, mode) < 0)) { + ret = errno; + virReportSystemError(errno, + _("cannot set mode of '%s' to %04o"), + path, mode); + goto childerror; + } + if ((hook) && ((ret = hook(fd, hookdata)) != 0)) { + goto childerror; + } if (close(fd) < 0) { ret = errno; virReportSystemError(errno, _("child failed to close new file '%s'"), @@ -1423,11 +1427,11 @@ int virDirCreate(const char *path, mode_t mode, int waitret; int status, ret = 0; - if ((!(flags & VIR_FILE_CREATE_AS_UID)) + if ((!(flags & VIR_DIR_CREATE_AS_UID)) || (getuid() != 0) || ((uid == 0) && (gid == 0)) - || ((flags & VIR_FILE_CREATE_ALLOW_EXIST) && (stat(path, &st) >= 0))) { - return virDirCreateSimple(path, mode, uid, gid, flags); + || ((flags & VIR_DIR_CREATE_ALLOW_EXIST) && (stat(path, &st) >= 0))) { + return virDirCreateNoFork(path, mode, uid, gid, flags); } int forkRet = virFork(&pid); @@ -1451,34 +1455,11 @@ int virDirCreate(const char *path, mode_t mode, if (!WIFEXITED(status) || (ret == EACCES)) { /* fall back to the simpler method, which works better in * some cases */ - return virDirCreateSimple(path, mode, uid, gid, flags); + return virDirCreateNoFork(path, mode, uid, gid, flags); } if (ret != 0) { goto parenterror; } - - /* check if group was set properly by creating after - * setgid. If not, try doing it with chown */ - if (stat(path, &st) == -1) { - ret = errno; - virReportSystemError(errno, - _("stat of '%s' failed"), - path); - goto parenterror; - } - if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) { - ret = errno; - virReportSystemError(errno, - _("cannot chown '%s' to group %u"), - path, gid); - goto parenterror; - } - if (chmod(path, mode) < 0) { - virReportSystemError(errno, - _("cannot set mode of '%s' to %04o"), - path, mode); - goto parenterror; - } parenterror: return ret; } @@ -1513,20 +1494,45 @@ parenterror: } goto childerror; } + /* check if group was set properly by creating after + * setgid. If not, try doing it with chown */ + if (stat(path, &st) == -1) { + ret = errno; + virReportSystemError(errno, + _("stat of '%s' failed"), path); + goto childerror; + } + if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) { + ret = errno; + virReportSystemError(errno, + _("cannot chown '%s' to group %u"), + path, gid); + goto childerror; + } + if ((flags & VIR_DIR_CREATE_FORCE_PERMS) + && chmod(path, mode) < 0) { + virReportSystemError(errno, + _("cannot set mode of '%s' to %04o"), + path, mode); + goto childerror; + } childerror: _exit(ret); } #else /* WIN32 */ -int virFileCreate(const char *path, mode_t mode, - uid_t uid, gid_t gid, unsigned int flags) { - return virFileCreateSimple(path, mode, uid, gid, flags); +int virFileOperation(const char *path, int openflags, mode_t mode, + uid_t uid, gid_t gid, + virFileOperationHook hook, void *hookdata, + unsigned int flags) { + return virFileOperationNoFork(path, openflags, mode, uid, gid, + hook, hookdata, flags); } int virDirCreate(const char *path, mode_t mode, uid_t uid, gid_t gid, unsigned int flags) { - return virDirCreateSimple(path, mode, uid, gid, flags); + return virDirCreateNoFork(path, mode, uid, gid, flags); } #endif diff --git a/src/util/util.h b/src/util/util.h index 8460100..1bc9999 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -110,13 +110,22 @@ char *virFindFileInPath(const char *file); int virFileExists(const char *path); enum { - VIR_FILE_CREATE_NONE = 0, - VIR_FILE_CREATE_AS_UID = (1 << 0), - VIR_FILE_CREATE_ALLOW_EXIST = (1 << 1), + VIR_FILE_OP_NONE = 0, + VIR_FILE_OP_AS_UID = (1 << 0), + VIR_FILE_OP_FORCE_PERMS = (1 << 1), }; +typedef int (*virFileOperationHook)(int fd, void *data); +int virFileOperation(const char *path, int openflags, mode_t mode, + uid_t uid, gid_t gid, + virFileOperationHook hook, void *hookdata, + unsigned int flags) ATTRIBUTE_RETURN_CHECK; -int virFileCreate(const char *path, mode_t mode, uid_t uid, gid_t gid, - unsigned int flags) ATTRIBUTE_RETURN_CHECK; +enum { + VIR_DIR_CREATE_NONE = 0, + VIR_DIR_CREATE_AS_UID = (1 << 0), + VIR_DIR_CREATE_FORCE_PERMS = (1 << 1), + VIR_DIR_CREATE_ALLOW_EXIST = (1 << 2), +}; int virDirCreate(const char *path, mode_t mode, uid_t uid, gid_t gid, unsigned int flags) ATTRIBUTE_RETURN_CHECK; int virFileMakePath(const char *path) ATTRIBUTE_RETURN_CHECK; -- 1.6.6.1
There were a few operations on the storage volume file that were still being done as root, which will fail if the file is on a root-squashed NFS share. The result was that attempts to create a storage volume of type "raw" on a root-squashed NFS share would fail. This patch uses the newly introduced "hook" function in virFileOperation to execute all those file operations in the child process that's run under the uid that owns the file (and, presumably, has permission to write to the NFS share). --- src/storage/storage_backend.c | 113 +++++++++++++++++++--------------------- 1 files changed, 54 insertions(+), 59 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 07c116a..8b9ed5d 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -270,61 +270,33 @@ cleanup: return ret; } -int -virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, - virStoragePoolObjPtr pool, - virStorageVolDefPtr vol, - virStorageVolDefPtr inputvol, - unsigned int flags ATTRIBUTE_UNUSED) -{ - int fd = -1; - int ret = -1; - int createstat; - unsigned long long remain; - char *buf = NULL; - - if (vol->target.encryption != NULL) { - virStorageReportError(VIR_ERR_NO_SUPPORT, - "%s", _("storage pool does not support encrypted " - "volumes")); - return -1; - } - - if ((createstat = virFileOperation(vol->target.path, O_RDWR | O_CREAT | O_EXCL, - vol->target.perms.mode, - vol->target.perms.uid, vol->target.perms.gid, - NULL, NULL, - VIR_FILE_OP_FORCE_PERMS | - (pool->def->type == VIR_STORAGE_POOL_NETFS - ? VIR_FILE_OP_AS_UID : 0))) < 0) { - virReportSystemError(createstat, - _("cannot create path '%s'"), - vol->target.path); - goto cleanup; - } +struct createRawFileOpHookData { + virStorageVolDefPtr vol; + virStorageVolDefPtr inputvol; +}; - if ((fd = open(vol->target.path, O_RDWR | O_EXCL | O_DSYNC)) < 0) { - virReportSystemError(errno, - _("cannot open new path '%s'"), - vol->target.path); - goto cleanup; - } +static int createRawFileOpHook(int fd, void *data) { + struct createRawFileOpHookData *hdata = data; + int ret = 0; + unsigned long long remain; /* Seek to the final size, so the capacity is available upfront * for progress reporting */ - if (ftruncate(fd, vol->capacity) < 0) { + if (ftruncate(fd, hdata->vol->capacity) < 0) { + ret = errno; virReportSystemError(errno, _("cannot extend file '%s'"), - vol->target.path); + hdata->vol->target.path); goto cleanup; } - remain = vol->allocation; + remain = hdata->vol->allocation; - if (inputvol) { - int res = virStorageBackendCopyToFD(vol, inputvol, + if (hdata->inputvol) { + int res = virStorageBackendCopyToFD(hdata->vol, hdata->inputvol, fd, &remain, 1); if (res < 0) + ret = -res; goto cleanup; } @@ -341,11 +313,11 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, if (bytes > remain) bytes = remain; - if ((r = safezero(fd, 0, vol->allocation - remain, + if ((r = safezero(fd, 0, hdata->vol->allocation - remain, bytes)) != 0) { - virReportSystemError(r, - _("cannot fill file '%s'"), - vol->target.path); + ret = errno; + virReportSystemError(r, _("cannot fill file '%s'"), + hdata->vol->target.path); goto cleanup; } remain -= bytes; @@ -354,28 +326,51 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, int r; if ((r = safezero(fd, 0, 0, remain)) != 0) { - virReportSystemError(r, - _("cannot fill file '%s'"), - vol->target.path); + ret = errno; + virReportSystemError(r, _("cannot fill file '%s'"), + hdata->vol->target.path); goto cleanup; } } } - if (close(fd) < 0) { - virReportSystemError(errno, - _("cannot close file '%s'"), - vol->target.path); +cleanup: + return ret; +} + +int +virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol, + virStorageVolDefPtr inputvol, + unsigned int flags ATTRIBUTE_UNUSED) +{ + int ret = -1; + int createstat; + struct createRawFileOpHookData hdata = { vol, inputvol }; + + if (vol->target.encryption != NULL) { + virStorageReportError(VIR_ERR_NO_SUPPORT, + "%s", _("storage pool does not support encrypted " + "volumes")); goto cleanup; } - fd = -1; + + if ((createstat = virFileOperation(vol->target.path, O_RDWR | O_CREAT | O_EXCL, + vol->target.perms.mode, + vol->target.perms.uid, vol->target.perms.gid, + createRawFileOpHook, &hdata, + VIR_FILE_OP_FORCE_PERMS | + (pool->def->type == VIR_STORAGE_POOL_NETFS + ? VIR_FILE_OP_AS_UID : 0))) < 0) { + virReportSystemError(createstat, + _("cannot create path '%s'"), + vol->target.path); + goto cleanup; + } ret = 0; cleanup: - if (fd != -1) - close(fd); - VIR_FREE(buf); - return ret; } -- 1.6.6.1
On Thu, Feb 18, 2010 at 01:25:43PM -0500, Laine Stump wrote:
(This patchset requires that the virFork() patchset I send a couple hours ago be applied first, otherwise it will fail to apply).
virFileCreate just didn't have what it takes to get the job done. It turns out that there are other things that must be done to files as the qemu (or whatever) user, so this patchset adds a hook function that gets called during the child process. That solves a problem I found with creating raw storage volumes on root-squash NFS, and will also be used for an upcoming domain-save-on-root-squash-nfs patch.
I'm now beginning to think that virFileCreate/virFileOperation is just too narrow in scope, and it's getting too many options. Possibly it will be better to just make a simpler virCallasUID() function that gets a pointer to a function to execute in the child process as an argument and does nothing but fork/setuid/call the function. That function would then contain *all* of the file operations, including creating/opening/closing the file. But that's too large of a change to contemplate so soon before a release, and this functionality is necessary for two important bug fixes (mentioned above), so...
Okay, I agree that the functions are getting a bit too complex and some cleanup will be in order, but for the sake of the release I pushed the 2 patches which looks good to me, with the expectation that we can cleanup this next month, thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (2)
-
Daniel Veillard -
Laine Stump