On Wed, Jan 20, 2010 at 02:29:40AM -0500, Laine Stump wrote:

Similar to virExecWithHook, but waits for child to exit. Useful for doing things like setuid after the fork but before the exec. --- src/util/util.c | 25 ++++++++++++++++++------- src/util/util.h | 3 +++ 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/src/util/util.c b/src/util/util.c index 67fae00..578d12b 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -804,9 +804,11 @@ error: * only if the command could not be run. */ int -virRun(virConnectPtr conn, - const char *const*argv, - int *status) { +virRunWithHook(virConnectPtr conn, + const char *const*argv, + virExecHook hook, + void *data, + int *status) { pid_t childpid; int exitstatus, execret, waitret; int ret = -1; @@ -823,7 +825,7 @@ virRun(virConnectPtr conn,

if ((execret = __virExec(conn, argv, NULL, NULL, &childpid, -1, &outfd, &errfd, - VIR_EXEC_NONE, NULL, NULL, NULL)) < 0) { + VIR_EXEC_NONE, hook, data, NULL)) < 0) { ret = execret; goto error; } @@ -879,9 +881,11 @@ virRun(virConnectPtr conn, #else /* __MINGW32__ */

int -virRun(virConnectPtr conn, - const char *const *argv ATTRIBUTE_UNUSED, - int *status) +virRunWithHook(virConnectPtr conn, + const char *const *argv ATTRIBUTE_UNUSED, + virExecHook hook ATTRIBUTE_UNUSED, + void *data ATTRIBUTE_UNUSED, + int *status) { if (status) *status = ENOTSUP; @@ -907,6 +911,13 @@ virExec(virConnectPtr conn,

#endif /* __MINGW32__ */

+int +virRun(virConnectPtr conn, + const char *const*argv, + int *status) { + return virRunWithHook(conn, argv, NULL, NULL, status); +} + /* Like gnulib's fread_file, but read no more than the specified maximum number of bytes. If the length of the input is <= max_len, and upon error while reading that data, it works just like fread_file. */ diff --git a/src/util/util.h b/src/util/util.h index d556daa..5e70038 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -81,6 +81,9 @@ int virExec(virConnectPtr conn, int *errfd, int flags) ATTRIBUTE_RETURN_CHECK; int virRun(virConnectPtr conn, const char *const*argv, int *status) ATTRIBUTE_RETURN_CHECK; +int virRunWithHook(virConnectPtr conn, const char *const*argv, + virExecHook hook, void *data, + int *status) ATTRIBUTE_RETURN_CHECK;

int virFileReadLimFD(int fd, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK;

ACK, looks fine to me, 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/

On Wed, Jan 20, 2010 at 02:29:41AM -0500, Laine Stump wrote:

These functions create a new file or directory with the given uid/gid. If the flag VIR_FILE_CREATE_AS_UID is given, they do this by forking a new process, calling setuid/setgid in the new process, and then creating the file. This is better than simply calling open then fchown, because in the latter case, a root-squashing nfs server would create the new file as user nobody, then refuse to allow fchown.

If VIR_FILE_CREATE_AS_UID is not specified, the simpler tactic of creating the file/dir, then chowning is is used. This gives better results in cases where the parent directory isn't on a root-squashing NFS server, but doesn't give permission for the specified uid/gid to create files. (Note that if the fork/setuid method fails to create the file due to access privileges, the parent process will make a second attempt using this simpler method.)

If the bit VIR_FILE_CREATE_ALLOW_EXIST is set in the flags, an existing file/directory will not cause an error; in this case, the function will simply set the permissions of the file/directory to those requested. If VIR_FILE_CREATE_ALLOW_EXIST is not specified, an existing file/directory is considered (and reported as) an error.

Return from both of these functions is 0 on success, or the value of errno if there was a failure. --- src/util/util.c | 304 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 10 ++ 2 files changed, 314 insertions(+), 0 deletions(-)

diff --git a/src/util/util.c b/src/util/util.c index 578d12b..d0f7fcd 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1138,6 +1138,310 @@ int virFileExists(const char *path) return(0); }

+ +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); + int fd = -1; + int ret = 0; + struct stat st; + + if ((fd = open(path, open_flags, mode)) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("failed to create file '%s'"), + path); + goto error; + } + if (fstat(fd, &st) == -1) { + ret = errno; + virReportSystemError(NULL, errno, _("stat of '%s' failed"), path);

misses a close(fd); here

+ goto error; + } + if (((st.st_uid != uid) || (st.st_gid != gid)) + && (fchown(fd, uid, gid) < 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot chown '%s' to (%u, %u)"), + path, uid, gid); + close(fd); + goto error; + } + if (fchmod(fd, mode) < 0) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot set mode of '%s' to %04o"), + path, mode); + close(fd); + goto error; + } + if (close(fd) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("failed to close new file '%s'"), + path); + goto error; + } + fd = -1; +error: + return ret; +} [...] +#ifndef WIN32 +int virFileCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + struct stat st; + pid_t pid; + int waitret, status, ret = 0; + int fd = -1; + + if ((!(flags & VIR_FILE_CREATE_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); + } + + /* parent is running as root, but caller requested that the + * file be created as some other user and/or group). The + * following dance avoids problems caused by root-squashing + * NFS servers. */ + + if ((pid = fork()) < 0) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot fork o create file '%s'"), path); + return ret; + } + + if (pid) { /* parent */ + /* wait for child to complete, and retrieve its exit code */ + while ((waitret = waitpid(pid, &status, 0) == -1) + && (errno == EINTR)); + if (waitret == -1) { + ret = errno; + virReportSystemError(NULL, errno, + _("failed to wait for child creating '%s'"), + path); + goto parenterror; + } + ret = WEXITSTATUS(status); + 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(NULL, errno, + _("stat of '%s' failed"), + path); + goto parenterror; + } + if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot chown '%s' to group %u"), + path, gid); + goto parenterror; + } + if (chmod(path, mode) < 0) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot set mode of '%s' to %04o"), + path, mode); + goto parenterror; + } +parenterror: + return ret; + } + + /* child - set desired uid/gid, then attempt to create the file */ + + if ((gid != 0) && (setgid(gid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot set gid %u creating '%s'"), + gid, path); + goto childerror; + } + if ((uid != 0) && (setuid(uid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot set uid %u creating '%s'"), + uid, path); + goto childerror; + } + if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) { + ret = errno; + if (ret != EACCES) { + /* in case of EACCES, the parent will retry */ + virReportSystemError(NULL, errno, + _("child failed to create file '%s'"), + path); + } + goto childerror; + } + if (close(fd) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("child failed to close new file '%s'"), + path); + goto childerror; + } +childerror: + _exit(ret); + +} + +int virDirCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + struct stat st; + pid_t pid; + int waitret; + int status, ret = 0; + + if ((!(flags & VIR_FILE_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); + } + + if ((pid = fork()) < 0) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot fork to create directory '%s'"), + path); + return ret; + } + + if (pid) { /* parent */ + /* wait for child to complete, and retrieve its exit code */ + while ((waitret = waitpid(pid, &status, 0) == -1) && (errno == EINTR)); + if (waitret == -1) { + ret = errno; + virReportSystemError(NULL, errno, + _("failed to wait for child creating '%s'"), + path); + goto parenterror; + } + ret = WEXITSTATUS(status); + if (!WIFEXITED(status) || (ret == EACCES)) { + /* fall back to the simpler method, which works better in + * some cases */ + return virDirCreateSimple(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(NULL, errno, + _("stat of '%s' failed"), + path); + goto parenterror; + } + if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot chown '%s' to group %u"), + path, gid); + goto parenterror; + } + if (chmod(path, mode) < 0) { + virReportSystemError(NULL, errno, + _("cannot set mode of '%s' to %04o"), + path, mode); + goto parenterror; + } +parenterror: + return ret; + } + + /* child - set desired uid/gid, then attempt to create the directory */ + + if ((gid != 0) && (setgid(gid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot set gid %u creating '%s'"), + gid, path); + goto childerror; + } + if ((uid != 0) && (setuid(uid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot set uid %u creating '%s'"), + uid, path); + goto childerror; + } + if (mkdir(path, mode) < 0) { + ret = errno; + if (ret != EACCES) { + /* in case of EACCES, the parent will retry */ + virReportSystemError(NULL, errno, _("child failed to create directory '%s'"), + path); + } + goto childerror; + } +childerror: + _exit(ret); +}

