storage: fs: Don't try to chown directory unless user requested

Currently we try to chown any directory passed to virDirCreate,
even if the user didn't request any explicit owner/group via the
pool/vol XML.

This causes issues with qemu:///session: try to build a pool of
a root owned directory like /tmp, and it fails trying to chown the
directory to the session user. Instead it should just leave things
as they are, unless the user requests changing permissions via
the pool XML.

Similarly this is annoying if creating a storage pool via system
libvirtd of an existing directory in user $HOME, it's now owned
by root.

The virDirCreate function is pretty convoluted, since it needs to
fork off in certain specific cases. Try to document that, to make
it clear where exactly we are changing behavior.

diff --git a/src/util/virfile.c b/src/util/virfile.c
index 984c5713c55205768362b669fd8c4f80cd90a9d5..8a06813925f7dd6ac5f276b3ec9f12bfa3be846a 100644 (file)
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -2303,7 +2303,8 @@ virDirCreateNoFork(const char *path,
         virReportSystemError(errno, _("stat of '%s' failed"), path);
         goto error;
     }
-    if (((st.st_uid != uid) || (st.st_gid != gid))
+    if (((uid != (uid_t) -1 && st.st_uid != uid) ||
+         (gid != (gid_t) -1 && st.st_gid != gid))
         && (chown(path, uid, gid) < 0)) {
         ret = -errno;
         virReportSystemError(errno, _("cannot chown '%s' to (%u, %u)"),
@@ -2335,19 +2336,32 @@ virDirCreate(const char *path,
     gid_t *groups;
     int ngroups;
 
-    /* allow using -1 to mean "current value" */
-    if (uid == (uid_t) -1)
-        uid = geteuid();
-    if (gid == (gid_t) -1)
-        gid = getegid();
-
+    /* Everything after this check is crazyness to allow setting uid/gid
+     * on directories that are on root-squash NFS shares. We only want
+     * to go that route if the follow conditions are true:
+     *
+     * 1) VIR_DIR_CREATE_AS_UID was passed, currently only used when
+     *    directory is being created for a NETFS pool
+     * 2) We are running as root, since that's when the root-squash
+     *    workaround is required.
+     * 3) An explicit uid/gid was requested
+     * 4) The directory doesn't already exist and the ALLOW_EXIST flag
+     *    wasn't passed.
+     *
+     * If any of those conditions are _not_ met, ignore the fork crazyness
+     */
     if ((!(flags & VIR_DIR_CREATE_AS_UID))
         || (geteuid() != 0)
-        || ((uid == 0) && (gid == 0))
-        || ((flags & VIR_DIR_CREATE_ALLOW_EXIST) && (stat(path, &st) >= 0))) {
+        || ((uid == (uid_t) -1) && (gid == (gid_t) -1))
+        || ((flags & VIR_DIR_CREATE_ALLOW_EXIST) && virFileExists(path))) {
         return virDirCreateNoFork(path, mode, uid, gid, flags);
     }
 
+    if (uid == (uid_t) -1)
+        uid = geteuid();
+    if (gid == (gid_t) -1)
+        gid = getegid();
+
     ngroups = virGetGroupList(uid, gid, &groups);
     if (ngroups < 0)
         return -errno;