12:17 a.m.
I'm planning to use these to fix the problems with creating storage
volumes on root-squashing NFS servers, but since they may be of
general utility I thought I'd throw them out for comments beforehand -
it something different would make them more generally useful, better
to change it now than after I've already finished code to use them.
My plan is to use virRunWithHook() to call setuid (or possibly some
sort of capng calisthenics) prior to execing qemu-img and
qcow-create. virFileCreate and virDirCreate will be used when creating
raw volumes.
I have written test code to verify that virFileCreate works in several
different scenarios. Haven't done so with virRun yet, but it's a
fairly simple change, just exposing __virExec arguments that were
already there. (I'd previously written test code (yes, it worked ;-))
that used a virRun with uid/gid args added on, but have decided
against that approach, since this seems more flexible)
12:17 a.m.
New subject: [libvirt] [PATCH 1/2] Add virRunWithHook.
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 44a4b2f..1d493de 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -792,9 +792,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;
@@ -811,7 +813,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;
}
@@ -867,9 +869,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;
@@ -895,6 +899,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;
--
1.6.6
12:17 a.m.
New subject: [libvirt] [PATCH 2/2] New utility functions virFileCreate and virDirCreate
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.)
Return from both of these functions is 0 on success, or the value of
errno if there was a failure.
---
src/util/util.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
src/util/util.h | 9 ++
2 files changed, 256 insertions(+), 0 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index 1d493de..d9a8e6e 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -1126,6 +1126,253 @@ int virFileExists(const char *path)
return(0);
}
+
+static int virFileCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) {
+ int fd = -1;
+ int ret = 0;
+
+ if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("failed to create file %s"),
+ path);
+ goto error;
+ }
+ if ((getuid() == 0) && ((uid != 0) || (gid != 0))) {
+ if (fchown(fd, uid, gid) < 0) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("cannot chown %s to (%u,
%u)"),
+ path, uid, gid);
+ 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;
+}
+
+static int virDirCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) {
+ int ret = 0;
+
+ if (mkdir(path, mode) < 0) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("failed to create directory %s"),
+ path);
+ goto error;
+ }
+
+ if ((getuid() == 0) && ((uid != 0) || (gid != getgid()))) {
+ if (chown(path, uid, gid) < 0) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("cannot chown %s to (%u,
%u)"),
+ path, uid, gid);
+ goto error;
+ }
+ }
+error:
+ return ret;
+}
+
+#ifndef WIN32
+int virFileCreate(const char *path, mode_t mode,
+ uid_t uid, gid_t gid, unsigned int flags) {
+ pid_t pid;
+ int waitret, status, ret = 0;
+ int fd = -1;
+
+ if ((!(flags & VIR_FILE_CREATE_AS_UID))
+ || (getuid() != 0)
+ || ((uid == 0) && (gid == 0))) {
+ return virFileCreateSimple(path, mode, uid, gid);
+ }
+
+ /* 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);
+ return ret;
+ }
+ ret = WEXITSTATUS(status);
+ if (!WIFEXITED(status) || (ret == EACCES)) {
+ /* fall back to the simpler method, which works better in
+ * some cases */
+ /* Should we log a warning here? */
+ return virFileCreateSimple(path, mode, uid, gid);
+ }
+ if ((ret == 0) && (gid != 0)) {
+ /* check if group was set properly by creating after
+ * setgid. If not, try doing it with chown */
+ struct stat st;
+ if (stat(path, &st) == -1) {
+ ret = errno;
+ virReportSystemError(NULL, errno,
+ _("stat of '%s' failed"),
+ path);
+ return ret;
+ }
+ if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("cannot chown %s to group
%u"),
+ path, gid);
+ }
+ }
+ 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) {
+ pid_t pid;
+ int waitret;
+ int status, ret = 0;
+
+ if ((!(flags & VIR_FILE_CREATE_AS_UID))
+ || (getuid() != 0)
+ || ((uid == 0) && (gid == 0))) {
+ return virDirCreateSimple(path, mode, uid, gid);
+ }
+
+ 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, &ret, 0) == -1) && (errno ==
EINTR));
+ if (waitret == -1) {
+ ret = errno;
+ virReportSystemError(NULL, errno,
+ _("failed to wait for child creating
'%s'"),
+ path);
+ return ret;
+ }
+ ret = WEXITSTATUS(status);
+ if (!WIFEXITED(status) || (ret == EACCES)) {
+ /* fall back to the simpler method, which works better in some cases */
+ /* Should we log a warning here? */
+ return virDirCreateSimple(path, mode, uid, gid);
+ }
+ if ((ret == 0) && (gid != 0)) {
+ /* check if group was set properly by creating after
+ * setgid. If not, try doing it with chown */
+ struct stat st;
+ if (stat(path, &st) == -1) {
+ ret = errno;
+ virReportSystemError(NULL, errno,
+ _("stat of '%s' failed"),
+ path);
+ return ret;
+ }
+ if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("cannot chown %s to group
%u"),
+ path, gid);
+ }
+ }
+
+ 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);
+}
+
+int virDirCreate(const char *path, mode_t mode,
+ uid_t uid, gid_t gid, unsigned int flags) {
+ return virDirCreateSimple(path, mode, uid, gid);
+}
+#endif
+
int virFileMakePath(const char *path)
{
struct stat st;
diff --git a/src/util/util.h b/src/util/util.h
index 5e70038..2762862 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -111,6 +111,15 @@ char *virFindFileInPath(const char *file);
int virFileExists(const char *path);
+enum {
+ VIR_FILE_CREATE_NONE = 0,
+ VIR_FILE_CREATE_AS_UID = (1 << 0),
+};
+
+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
11:46 p.m.
New subject: [libvirt] [PATCH 0/3] Patchset to avoid uid problems related to root-squash NFS
The first two patches add some utility functions, obsoleting the
earlier two patches I sent to the list on Jan 8:
https://www.redhat.com/archives/libvir-list/2010-January/msg00114.html
The third patch uses these new utilities to rework the storage volume
build functions to eliminate problems with pools located on
root-squashing NFS servers.
Note that the method of setting uid and gid of the created volumes
remains the same in all cases where the pool containing the volume
isn't type netfs - the volume will be created, then chowned. Only in
the case that the volume will be in a pool of type netfs do we use a
new method - first fork() and setuid/setgid in the child process
before creating the volume. After returning from the child process, a
couple extra steps are taken - 1) check if the child failed to create
the new volume, and if so, try again as root, and 2) check if the
volume really was created with the desired uid/gid, and if not, try
setting it with chown. (This final step solves the problem of a parent
directory that has the setgid bit on - simple calling setgid on the
process that creates the file will then have no effect on the gid of
the new file - it must be set explicitly after creation).
The one problem I have with this final patch is the way that I
determine if the pool containing the volume is of type netfs. The
virStorageVolDef itself has no information about the pool; that
information is only known a few layers up the call chain when someone
is getting the buildVol or .buildVolFrom memor of a virStorageBackend
struct. The way that I found to maintain the information of pool type
(or more precisely, whether this is a netfs pool or something else)
was to make buildVol and buildVolFrom point to different functions in
the case of virStorageBackenNetFilesystem (it previously pointed to
the same function as for type dir and fs). Since that first level
function is just a stub that calls
_virStorageBackendFileSystemVolBuild with a flag that is always 0, and
since _virStorageBackendFileSystemVolBuild then calls the actual
"create_func" with that same flag, AND since flag is currently unused
by any of these functions, I just have the toplevel stub function set
the bit "VIR_STORAGE_BUILD_NETFS" in the flag, and use that
information when the time comes in the individual create_funcs.
I don't like this because as far as I know, the flags are really there
to allow for future expansion in the public API, and I've now
subverted that usage 2/3 of the way down the call chain.
So, I would appreciate either some help in deciding the best way for
lower level functions (like the functions pointed to by create_func)
to learn the type of the pool that will contain the volume pointed to
by the virStorageVolDefPtr, or alternately affirmation that my
subversion of the existing flags is acceptable.
P.S. I have tested creation of raw and qcow volumes in several
differnet scenarios (eg, parent directory isn't owned by the desired
uid, creating a need to fall back to original method, parent directory
has setgid bit on, etc.) with liberal DEBUGs throughout the code and
verified that pretty much every path works.
12:25 a.m.
New subject: [libvirt] [PATCH 0/3] Take 2 - Patchset to avoid uid problems in volume creation
The first patch is unchanged. The 2nd and 3rd patches reflact Dan's
suggestions. (wrt Cole's concern about the pool object being unlocked
during the buildVol call - Dan pointed out that this is read-only
usage of the object, and it can't be deleted while in use, so it's
safe to pass it in here.)
Note that I've tested this as much as I can with my own rig plus some
DEBUG statements and small code changes (in particular, forcing
creation of volumes on local filesystems to use the code intended for
netfs), but testing on real NFS (as well as Samba to see if it solves
that problem too) would be very much appreciated.
12:25 a.m.
New subject: [libvirt] [PATCH 1/3] Add virRunWithHook.
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 44a4b2f..1d493de 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -792,9 +792,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;
@@ -811,7 +813,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;
}
@@ -867,9 +869,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;
@@ -895,6 +899,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;
--
1.6.6
4:35 a.m.
New subject: [libvirt] [PATCH 1/3] Add virRunWithHook.
On Wed, Jan 13, 2010 at 01:25:05AM -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 44a4b2f..1d493de 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -792,9 +792,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;
@@ -811,7 +813,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;
}
@@ -867,9 +869,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;
@@ -895,6 +899,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
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
12:25 a.m.
New subject: [libvirt] [PATCH 2/3] New utility functions virFileCreate and virDirCreate
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.)
Return from both of these functions is 0 on success, or the value of
errno if there was a failure.
---
src/util/util.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
src/util/util.h | 9 ++
2 files changed, 256 insertions(+), 0 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index 1d493de..1cb29f4 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -1126,6 +1126,253 @@ int virFileExists(const char *path)
return(0);
}
+
+static int virFileCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) {
+ int fd = -1;
+ int ret = 0;
+
+ if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("failed to create file
'%s'"),
+ path);
+ goto error;
+ }
+ if ((getuid() == 0) && ((uid != 0) || (gid != 0))) {
+ if (fchown(fd, uid, gid) < 0) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("cannot chown '%s' to (%u,
%u)"),
+ path, uid, gid);
+ 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;
+}
+
+static int virDirCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) {
+ int ret = 0;
+
+ if (mkdir(path, mode) < 0) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("failed to create directory
'%s'"),
+ path);
+ goto error;
+ }
+
+ if ((getuid() == 0) && ((uid != 0) || (gid != getgid()))) {
+ if (chown(path, uid, gid) < 0) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("cannot chown '%s' to (%u,
%u)"),
+ path, uid, gid);
+ goto error;
+ }
+ }
+error:
+ return ret;
+}
+
+#ifndef WIN32
+int virFileCreate(const char *path, mode_t mode,
+ uid_t uid, gid_t gid, unsigned int flags) {
+ pid_t pid;
+ int waitret, status, ret = 0;
+ int fd = -1;
+
+ if ((!(flags & VIR_FILE_CREATE_AS_UID))
+ || (getuid() != 0)
+ || ((uid == 0) && (gid == 0))) {
+ return virFileCreateSimple(path, mode, uid, gid);
+ }
+
+ /* 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);
+ return ret;
+ }
+ ret = WEXITSTATUS(status);
+ if (!WIFEXITED(status) || (ret == EACCES)) {
+ /* fall back to the simpler method, which works better in
+ * some cases */
+ /* Should we log a warning here? */
+ return virFileCreateSimple(path, mode, uid, gid);
+ }
+ if ((ret == 0) && (gid != 0)) {
+ /* check if group was set properly by creating after
+ * setgid. If not, try doing it with chown */
+ struct stat st;
+ if (stat(path, &st) == -1) {
+ ret = errno;
+ virReportSystemError(NULL, errno,
+ _("stat of '%s' failed"),
+ path);
+ return ret;
+ }
+ if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("cannot chown '%s' to
group %u"),
+ path, gid);
+ }
+ }
+ 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) {
+ pid_t pid;
+ int waitret;
+ int status, ret = 0;
+
+ if ((!(flags & VIR_FILE_CREATE_AS_UID))
+ || (getuid() != 0)
+ || ((uid == 0) && (gid == 0))) {
+ return virDirCreateSimple(path, mode, uid, gid);
+ }
+
+ 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);
+ return ret;
+ }
+ ret = WEXITSTATUS(status);
+ if (!WIFEXITED(status) || (ret == EACCES)) {
+ /* fall back to the simpler method, which works better in some cases */
+ /* Should we log a warning here? */
+ return virDirCreateSimple(path, mode, uid, gid);
+ }
+ if ((ret == 0) && (gid != 0)) {
+ /* check if group was set properly by creating after
+ * setgid. If not, try doing it with chown */
+ struct stat st;
+ if (stat(path, &st) == -1) {
+ ret = errno;
+ virReportSystemError(NULL, errno,
+ _("stat of '%s' failed"),
+ path);
+ return ret;
+ }
+ if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("cannot chown '%s' to
group %u"),
+ path, gid);
+ }
+ }
+
+ 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);
+}
+
+int virDirCreate(const char *path, mode_t mode,
+ uid_t uid, gid_t gid, unsigned int flags) {
+ return virDirCreateSimple(path, mode, uid, gid);
+}
+#endif
+
int virFileMakePath(const char *path)
{
struct stat st;
diff --git a/src/util/util.h b/src/util/util.h
index 5e70038..2762862 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -111,6 +111,15 @@ char *virFindFileInPath(const char *file);
int virFileExists(const char *path);
+enum {
+ VIR_FILE_CREATE_NONE = 0,
+ VIR_FILE_CREATE_AS_UID = (1 << 0),
+};
+
+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
4:36 a.m.
New subject: [libvirt] [PATCH 2/3] New utility functions virFileCreate and virDirCreate
On Wed, Jan 13, 2010 at 01:25:06AM -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.)
Return from both of these functions is 0 on success, or the value of
errno if there was a failure.
---
src/util/util.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
src/util/util.h | 9 ++
2 files changed, 256 insertions(+), 0 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index 1d493de..1cb29f4 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -1126,6 +1126,253 @@ int virFileExists(const char *path)
return(0);
}
+
+static int virFileCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) {
+ int fd = -1;
+ int ret = 0;
+
+ if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("failed to create file
'%s'"),
+ path);
+ goto error;
+ }
+ if ((getuid() == 0) && ((uid != 0) || (gid != 0))) {
+ if (fchown(fd, uid, gid) < 0) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("cannot chown '%s' to (%u,
%u)"),
+ path, uid, gid);
+ 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;
+}
+
+static int virDirCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) {
+ int ret = 0;
+
+ if (mkdir(path, mode) < 0) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("failed to create directory
'%s'"),
+ path);
+ goto error;
+ }
+
+ if ((getuid() == 0) && ((uid != 0) || (gid != getgid()))) {
+ if (chown(path, uid, gid) < 0) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("cannot chown '%s' to (%u,
%u)"),
+ path, uid, gid);
+ goto error;
+ }
+ }
+error:
+ return ret;
+}
+
+#ifndef WIN32
+int virFileCreate(const char *path, mode_t mode,
+ uid_t uid, gid_t gid, unsigned int flags) {
+ pid_t pid;
+ int waitret, status, ret = 0;
+ int fd = -1;
+
+ if ((!(flags & VIR_FILE_CREATE_AS_UID))
+ || (getuid() != 0)
+ || ((uid == 0) && (gid == 0))) {
+ return virFileCreateSimple(path, mode, uid, gid);
+ }
+
+ /* 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);
+ return ret;
+ }
+ ret = WEXITSTATUS(status);
+ if (!WIFEXITED(status) || (ret == EACCES)) {
+ /* fall back to the simpler method, which works better in
+ * some cases */
+ /* Should we log a warning here? */
+ return virFileCreateSimple(path, mode, uid, gid);
+ }
+ if ((ret == 0) && (gid != 0)) {
+ /* check if group was set properly by creating after
+ * setgid. If not, try doing it with chown */
+ struct stat st;
+ if (stat(path, &st) == -1) {
+ ret = errno;
+ virReportSystemError(NULL, errno,
+ _("stat of '%s' failed"),
+ path);
+ return ret;
+ }
+ if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("cannot chown '%s' to
group %u"),
+ path, gid);
+ }
+ }
+ 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) {
+ pid_t pid;
+ int waitret;
+ int status, ret = 0;
+
+ if ((!(flags & VIR_FILE_CREATE_AS_UID))
+ || (getuid() != 0)
+ || ((uid == 0) && (gid == 0))) {
+ return virDirCreateSimple(path, mode, uid, gid);
+ }
+
+ 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);
+ return ret;
+ }
+ ret = WEXITSTATUS(status);
+ if (!WIFEXITED(status) || (ret == EACCES)) {
+ /* fall back to the simpler method, which works better in some cases */
+ /* Should we log a warning here? */
+ return virDirCreateSimple(path, mode, uid, gid);
+ }
+ if ((ret == 0) && (gid != 0)) {
+ /* check if group was set properly by creating after
+ * setgid. If not, try doing it with chown */
+ struct stat st;
+ if (stat(path, &st) == -1) {
+ ret = errno;
+ virReportSystemError(NULL, errno,
+ _("stat of '%s' failed"),
+ path);
+ return ret;
+ }
+ if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("cannot chown '%s' to
group %u"),
+ path, gid);
+ }
+ }
+
+ 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);
+}
+
+int virDirCreate(const char *path, mode_t mode,
+ uid_t uid, gid_t gid, unsigned int flags) {
+ return virDirCreateSimple(path, mode, uid, gid);
+}
+#endif
+
int virFileMakePath(const char *path)
{
struct stat st;
diff --git a/src/util/util.h b/src/util/util.h
index 5e70038..2762862 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -111,6 +111,15 @@ char *virFindFileInPath(const char *file);
int virFileExists(const char *path);
+enum {
+ VIR_FILE_CREATE_NONE = 0,
+ VIR_FILE_CREATE_AS_UID = (1 << 0),
+};
+
+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
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
8:24 a.m.
New subject: [libvirt] [PATCH 2/3] New utility functions virFileCreate and virDirCreate
Quoting Laine Stump (laine(a)laine.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.)
Return from both of these functions is 0 on success, or the value of
errno if there was a failure.
---
src/util/util.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
src/util/util.h | 9 ++
2 files changed, 256 insertions(+), 0 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index 1d493de..1cb29f4 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -1126,6 +1126,253 @@ int virFileExists(const char *path)
return(0);
}
+
+static int virFileCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) {
+ int fd = -1;
+ int ret = 0;
+
+ if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("failed to create file
'%s'"),
+ path);
+ goto error;
+ }
+ if ((getuid() == 0) && ((uid != 0) || (gid != 0))) {
How about checking for CAP_CHOWN instead of getuid()==0? Otherwise
if I try installing this certain ways, virFileCreateSimple() will refuse
to try the chown even though it could succeed.
-serge
8:32 a.m.
New subject: [libvirt] [PATCH 2/3] New utility functions virFileCreate and virDirCreate
Quoting Serge E. Hallyn (serue(a)us.ibm.com):
Quoting Laine Stump (laine(a)laine.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.)
>
> Return from both of these functions is 0 on success, or the value of
> errno if there was a failure.
> ---
> src/util/util.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> src/util/util.h | 9 ++
> 2 files changed, 256 insertions(+), 0 deletions(-)
>
> diff --git a/src/util/util.c b/src/util/util.c
> index 1d493de..1cb29f4 100644
> --- a/src/util/util.c
> +++ b/src/util/util.c
> @@ -1126,6 +1126,253 @@ int virFileExists(const char *path)
> return(0);
> }
>
> +
> +static int virFileCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid)
{
> + int fd = -1;
> + int ret = 0;
> +
> + if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) {
> + ret = errno;
> + virReportSystemError(NULL, errno, _("failed to create file
'%s'"),
> + path);
> + goto error;
> + }
> + if ((getuid() == 0) && ((uid != 0) || (gid != 0))) {
How about checking for CAP_CHOWN instead of getuid()==0? Otherwise
if I try installing this certain ways, virFileCreateSimple() will refuse
to try the chown even though it could succeed.
I guess for certain netfs's the uid might matter more, so checking for
either condition - or in fact just trying the fchown, then doing a stat,
then making sure st.st_{ug}id are correct after the fact - seems useful.
-serge
10:44 a.m.
New subject: [libvirt] [PATCH 2/3] New utility functions virFileCreate and virDirCreate
On 01/13/2010 09:32 AM, Serge E. Hallyn wrote:
Quoting Serge E. Hallyn (serue(a)us.ibm.com):
> Quoting Laine Stump (laine(a)laine.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.)
>>
>> Return from both of these functions is 0 on success, or the value of
>> errno if there was a failure.
>> ---
>> src/util/util.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>> src/util/util.h | 9 ++
>> 2 files changed, 256 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/util/util.c b/src/util/util.c
>> index 1d493de..1cb29f4 100644
>> --- a/src/util/util.c
>> +++ b/src/util/util.c
>> @@ -1126,6 +1126,253 @@ int virFileExists(const char *path)
>> return(0);
>> }
>>
>> +
>> +static int virFileCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t
gid) {
>> + int fd = -1;
>> + int ret = 0;
>> +
>> + if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode))< 0) {
>> + ret = errno;
>> + virReportSystemError(NULL, errno, _("failed to create file
'%s'"),
>> + path);
>> + goto error;
>> + }
>> + if ((getuid() == 0)&& ((uid != 0) || (gid != 0))) {
>>
> How about checking for CAP_CHOWN instead of getuid()==0? Otherwise
> if I try installing this certain ways, virFileCreateSimple() will refuse
> to try the chown even though it could succeed.
>
I guess for certain netfs's the uid might matter more, so checking for
either condition - or in fact just trying the fchown, then doing a stat,
then making sure st.st_{ug}id are correct after the fact - seems useful.
That was actually just a duplication of existing functionality from the
code that will now call virFileCreate() (right down to the lack of any
error reporting if you're not running as root and the caller requests
the file to be created with a different uid).
The same check/chown is done in a couple other places in this patch
series, so your comment applies to those as well.
If I were to remove the check of current uid, should a failure of chown
then be reported as an error, or ignored (making it closer to current
behavior for non-root users)? And does this need to be added to the
current patch, or can it be considered a follow-on? (I guess the
question behind that is do people commonly use libvirt in a manner that
would be affected by this, or is this theorizing about something someone
*might* do?)
12:25 a.m.
New subject: [libvirt] [PATCH 3/3] Create storage volumes directly with desired uid/gid.
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.
---
src/storage/storage_backend.c | 115 ++++++++++++++++++++++++++++----
src/storage/storage_backend.h | 8 ++-
src/storage/storage_backend_disk.c | 3 +-
src/storage/storage_backend_fs.c | 25 +++----
src/storage/storage_backend_logical.c | 3 +-
src/storage/storage_driver.c | 4 +-
6 files changed, 123 insertions(+), 35 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 9dc801c..1a39a55 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)
@@ -229,6 +230,15 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn,
goto cleanup;
}
+ if (getuid() == 0) {
+ if (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 (close(fd) < 0) {
virReportSystemError(conn, errno,
_("cannot close file '%s'"),
@@ -247,12 +257,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 +275,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 +474,82 @@ 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(NULL, errno,
+ _("Cannot run %s to create %s"),
+ cmdargv[0], vol->target.path);
+ return -1;
+ }
+ if (stat(vol->target.path, &st) < 0) {
+ virReportSystemError(NULL, errno,
+ _("%s failed to create %s"),
+ cmdargv[0], vol->target.path);
+ return -1;
+ }
+ }
+
+ gid = (vol->target.perms.gid != 0) ? vol->target.perms.gid : -1;
+ uid = (vol->target.perms.uid != 0) ? vol->target.perms.uid : -1;
+ if (((gid != -1) && (gid != st.st_gid))
+ || ((uid != -1) && (uid != st.st_uid))) {
+ /* either uid or gid wasn't properly set during creation */
+ if (chown(vol->target.path, uid, gid) < 0) {
+ virReportSystemError(NULL, errno, _("cannot chown %s to (%u,
%u)"),
+ vol->target.path, uid, gid);
+ 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 +707,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 +719,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 +761,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 88c6161..fc5d8fb 100644
--- a/src/storage/storage_backend.h
+++ b/src/storage/storage_backend.h
@@ -34,14 +34,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 4fe40b3..1f98627 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -737,6 +737,7 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
}
static int createFileDir(virConnectPtr conn,
+ virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
unsigned int flags ATTRIBUTE_UNUSED) {
@@ -747,7 +748,10 @@ static int createFileDir(virConnectPtr conn,
return -1;
}
- if (mkdir(vol->target.path, vol->target.perms.mode) < 0) {
+ if (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, errno,
_("cannot create path '%s'"),
vol->target.path);
@@ -759,6 +763,7 @@ static int createFileDir(virConnectPtr conn,
static int
_virStorageBackendFileSystemVolBuild(virConnectPtr conn,
+ virStoragePoolObjPtr pool,
virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol)
{
@@ -794,7 +799,7 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn,
return -1;
}
- if (create_func(conn, vol, inputvol, 0) < 0)
+ if (create_func(conn, pool, vol, inputvol, 0) < 0)
return -1;
if ((fd = open(vol->target.path, O_RDONLY)) < 0) {
@@ -804,16 +809,6 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn,
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'"),
@@ -839,8 +834,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 +844,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);
--
1.6.6
11:46 p.m.
New subject: [libvirt] [PATCH 1/3] Add virRunWithHook.
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 44a4b2f..1d493de 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -792,9 +792,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;
@@ -811,7 +813,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;
}
@@ -867,9 +869,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;
@@ -895,6 +899,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;
--
1.6.6
11:54 a.m.
New subject: [libvirt] [PATCH 1/3] Add virRunWithHook.
On Mon, Jan 11, 2010 at 12:46:10AM -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 44a4b2f..1d493de 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -792,9 +792,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;
@@ -811,7 +813,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;
}
@@ -867,9 +869,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;
@@ -895,6 +899,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
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
11:46 p.m.
New subject: [libvirt] [PATCH 2/3] New utility functions virFileCreate and virDirCreate
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.)
Return from both of these functions is 0 on success, or the value of
errno if there was a failure.
---
src/util/util.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
src/util/util.h | 9 ++
2 files changed, 256 insertions(+), 0 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index 1d493de..6d31f47 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -1126,6 +1126,253 @@ int virFileExists(const char *path)
return(0);
}
+
+static int virFileCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) {
+ int fd = -1;
+ int ret = 0;
+
+ if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("failed to create file %s"),
+ path);
+ goto error;
+ }
+ if ((getuid() == 0) && ((uid != 0) || (gid != 0))) {
+ if (fchown(fd, uid, gid) < 0) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("cannot chown %s to (%u,
%u)"),
+ path, uid, gid);
+ 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;
+}
+
+static int virDirCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) {
+ int ret = 0;
+
+ if (mkdir(path, mode) < 0) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("failed to create directory %s"),
+ path);
+ goto error;
+ }
+
+ if ((getuid() == 0) && ((uid != 0) || (gid != getgid()))) {
+ if (chown(path, uid, gid) < 0) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("cannot chown %s to (%u,
%u)"),
+ path, uid, gid);
+ goto error;
+ }
+ }
+error:
+ return ret;
+}
+
+#ifndef WIN32
+int virFileCreate(const char *path, mode_t mode,
+ uid_t uid, gid_t gid, unsigned int flags) {
+ pid_t pid;
+ int waitret, status, ret = 0;
+ int fd = -1;
+
+ if ((!(flags & VIR_FILE_CREATE_AS_UID))
+ || (getuid() != 0)
+ || ((uid == 0) && (gid == 0))) {
+ return virFileCreateSimple(path, mode, uid, gid);
+ }
+
+ /* 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);
+ return ret;
+ }
+ ret = WEXITSTATUS(status);
+ if (!WIFEXITED(status) || (ret == EACCES)) {
+ /* fall back to the simpler method, which works better in
+ * some cases */
+ /* Should we log a warning here? */
+ return virFileCreateSimple(path, mode, uid, gid);
+ }
+ if ((ret == 0) && (gid != 0)) {
+ /* check if group was set properly by creating after
+ * setgid. If not, try doing it with chown */
+ struct stat st;
+ if (stat(path, &st) == -1) {
+ ret = errno;
+ virReportSystemError(NULL, errno,
+ _("stat of '%s' failed"),
+ path);
+ return ret;
+ }
+ if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("cannot chown %s to group
%u"),
+ path, gid);
+ }
+ }
+ 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) {
+ pid_t pid;
+ int waitret;
+ int status, ret = 0;
+
+ if ((!(flags & VIR_FILE_CREATE_AS_UID))
+ || (getuid() != 0)
+ || ((uid == 0) && (gid == 0))) {
+ return virDirCreateSimple(path, mode, uid, gid);
+ }
+
+ 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);
+ return ret;
+ }
+ ret = WEXITSTATUS(status);
+ if (!WIFEXITED(status) || (ret == EACCES)) {
+ /* fall back to the simpler method, which works better in some cases */
+ /* Should we log a warning here? */
+ return virDirCreateSimple(path, mode, uid, gid);
+ }
+ if ((ret == 0) && (gid != 0)) {
+ /* check if group was set properly by creating after
+ * setgid. If not, try doing it with chown */
+ struct stat st;
+ if (stat(path, &st) == -1) {
+ ret = errno;
+ virReportSystemError(NULL, errno,
+ _("stat of '%s' failed"),
+ path);
+ return ret;
+ }
+ if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("cannot chown %s to group
%u"),
+ path, gid);
+ }
+ }
+
+ 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);
+}
+
+int virDirCreate(const char *path, mode_t mode,
+ uid_t uid, gid_t gid, unsigned int flags) {
+ return virDirCreateSimple(path, mode, uid, gid);
+}
+#endif
+
int virFileMakePath(const char *path)
{
struct stat st;
diff --git a/src/util/util.h b/src/util/util.h
index 5e70038..2762862 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -111,6 +111,15 @@ char *virFindFileInPath(const char *file);
int virFileExists(const char *path);
+enum {
+ VIR_FILE_CREATE_NONE = 0,
+ VIR_FILE_CREATE_AS_UID = (1 << 0),
+};
+
+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
6:24 a.m.
New subject: [libvirt] [PATCH 2/3] New utility functions virFileCreate and virDirCreate
On Mon, Jan 11, 2010 at 12:46:11AM -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.)
Return from both of these functions is 0 on success, or the value of
errno if there was a failure.
---
src/util/util.c | 247 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
src/util/util.h | 9 ++
2 files changed, 256 insertions(+), 0 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index 1d493de..6d31f47 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -1126,6 +1126,253 @@ int virFileExists(const char *path)
return(0);
}
+
+static int virFileCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid) {
+ int fd = -1;
+ int ret = 0;
+
+ if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) {
+ ret = errno;
+ virReportSystemError(NULL, errno, _("failed to create file %s"),
+ path);
Can you just put single quotes around the '%s' placeholder for the path.
Every now & then someone uses a crazy path with whitespace and quotes help
identify that kind of issue. Likewise for the rest of the calls.
Functionally the patch looks good to me.
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
11:46 p.m.
New subject: [libvirt] [PATCH 3/3] Create storage volumes directly with desired uid/gid.
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.
---
src/storage/storage_backend.c | 115 ++++++++++++++++++++++++++++++++------
src/storage/storage_backend.h | 4 +
src/storage/storage_backend_fs.c | 48 ++++++++++------
3 files changed, 132 insertions(+), 35 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 9dc801c..8a312ca 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -229,6 +229,15 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn,
goto cleanup;
}
+ if (getuid() == 0) {
+ if (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 (close(fd) < 0) {
virReportSystemError(conn, errno,
_("cannot close file '%s'"),
@@ -249,10 +258,11 @@ int
virStorageBackendCreateRaw(virConnectPtr conn,
virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
- unsigned int flags ATTRIBUTE_UNUSED)
+ unsigned int flags)
{
int fd = -1;
int ret = -1;
+ int createstat;
unsigned long long remain;
char *buf = NULL;
@@ -263,14 +273,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,
+ (flags & VIR_STORAGE_BUILD_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 +472,81 @@ 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,
+ virStorageVolDefPtr vol,
+ const char **cmdargv,
+ unsigned int flags) {
+ struct stat st;
+ gid_t gid;
+ uid_t uid;
+ int filecreated = 0;
+
+ if ((flags & VIR_STORAGE_BUILD_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(NULL, errno,
+ _("Cannot run %s to create %s"),
+ cmdargv[0], vol->target.path);
+ return -1;
+ }
+ if (stat(vol->target.path, &st) < 0) {
+ virReportSystemError(NULL, errno,
+ _("%s failed to create %s"),
+ cmdargv[0], vol->target.path);
+ return -1;
+ }
+ }
+
+ gid = (vol->target.perms.gid != 0) ? vol->target.perms.gid : -1;
+ uid = (vol->target.perms.uid != 0) ? vol->target.perms.uid : -1;
+ if (((gid != -1) && (gid != st.st_gid))
+ || ((uid != -1) && (uid != st.st_uid))) {
+ /* either uid or gid wasn't properly set during creation */
+ if (chown(vol->target.path, uid, gid) < 0) {
+ virReportSystemError(NULL, errno, _("cannot chown %s to (%u,
%u)"),
+ vol->target.path, uid, gid);
+ return -1;
+ }
+ }
+ return 0;
+}
+
static int
virStorageBackendCreateQemuImg(virConnectPtr conn,
virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
- unsigned int flags ATTRIBUTE_UNUSED)
+ unsigned int flags)
{
+ int ret;
char size[100];
char *create_tool;
short use_kvmimg;
@@ -616,14 +704,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, vol, imgargv, flags);
VIR_FREE(imgargv[0]);
- return 0;
+ return ret;
}
/*
@@ -636,6 +720,7 @@ virStorageBackendCreateQcowCreate(virConnectPtr conn,
virStorageVolDefPtr inputvol,
unsigned int flags ATTRIBUTE_UNUSED)
{
+ int ret;
char size[100];
const char *imgargv[4];
@@ -672,14 +757,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, vol, imgargv, flags);
VIR_FREE(imgargv[0]);
- return 0;
+ return ret;
}
virStorageBackendBuildVolFrom
diff --git a/src/storage/storage_backend.h b/src/storage/storage_backend.h
index 88c6161..1e33308 100644
--- a/src/storage/storage_backend.h
+++ b/src/storage/storage_backend.h
@@ -40,6 +40,10 @@ typedef int (*virStorageBackendRefreshVol)(virConnectPtr conn,
virStoragePoolObj
typedef int (*virStorageBackendDeleteVol)(virConnectPtr conn, virStoragePoolObjPtr pool,
virStorageVolDefPtr vol, unsigned int flags);
typedef int (*virStorageBackendBuildVolFrom)(virConnectPtr conn, virStorageVolDefPtr
origvol, virStorageVolDefPtr newvol, unsigned int flags);
+enum {
+ VIR_STORAGE_BUILD_NETFS = (1 << 0),
+};
+
/* File creation/cloning functions used for cloning between backends */
int virStorageBackendCreateRaw(virConnectPtr conn,
virStorageVolDefPtr vol,
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 4fe40b3..8fdf8b7 100644
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -739,7 +739,7 @@ virStorageBackendFileSystemVolCreate(virConnectPtr conn,
static int createFileDir(virConnectPtr conn,
virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
- unsigned int flags ATTRIBUTE_UNUSED) {
+ unsigned int flags) {
if (inputvol) {
virStorageReportError(conn, VIR_ERR_INTERNAL_ERROR,
"%s",
@@ -747,7 +747,9 @@ static int createFileDir(virConnectPtr conn,
return -1;
}
- if (mkdir(vol->target.path, vol->target.perms.mode) < 0) {
+ if (virDirCreate(vol->target.path, vol->target.perms.mode,
+ vol->target.perms.uid, vol->target.perms.gid,
+ flags & VIR_STORAGE_BUILD_NETFS ? VIR_FILE_CREATE_AS_UID : 0) !=
0) {
virReportSystemError(conn, errno,
_("cannot create path '%s'"),
vol->target.path);
@@ -760,7 +762,8 @@ static int createFileDir(virConnectPtr conn,
static int
_virStorageBackendFileSystemVolBuild(virConnectPtr conn,
virStorageVolDefPtr vol,
- virStorageVolDefPtr inputvol)
+ virStorageVolDefPtr inputvol,
+ unsigned int flags)
{
int fd;
virStorageBackendBuildVolFrom create_func;
@@ -794,7 +797,7 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn,
return -1;
}
- if (create_func(conn, vol, inputvol, 0) < 0)
+ if (create_func(conn, vol, inputvol, flags) < 0)
return -1;
if ((fd = open(vol->target.path, O_RDONLY)) < 0) {
@@ -804,16 +807,6 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn,
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'"),
@@ -840,7 +833,15 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn,
static int
virStorageBackendFileSystemVolBuild(virConnectPtr conn,
virStorageVolDefPtr vol) {
- return _virStorageBackendFileSystemVolBuild(conn, vol, NULL);
+ return _virStorageBackendFileSystemVolBuild(conn, vol, NULL, 0);
+}
+
+/* version for vols created in netfs-based pools */
+static int
+virStorageBackendFileSystemNetVolBuild(virConnectPtr conn,
+ virStorageVolDefPtr vol) {
+ return _virStorageBackendFileSystemVolBuild(conn, vol, NULL,
+ VIR_STORAGE_BUILD_NETFS);
}
/*
@@ -851,9 +852,20 @@ virStorageBackendFileSystemVolBuildFrom(virConnectPtr conn,
virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
unsigned int flags ATTRIBUTE_UNUSED) {
- return _virStorageBackendFileSystemVolBuild(conn, vol, inputvol);
+ return _virStorageBackendFileSystemVolBuild(conn, vol, inputvol, 0);
+}
+
+/* version for vols created in netfs-based pools */
+static int
+virStorageBackendFileSystemNetVolBuildFrom(virConnectPtr conn,
+ virStorageVolDefPtr vol,
+ virStorageVolDefPtr inputvol,
+ unsigned int flags ATTRIBUTE_UNUSED) {
+ return _virStorageBackendFileSystemVolBuild(conn, vol, inputvol,
+ VIR_STORAGE_BUILD_NETFS);
}
+
/**
* Remove a volume - just unlinks for now
*/
@@ -959,8 +971,8 @@ virStorageBackend virStorageBackendNetFileSystem = {
.refreshPool = virStorageBackendFileSystemRefresh,
.stopPool = virStorageBackendFileSystemStop,
.deletePool = virStorageBackendFileSystemDelete,
- .buildVol = virStorageBackendFileSystemVolBuild,
- .buildVolFrom = virStorageBackendFileSystemVolBuildFrom,
+ .buildVol = virStorageBackendFileSystemNetVolBuild,
+ .buildVolFrom = virStorageBackendFileSystemNetVolBuildFrom,
.createVol = virStorageBackendFileSystemVolCreate,
.refreshVol = virStorageBackendFileSystemVolRefresh,
.deleteVol = virStorageBackendFileSystemVolDelete,
--
1.6.6
6:36 a.m.
New subject: [libvirt] [PATCH 3/3] Create storage volumes directly with desired uid/gid.
On Mon, Jan 11, 2010 at 12:46:12AM -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.
---
src/storage/storage_backend.c | 115 ++++++++++++++++++++++++++++++++------
src/storage/storage_backend.h | 4 +
src/storage/storage_backend_fs.c | 48 ++++++++++------
3 files changed, 132 insertions(+), 35 deletions(-)
[snip]
@@ -840,7 +833,15 @@
_virStorageBackendFileSystemVolBuild(virConnectPtr conn,
static int
virStorageBackendFileSystemVolBuild(virConnectPtr conn,
virStorageVolDefPtr vol) {
- return _virStorageBackendFileSystemVolBuild(conn, vol, NULL);
+ return _virStorageBackendFileSystemVolBuild(conn, vol, NULL, 0);
+}
+
+/* version for vols created in netfs-based pools */
+static int
+virStorageBackendFileSystemNetVolBuild(virConnectPtr conn,
+ virStorageVolDefPtr vol) {
+ return _virStorageBackendFileSystemVolBuild(conn, vol, NULL,
+ VIR_STORAGE_BUILD_NETFS);
}
/*
@@ -851,9 +852,20 @@ virStorageBackendFileSystemVolBuildFrom(virConnectPtr conn,
virStorageVolDefPtr vol,
virStorageVolDefPtr inputvol,
unsigned int flags ATTRIBUTE_UNUSED) {
- return _virStorageBackendFileSystemVolBuild(conn, vol, inputvol);
+ return _virStorageBackendFileSystemVolBuild(conn, vol, inputvol, 0);
+}
+
+/* version for vols created in netfs-based pools */
+static int
+virStorageBackendFileSystemNetVolBuildFrom(virConnectPtr conn,
+ virStorageVolDefPtr vol,
+ virStorageVolDefPtr inputvol,
+ unsigned int flags ATTRIBUTE_UNUSED) {
+ return _virStorageBackendFileSystemVolBuild(conn, vol, inputvol,
+ VIR_STORAGE_BUILD_NETFS);
}
I think I'd be inclined to edit storage_backend.h and change the
typedef int (*virStorageBackendBuildVol)(virConnectPtr conn, virStorageVolDefPtr vol);
to
typedef int (*virStorageBackendBuildVol)(virConnectPtr conn, virStoragePoolObjPtr pool,
virStorageVolDefPtr vol);
so that the build method has direct access to the pool type. Likewise
for the BuildFrom method prototype. Once that change ripples through
all the code, the place where you need to distinguish local vs net
filesytem will have access to the pool object it wants
Regards,
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
9:42 a.m.
New subject: [libvirt] [PATCH 3/3] Create storage volumes directly with desired uid/gid.
On 01/12/2010 07:36 AM, Daniel P. Berrange wrote:
On Mon, Jan 11, 2010 at 12:46:12AM -0500, Laine Stump wrote:
[snip]
> @@ -840,7 +833,15 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn,
> static int
> virStorageBackendFileSystemVolBuild(virConnectPtr conn,
> virStorageVolDefPtr vol) {
> - return _virStorageBackendFileSystemVolBuild(conn, vol, NULL);
> + return _virStorageBackendFileSystemVolBuild(conn, vol, NULL, 0);
> +}
> +
> +/* version for vols created in netfs-based pools */
> +static int
> +virStorageBackendFileSystemNetVolBuild(virConnectPtr conn,
> + virStorageVolDefPtr vol) {
> + return _virStorageBackendFileSystemVolBuild(conn, vol, NULL,
> + VIR_STORAGE_BUILD_NETFS);
> }
>
> /*
> @@ -851,9 +852,20 @@ virStorageBackendFileSystemVolBuildFrom(virConnectPtr conn,
> virStorageVolDefPtr vol,
> virStorageVolDefPtr inputvol,
> unsigned int flags ATTRIBUTE_UNUSED) {
> - return _virStorageBackendFileSystemVolBuild(conn, vol, inputvol);
> + return _virStorageBackendFileSystemVolBuild(conn, vol, inputvol, 0);
> +}
> +
> +/* version for vols created in netfs-based pools */
> +static int
> +virStorageBackendFileSystemNetVolBuildFrom(virConnectPtr conn,
> + virStorageVolDefPtr vol,
> + virStorageVolDefPtr inputvol,
> + unsigned int flags ATTRIBUTE_UNUSED) {
> + return _virStorageBackendFileSystemVolBuild(conn, vol, inputvol,
> + VIR_STORAGE_BUILD_NETFS);
> }
I think I'd be inclined to edit storage_backend.h and change the
typedef int (*virStorageBackendBuildVol)(virConnectPtr conn, virStorageVolDefPtr vol);
to
typedef int (*virStorageBackendBuildVol)(virConnectPtr conn, virStoragePoolObjPtr pool,
virStorageVolDefPtr vol);
so that the build method has direct access to the pool type. Likewise
for the BuildFrom method prototype. Once that change ripples through
all the code, the place where you need to distinguish local vs net
filesytem will have access to the pool object it wants
This is a possibly problematic, since BuildVol is intended to be called
with the pool lock dropped. So maybe BuildVol should have an extra 'int
pool_type' argument rather than the whole pool object.
- Cole
5464
days inactive
5469
days old
19 comments
4 participants
participants (4)
-
Cole Robinson -
Daniel P. Berrange -
Laine Stump -
Serge E. Hallyn