This is fairly tortuous, but apparently we have no choice ...

+#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 virDirCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + return virDirCreateSimple(path, mode, uid, gid, flags); +} +#endif + int virFileMakePath(const char *path) { struct stat st; diff --git a/src/util/util.h b/src/util/util.h index 5e70038..b0219cc 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -111,6 +111,16 @@ 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), +}; + +int virFileCreate(const char *path, mode_t mode, uid_t uid, gid_t gid, + unsigned int flags) ATTRIBUTE_RETURN_CHECK; +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;

int virFileBuildPath(const char *dir,

ACK, but there is the small fd leak to plug 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/

These functions create a new file or directory with the given uid/gid. If the flag VIR_FILE_CREATE_AS_UID is given, they do this by forking a new process, calling setuid/setgid in the new process, and then creating the file. This is better than simply calling open then fchown, because in the latter case, a root-squashing nfs server would create the new file as user nobody, then refuse to allow fchown. If VIR_FILE_CREATE_AS_UID is not specified, the simpler tactic of creating the file/dir, then chowning is is used. This gives better results in cases where the parent directory isn't on a root-squashing NFS server, but doesn't give permission for the specified uid/gid to create files. (Note that if the fork/setuid method fails to create the file due to access privileges, the parent process will make a second attempt using this simpler method.) If the bit VIR_FILE_CREATE_ALLOW_EXIST is set in the flags, an existing file/directory will not cause an error; in this case, the function will simply set the permissions of the file/directory to those requested. If VIR_FILE_CREATE_ALLOW_EXIST is not specified, an existing file/directory is considered (and reported as) an error. Return from both of these functions is 0 on success, or the value of errno if there was a failure. --- src/util/util.c | 305 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ src/util/util.h | 10 ++ 2 files changed, 315 insertions(+), 0 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 578d12b..08d74a3 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -1138,6 +1138,311 @@ int virFileExists(const char *path) return(0); } + +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); + int fd = -1; + int ret = 0; + struct stat st; + + if ((fd = open(path, open_flags, mode)) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("failed to create file '%s'"), + path); + goto error; + } + if (fstat(fd, &st) == -1) { + ret = errno; + virReportSystemError(NULL, errno, _("stat of '%s' failed"), path); + goto error; + } + if (((st.st_uid != uid) || (st.st_gid != gid)) + && (fchown(fd, uid, gid) < 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot chown '%s' to (%u, %u)"), + path, uid, gid); + goto error; + } + if (fchmod(fd, mode) < 0) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot set mode of '%s' to %04o"), + path, mode); + goto error; + } + if (close(fd) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("failed to close new file '%s'"), + path); + fd = -1; + goto error; + } + fd = -1; +error: + if (fd != -1) + close(fd); + return ret; +} + +static int virDirCreateSimple(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))) + { + ret = errno; + virReportSystemError(NULL, errno, _("failed to create directory '%s'"), + path); + goto error; + } + + if (stat(path, &st) == -1) { + ret = errno; + virReportSystemError(NULL, errno, _("stat of '%s' failed"), path); + goto error; + } + if (((st.st_uid != uid) || (st.st_gid != gid)) + && (chown(path, uid, gid) < 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot chown '%s' to (%u, %u)"), + path, uid, gid); + goto error; + } + if (chmod(path, mode) < 0) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot set mode of '%s' to %04o"), + path, mode); + goto error; + } +error: + return ret; +} + +#ifndef WIN32 +int virFileCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + struct stat st; + pid_t pid; + int waitret, status, ret = 0; + int fd; + + if ((!(flags & VIR_FILE_CREATE_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); + } + + /* parent is running as root, but caller requested that the + * file be created as some other user and/or group). The + * following dance avoids problems caused by root-squashing + * NFS servers. */ + + if ((pid = fork()) < 0) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot fork o create file '%s'"), path); + return ret; + } + + if (pid) { /* parent */ + /* wait for child to complete, and retrieve its exit code */ + while ((waitret = waitpid(pid, &status, 0) == -1) + && (errno == EINTR)); + if (waitret == -1) { + ret = errno; + virReportSystemError(NULL, errno, + _("failed to wait for child creating '%s'"), + path); + goto parenterror; + } + ret = WEXITSTATUS(status); + 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(NULL, errno, + _("stat of '%s' failed"), + path); + goto parenterror; + } + if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot chown '%s' to group %u"), + path, gid); + goto parenterror; + } + if (chmod(path, mode) < 0) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot set mode of '%s' to %04o"), + path, mode); + goto parenterror; + } +parenterror: + return ret; + } + + /* child - set desired uid/gid, then attempt to create the file */ + + if ((gid != 0) && (setgid(gid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot set gid %u creating '%s'"), + gid, path); + goto childerror; + } + if ((uid != 0) && (setuid(uid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot set uid %u creating '%s'"), + uid, path); + goto childerror; + } + if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) { + ret = errno; + if (ret != EACCES) { + /* in case of EACCES, the parent will retry */ + virReportSystemError(NULL, errno, + _("child failed to create file '%s'"), + path); + } + goto childerror; + } + if (close(fd) < 0) { + ret = errno; + virReportSystemError(NULL, errno, _("child failed to close new file '%s'"), + path); + goto childerror; + } +childerror: + _exit(ret); + +} + +int virDirCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + struct stat st; + pid_t pid; + int waitret; + int status, ret = 0; + + if ((!(flags & VIR_FILE_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); + } + + if ((pid = fork()) < 0) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot fork to create directory '%s'"), + path); + return ret; + } + + if (pid) { /* parent */ + /* wait for child to complete, and retrieve its exit code */ + while ((waitret = waitpid(pid, &status, 0) == -1) && (errno == EINTR)); + if (waitret == -1) { + ret = errno; + virReportSystemError(NULL, errno, + _("failed to wait for child creating '%s'"), + path); + goto parenterror; + } + ret = WEXITSTATUS(status); + if (!WIFEXITED(status) || (ret == EACCES)) { + /* fall back to the simpler method, which works better in + * some cases */ + return virDirCreateSimple(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(NULL, errno, + _("stat of '%s' failed"), + path); + goto parenterror; + } + if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) { + ret = errno; + virReportSystemError(NULL, errno, + _("cannot chown '%s' to group %u"), + path, gid); + goto parenterror; + } + if (chmod(path, mode) < 0) { + virReportSystemError(NULL, errno, + _("cannot set mode of '%s' to %04o"), + path, mode); + goto parenterror; + } +parenterror: + return ret; + } + + /* child - set desired uid/gid, then attempt to create the directory */ + + if ((gid != 0) && (setgid(gid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot set gid %u creating '%s'"), + gid, path); + goto childerror; + } + if ((uid != 0) && (setuid(uid) != 0)) { + ret = errno; + virReportSystemError(NULL, errno, _("cannot set uid %u creating '%s'"), + uid, path); + goto childerror; + } + if (mkdir(path, mode) < 0) { + ret = errno; + if (ret != EACCES) { + /* in case of EACCES, the parent will retry */ + virReportSystemError(NULL, errno, _("child failed to create directory '%s'"), + path); + } + 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 virDirCreate(const char *path, mode_t mode, + uid_t uid, gid_t gid, unsigned int flags) { + return virDirCreateSimple(path, mode, uid, gid, flags); +} +#endif + int virFileMakePath(const char *path) { struct stat st; diff --git a/src/util/util.h b/src/util/util.h index 5e70038..b0219cc 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -111,6 +111,16 @@ 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), +}; + +int virFileCreate(const char *path, mode_t mode, uid_t uid, gid_t gid, + unsigned int flags) ATTRIBUTE_RETURN_CHECK; +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; int virFileBuildPath(const char *dir, -- 1.6.6

On Wed, Jan 20, 2010 at 02:29:42AM -0500, Laine Stump wrote:

In order to avoid problems trying to chown files that were created by root on a root-squashing nfs server, fork a new process that setuid's to the desired uid before creating the file. (It's only done this way if the pool containing the new volume is of type 'netfs', otherwise the old method of creating the file followed by chown() is used.)

This changes the semantics of the "create_func" slightly - previously it was assumed that this function just created the file, then the caller would chown it to the desired uid. Now, create_func does both operations.

There are multiple functions that can take on the role of create_func:

createFileDir - previously called mkdir(), now calls virDirCreate(). virStorageBackendCreateRaw - previously called open(), now calls virFileCreate(). virStorageBackendCreateQemuImg - use virRunWithHook() to setuid/gid. virStorageBackendCreateQcowCreate - same. virStorageBackendCreateBlockFrom - preserve old behavior (but attempt chown when necessary even if not root) --- src/storage/storage_backend.c | 136 +++++++++++++++++++++++++++++---- src/storage/storage_backend.h | 8 ++- src/storage/storage_backend_disk.c | 3 +- src/storage/storage_backend_fs.c | 54 ++++---------- src/storage/storage_backend_logical.c | 3 +- src/storage/storage_driver.c | 4 +- 6 files changed, 147 insertions(+), 61 deletions(-)

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 9dc801c..bc656f2 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -205,6 +205,7 @@ cleanup:

static int virStorageBackendCreateBlockFrom(virConnectPtr conn, + virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags ATTRIBUTE_UNUSED) @@ -212,6 +213,9 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn, int fd = -1; int ret = -1; unsigned long long remain; + struct stat st; + gid_t gid; + uid_t uid;

if ((fd = open(vol->target.path, O_RDWR)) < 0) { virReportSystemError(conn, errno, @@ -229,6 +233,28 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn, goto cleanup; }

+ if (fstat(fd, &st) == -1) { + ret = errno; + virReportSystemError(conn, errno, _("stat of '%s' failed"), + vol->target.path); + goto cleanup; + } + uid = (vol->target.perms.uid != st.st_uid) ? vol->target.perms.uid : -1; + gid = (vol->target.perms.gid != st.st_gid) ? vol->target.perms.gid : -1; + if (((uid != -1) || (gid != -1)) + && (fchown(fd, vol->target.perms.uid, vol->target.perms.gid) < 0)) { + virReportSystemError(conn, errno, + _("cannot chown '%s' to (%u, %u)"), + vol->target.path, vol->target.perms.uid, + vol->target.perms.gid); + goto cleanup; + } + if (fchmod(fd, vol->target.perms.mode) < 0) { + virReportSystemError(conn, errno, + _("cannot set mode of '%s' to %04o"), + vol->target.path, vol->target.perms.mode); + goto cleanup; + } if (close(fd) < 0) { virReportSystemError(conn, errno, _("cannot close file '%s'"), @@ -247,12 +273,14 @@ cleanup:

int virStorageBackendCreateRaw(virConnectPtr conn, + 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;

@@ -263,14 +291,23 @@ virStorageBackendCreateRaw(virConnectPtr conn, return -1; }

- if ((fd = open(vol->target.path, O_RDWR | O_CREAT | O_EXCL, - vol->target.perms.mode)) < 0) { - virReportSystemError(conn, errno, + 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) { + virReportSystemError(conn, createstat, _("cannot create path '%s'"), vol->target.path); goto cleanup; }

+ if ((fd = open(vol->target.path, O_RDWR | O_EXCL)) < 0) { + virReportSystemError(conn, errno, + _("cannot open new path '%s'"), + vol->target.path); + goto cleanup; + } + /* Seek to the final size, so the capacity is available upfront * for progress reporting */ if (ftruncate(fd, vol->capacity) < 0) { @@ -453,12 +490,87 @@ cleanup: return ret; }

+static int virStorageBuildSetUIDHook(void *data) { + virStorageVolDefPtr vol = data; + + if ((vol->target.perms.gid != 0) + && (setgid(vol->target.perms.gid) != 0)) { + virReportSystemError(NULL, errno, + _("Cannot set gid to %u before creating %s"), + vol->target.perms.gid, vol->target.path); + return -1; + } + if ((vol->target.perms.uid != 0) + && (setuid(vol->target.perms.uid) != 0)) { + virReportSystemError(NULL, errno, + _("Cannot set uid to %u before creating %s"), + vol->target.perms.uid, vol->target.path); + return -1; + } + return 0; +} + +static int virStorageBackendCreateExecCommand(virConnectPtr conn, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol, + const char **cmdargv) { + struct stat st; + gid_t gid; + uid_t uid; + int filecreated = 0; + + if ((pool->def->type == VIR_STORAGE_POOL_NETFS) + && (getuid() == 0) + && ((vol->target.perms.uid != 0) || (vol->target.perms.gid != 0))) { + if (virRunWithHook(conn, cmdargv, + virStorageBuildSetUIDHook, vol, NULL) == 0) { + /* command was successfully run, check if the file was created */ + if (stat(vol->target.path, &st) >=0) + filecreated = 1; + } + } + if (!filecreated) { + if (virRun(conn, cmdargv, NULL) < 0) { + virReportSystemError(conn, errno, + _("Cannot run %s to create %s"), + cmdargv[0], vol->target.path); + return -1; + } + if (stat(vol->target.path, &st) < 0) { + virReportSystemError(conn, errno, + _("%s failed to create %s"), + cmdargv[0], vol->target.path); + return -1; + } + } + + uid = (vol->target.perms.uid != st.st_uid) ? vol->target.perms.uid : -1; + gid = (vol->target.perms.gid != st.st_gid) ? vol->target.perms.gid : -1; + if (((uid != -1) || (gid != -1)) + && (chown(vol->target.path, uid, gid) < 0)) { + virReportSystemError(conn, errno, + _("cannot chown %s to (%u, %u)"), + vol->target.path, vol->target.perms.uid, + vol->target.perms.gid); + return -1; + } + if (chmod(vol->target.path, vol->target.perms.mode) < 0) { + virReportSystemError(conn, errno, + _("cannot set mode of '%s' to %04o"), + vol->target.path, vol->target.perms.mode); + return -1; + } + return 0; +} + static int virStorageBackendCreateQemuImg(virConnectPtr conn, + virStoragePoolObjPtr pool, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags ATTRIBUTE_UNUSED) { + int ret; char size[100]; char *create_tool; short use_kvmimg; @@ -616,14 +728,10 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, /* Size in KB */ snprintf(size, sizeof(size), "%lluK", vol->capacity/1024);

- if (virRun(conn, imgargv, NULL) < 0) { - VIR_FREE(imgargv[0]); - return -1; - } - + ret = virStorageBackendCreateExecCommand(conn, pool, vol, imgargv); VIR_FREE(imgargv[0]);

- return 0; + return ret; }

/* @@ -632,10 +740,12 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, */ static int virStorageBackendCreateQcowCreate(virConnectPtr conn, + virStoragePoolObjPtr pool, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags ATTRIBUTE_UNUSED) { + int ret; char size[100]; const char *imgargv[4];

@@ -672,14 +782,10 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn, imgargv[2] = vol->target.path; imgargv[3] = NULL;

- if (virRun(conn, imgargv, NULL) < 0) { - VIR_FREE(imgargv[0]); - return -1; - } - + ret = virStorageBackendCreateExecCommand(conn, pool, vol, imgargv); VIR_FREE(imgargv[0]);

- return 0; + return ret; }

virStorageBackendBuildVolFrom diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h index 1c38eeb..988539c 100644 --- a/src/storage/storage_backend.h +++ b/src/storage/storage_backend.h @@ -35,14 +35,18 @@ typedef int (*virStorageBackendRefreshPool)(virConnectPtr conn, virStoragePoolOb typedef int (*virStorageBackendStopPool)(virConnectPtr conn, virStoragePoolObjPtr pool); typedef int (*virStorageBackendDeletePool)(virConnectPtr conn, virStoragePoolObjPtr pool, unsigned int flags);

-typedef int (*virStorageBackendBuildVol)(virConnectPtr conn, virStorageVolDefPtr vol); +typedef int (*virStorageBackendBuildVol)(virConnectPtr conn, + virStoragePoolObjPtr pool, virStorageVolDefPtr vol); typedef int (*virStorageBackendCreateVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol); typedef int (*virStorageBackendRefreshVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol); typedef int (*virStorageBackendDeleteVol)(virConnectPtr conn, virStoragePoolObjPtr pool, virStorageVolDefPtr vol, unsigned int flags); -typedef int (*virStorageBackendBuildVolFrom)(virConnectPtr conn, virStorageVolDefPtr origvol, virStorageVolDefPtr newvol, unsigned int flags); +typedef int (*virStorageBackendBuildVolFrom)(virConnectPtr conn, virStoragePoolObjPtr pool, + virStorageVolDefPtr origvol, virStorageVolDefPtr newvol, + unsigned int flags);

/* File creation/cloning functions used for cloning between backends */ int virStorageBackendCreateRaw(virConnectPtr conn, + virStoragePoolObjPtr pool, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags); diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c index 72ccd81..5ecf210 100644 --- a/src/storage/storage_backend_disk.c +++ b/src/storage/storage_backend_disk.c @@ -599,6 +599,7 @@ virStorageBackendDiskCreateVol(virConnectPtr conn,

static int virStorageBackendDiskBuildVolFrom(virConnectPtr conn, + virStoragePoolObjPtr pool, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags) @@ -609,7 +610,7 @@ virStorageBackendDiskBuildVolFrom(virConnectPtr conn, if (!build_func) return -1;

- return build_func(conn, vol, inputvol, flags); + return build_func(conn, pool, vol, inputvol, flags); }

static int diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index b03e4e9..cc26f2f 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -737,9 +737,12 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn, }

static int createFileDir(virConnectPtr conn, + virStoragePoolObjPtr pool, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags ATTRIBUTE_UNUSED) { + int err; + if (inputvol) { virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR, "%s", @@ -747,9 +750,11 @@ static int createFileDir(virConnectPtr conn, return -1; }

- if (mkdir(vol->target.path, vol->target.perms.mode) < 0) { - virReportSystemError(conn, errno, - _("cannot create path '%s'"), + if ((err = virDirCreate(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) { + virReportSystemError(conn, err, _("cannot create path '%s'"), vol->target.path); return -1; } @@ -759,10 +764,10 @@ static int createFileDir(virConnectPtr conn,

static int _virStorageBackendFileSystemVolBuild(virConnectPtr conn, + virStoragePoolObjPtr pool, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol) { - int fd; virStorageBackendBuildVolFrom create_func; int tool_type;

@@ -794,41 +799,8 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn, return -1; }

- if (create_func(conn, vol, inputvol, 0) < 0) - return -1; - - if ((fd = open(vol->target.path, O_RDONLY)) < 0) { - virReportSystemError(conn, errno, - _("cannot read path '%s'"), - vol->target.path); + if (create_func(conn, pool, vol, inputvol, 0) < 0) return -1; - } - - /* We can only chown/grp if root */ - if (getuid() == 0) { - if (fchown(fd, vol->target.perms.uid, vol->target.perms.gid) < 0) { - virReportSystemError(conn, errno, - _("cannot set file owner '%s'"), - vol->target.path); - close(fd); - return -1; - } - } - if (fchmod(fd, vol->target.perms.mode) < 0) { - virReportSystemError(conn, errno, - _("cannot set file mode '%s'"), - vol->target.path); - close(fd); - return -1; - } - - if (close(fd) < 0) { - virReportSystemError(conn, errno, - _("cannot close file '%s'"), - vol->target.path); - return -1; - } - return 0; }

@@ -839,8 +811,9 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn, */ static int virStorageBackendFileSystemVolBuild(virConnectPtr conn, + virStoragePoolObjPtr pool, virStorageVolDefPtr vol) { - return _virStorageBackendFileSystemVolBuild(conn, vol, NULL); + return _virStorageBackendFileSystemVolBuild(conn, pool, vol, NULL); }

/* @@ -848,10 +821,11 @@ virStorageBackendFileSystemVolBuild(virConnectPtr conn, */ static int virStorageBackendFileSystemVolBuildFrom(virConnectPtr conn, + virStoragePoolObjPtr pool, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags ATTRIBUTE_UNUSED) { - return _virStorageBackendFileSystemVolBuild(conn, vol, inputvol); + return _virStorageBackendFileSystemVolBuild(conn, pool, vol, inputvol); }

/** diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 5eebfac..c2e74a5 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -660,6 +660,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn,

static int virStorageBackendLogicalBuildVolFrom(virConnectPtr conn, + virStoragePoolObjPtr pool, virStorageVolDefPtr vol, virStorageVolDefPtr inputvol, unsigned int flags) @@ -670,7 +671,7 @@ virStorageBackendLogicalBuildVolFrom(virConnectPtr conn, if (!build_func) return -1;

- return build_func(conn, vol, inputvol, flags); + return build_func(conn, pool, vol, inputvol, flags); }

static int diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index c2f7850..50fcbe2 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -1331,7 +1331,7 @@ storageVolumeCreateXML(virStoragePoolPtr obj, voldef->building = 1; virStoragePoolObjUnlock(pool);

- buildret = backend->buildVol(obj->conn, buildvoldef); + buildret = backend->buildVol(obj->conn, pool, buildvoldef);

storageDriverLock(driver); virStoragePoolObjLock(pool); @@ -1484,7 +1484,7 @@ storageVolumeCreateXMLFrom(virStoragePoolPtr obj, virStoragePoolObjUnlock(origpool); }

- buildret = backend->buildVolFrom(obj->conn, newvol, origvol, flags); + buildret = backend->buildVolFrom(obj->conn, pool, newvol, origvol, flags);

storageDriverLock(driver); virStoragePoolObjLock(pool);

It may have been simpler to pass a flag for the NETFS pool type instead of adding the pool argument to the full hierarchy of functions, but that doesn't change things that much, and might be useful in other cases not yet reached, ACK 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/

On Wed, Jan 20, 2010 at 02:29:43AM -0500, Laine Stump wrote:

Previously the uid/gid/mode in the xml was ignored when creating new storage pool directories. This commit attempts to honor the requested permissions, and spits out an error if it can't.

Note that when creating the directory, the rest of the path leading up to the final element is created using current uid/gid/mode, and the final element gets the settings from xml. It is NOT an error for the directory to already exist; in this case, the perms for the existing directory are just set (if necessary). --- src/storage/storage_backend_fs.c | 41 +++++++++++++++++++++++++++++++++++-- 1 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index cc26f2f..481e46e 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -506,9 +506,44 @@ virStorageBackendFileSystemBuild(virConnectPtr conn, unsigned int flags ATTRIBUTE_UNUSED) { int err; - if ((err = virFileMakePath(pool->def->target.path)) < 0) { - virReportSystemError(conn, err, - _("cannot create path '%s'"), + char path[PATH_MAX];

Urgh, I though we we trying to avoid allocating a full page like this for an argument on the stack...

+ char *p; + + if (virStrcpyStatic(path, pool->def->target.path) == NULL) { + virStorageReportError(conn, VIR_ERR_INVALID_ARG, + _("cannot make copy of path '%s'"), + pool->def->target.path); + return -1; + } + if (!(p = strrchr(path, '/'))) { + virStorageReportError(conn, VIR_ERR_INVALID_ARG, + _("path '%s' is not absolute"), + pool->def->target.path); + return -1; + } + + if (p != path) { + /* assure all directories in the path prior to the final dir + * exist, with default uid/gid/mode. */ + *p = '\0'; + if ((err = virFileMakePath(path)) != 0) { + virReportSystemError(conn, err, _("cannot create path '%s'"), + path); + return -1; + } + }

and considering the handling done with path, I think a simple making patch a char * and initializing it with just a simple strdup() should be just fine, all we are doing is truncating the path. But it also need to be freed.

+ /* 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 ((err = virDirCreate(pool->def->target.path, + pool->def->target.perms.mode, + pool->def->target.perms.uid, + pool->def->target.perms.gid, + VIR_FILE_CREATE_ALLOW_EXIST | + (pool->def->type == VIR_STORAGE_POOL_NETFS + ? VIR_FILE_CREATE_AS_UID : 0)) != 0)) { + virReportSystemError(conn, err, _("cannot create path '%s'"), pool->def->target.path); return -1; }

It's probably better to get rid of char path[PATH_MAX] before commiting this, 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/

Previously the uid/gid/mode in the xml was ignored when creating new storage pool directories. This commit attempts to honor the requested permissions, and spits out an error if it can't. Note that when creating the directory, the rest of the path leading up to the final element is created using current uid/gid/mode, and the final element gets the settings from xml. It is NOT an error for the directory to already exist; in this case, the perms for the existing directory are just set (if necessary). --- src/storage/storage_backend_fs.c | 49 ++++++++++++++++++++++++++++++++----- 1 files changed, 42 insertions(+), 7 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index cc26f2f..0d1c7a7 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -505,15 +505,50 @@ virStorageBackendFileSystemBuild(virConnectPtr conn, virStoragePoolObjPtr pool, unsigned int flags ATTRIBUTE_UNUSED) { - int err; - if ((err = virFileMakePath(pool->def->target.path)) < 0) { - virReportSystemError(conn, err, - _("cannot create path '%s'"), - pool->def->target.path); - return -1; + int err, ret = -1; + char *parent; + char *p; + + if ((parent = strdup(pool->def->target.path)) == NULL) { + virReportOOMError(conn); + goto error; + } + if (!(p = strrchr(parent, '/'))) { + virStorageReportError(conn, 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 ((err = virFileMakePath(parent)) != 0) { + virReportSystemError(conn, err, _("cannot create path '%s'"), + parent); + goto error; + } } - return 0; + /* 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 ((err = virDirCreate(pool->def->target.path, + pool->def->target.perms.mode, + pool->def->target.perms.uid, + pool->def->target.perms.gid, + VIR_FILE_CREATE_ALLOW_EXIST | + (pool->def->type == VIR_STORAGE_POOL_NETFS + ? VIR_FILE_CREATE_AS_UID : 0)) != 0)) { + virReportSystemError(conn, err, _("cannot create path '%s'"), + pool->def->target.path); + goto error; + } + ret = 0; +error: + VIR_FREE(parent); + return ret; } -- 1.6.6