Change default for storage uid/gid from getuid()/getgid() to -1/-1

This allows the config to have a setting that means "leave it alone",
eg when building a pool where the directory already exists the user
may want the current uid/gid of the directory left intact. This
actually gets us back to older behavior - before recent changes to the
pool building code, we weren't as insistent about honoring the uid/gid
settings in the XML, and virt-manager was taking advantage of this
behavior.

As a side benefit, removing calls to getuid/getgid from the XML
parsing functions also seems like a good idea. And having a default
that is different from a common/useful value (0 == root) is a good
thing in general, as it removes ambiguity from decisions (at least one
place in the code was checking for (perms.uid == 0) to see if a
special uid was requested).

Note that this will only affect newly created pools and volumes. Due
to the way that the XML is parsed, then formatted for newly created
volumes, all existing pools/volumes already have an explicit uid and
gid set.

src/conf/storage_conf.c: Remove calls to setuid/setgid for default values
                         of uid/gid, and set them to -1 instead

src/storage/storage_backend.c:
src/storage/storage_backend_fs.c:
        Make account for the new default values of perms.uid
        and perms.gid.

diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c
index b0f326fc39ef6af8d89678dae35aa71d6722b89b..3e37d5fb82673cd98faaec05cc785c50bb9a997e 100644 (file)
--- a/src/conf/storage_conf.c
+++ b/src/conf/storage_conf.c
@@ -539,8 +539,8 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
     if (node == NULL) {
         /* Set default values if there is not <permissions> element */
         perms->mode = defaultmode;
-        perms->uid = getuid();
-        perms->gid = getgid();
+        perms->uid = -1;
+        perms->gid = -1;
         perms->label = NULL;
         return 0;
     }
@@ -564,7 +564,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
     }
 
     if (virXPathNode("./owner", ctxt) == NULL) {
-        perms->uid = getuid();
+        perms->uid = -1;
     } else {
         if (virXPathLong("number(./owner)", ctxt, &v) < 0) {
             virStorageReportError(VIR_ERR_XML_ERROR,
@@ -575,7 +575,7 @@ virStorageDefParsePerms(xmlXPathContextPtr ctxt,
     }
 
     if (virXPathNode("./group", ctxt) == NULL) {
-        perms->gid = getgid();
+        perms->gid = -1;
     } else {
         if (virXPathLong("number(./group)", ctxt, &v) < 0) {
             virStorageReportError(VIR_ERR_XML_ERROR,

diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index 849f01b63c7061d59de0830b3057114a509bbb23..ee9e90feae891ec6ec87b28dcfa20963a28cf666 100644 (file)
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -241,11 +241,10 @@ virStorageBackendCreateBlockFrom(virConnectPtr conn ATTRIBUTE_UNUSED,
     uid = (vol->target.perms.uid != st.st_uid) ? vol->target.perms.uid : -1;
     gid = (vol->target.perms.gid != st.st_gid) ? vol->target.perms.gid : -1;
     if (((uid != -1) || (gid != -1))
-        && (fchown(fd, vol->target.perms.uid, vol->target.perms.gid) < 0)) {
+        && (fchown(fd, uid, gid) < 0)) {
         virReportSystemError(errno,
                              _("cannot chown '%s' to (%u, %u)"),
-                             vol->target.path, vol->target.perms.uid,
-                             vol->target.perms.gid);
+                             vol->target.path, uid, gid);
         goto cleanup;
     }
     if (fchmod(fd, vol->target.perms.mode) < 0) {
@@ -356,10 +355,12 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
         goto cleanup;
     }
 
+    uid_t uid = (vol->target.perms.uid == -1) ? getuid() : vol->target.perms.uid;
+    gid_t gid = (vol->target.perms.gid == -1) ? getgid() : vol->target.perms.gid;
+
     if ((createstat = virFileOperation(vol->target.path,
                                        O_RDWR | O_CREAT | O_EXCL | O_DSYNC,
-                                       vol->target.perms.mode,
-                                       vol->target.perms.uid, vol->target.perms.gid,
+                                       vol->target.perms.mode, uid, gid,
                                        createRawFileOpHook, &hdata,
                                        VIR_FILE_OP_FORCE_PERMS |
                                        (pool->def->type == VIR_STORAGE_POOL_NETFS
@@ -491,14 +492,14 @@ cleanup:
 static int virStorageBuildSetUIDHook(void *data) {
     virStorageVolDefPtr vol = data;
 
-    if ((vol->target.perms.gid != 0)
+    if ((vol->target.perms.gid != -1)
         && (setgid(vol->target.perms.gid) != 0)) {
         virReportSystemError(errno,
                              _("Cannot set gid to %u before creating %s"),
                              vol->target.perms.gid, vol->target.path);
         return -1;
     }
-    if ((vol->target.perms.uid != 0)
+    if ((vol->target.perms.uid != -1)
         && (setuid(vol->target.perms.uid) != 0)) {
         virReportSystemError(errno,
                              _("Cannot set uid to %u before creating %s"),
@@ -517,8 +518,11 @@ static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool,
     int filecreated = 0;
 
     if ((pool->def->type == VIR_STORAGE_POOL_NETFS)
-        && (getuid() == 0)
-        && ((vol->target.perms.uid != 0) || (vol->target.perms.gid != 0))) {
+        && (((getuid() == 0)
+             && (vol->target.perms.uid != -1)
+             && (vol->target.perms.uid != 0))
+            || ((vol->target.perms.gid != -1)
+                && (vol->target.perms.gid != getgid())))) {
         if (virRunWithHook(cmdargv,
                            virStorageBuildSetUIDHook, vol, NULL) == 0) {
             /* command was successfully run, check if the file was created */
@@ -547,8 +551,7 @@ static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool,
         && (chown(vol->target.path, uid, gid) < 0)) {
         virReportSystemError(errno,
                              _("cannot chown %s to (%u, %u)"),
-                             vol->target.path, vol->target.perms.uid,
-                             vol->target.perms.gid);
+                             vol->target.path, uid, gid);
         return -1;
     }
     if (chmod(vol->target.path, vol->target.perms.mode) < 0) {

diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c
index 8279d781f152b7e4d77fcf04cfe390b7e2871c6e..dfd417f47f89b04ab8a5236ec8326ccd0265cfde 100644 (file)
--- a/src/storage/storage_backend_fs.c
+++ b/src/storage/storage_backend_fs.c
@@ -529,16 +529,28 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED,
     /* Now create the final dir in the path with the uid/gid/mode
      * requested in the config. If the dir already exists, just set
      * the perms. */
-    if ((err = virDirCreate(pool->def->target.path,
-                            pool->def->target.perms.mode,
-                            pool->def->target.perms.uid,
-                            pool->def->target.perms.gid,
-                            VIR_DIR_CREATE_FORCE_PERMS | VIR_DIR_CREATE_ALLOW_EXIST |
-                            (pool->def->type == VIR_STORAGE_POOL_NETFS
-                             ? VIR_DIR_CREATE_AS_UID : 0)) != 0)) {
-        virReportSystemError(err, _("cannot create path '%s'"),
-                             pool->def->target.path);
-        goto error;
+
+    struct stat st;
+
+    if ((stat(pool->def->target.path, &st) < 0)
+        || (pool->def->target.perms.uid != -1)) {
+
+        uid_t uid = (pool->def->target.perms.uid == -1)
+            ? getuid() : pool->def->target.perms.uid;
+        gid_t gid = (pool->def->target.perms.gid == -1)
+            ? getgid() : pool->def->target.perms.gid;
+
+        if ((err = virDirCreate(pool->def->target.path,
+                                pool->def->target.perms.mode,
+                                uid, gid,
+                                VIR_DIR_CREATE_FORCE_PERMS |
+                                VIR_DIR_CREATE_ALLOW_EXIST |
+                                (pool->def->type == VIR_STORAGE_POOL_NETFS
+                                 ? VIR_DIR_CREATE_AS_UID : 0)) != 0)) {
+            virReportSystemError(err, _("cannot create path '%s'"),
+                                 pool->def->target.path);
+            goto error;
+        }
     }
     ret = 0;
 error:
@@ -777,8 +789,13 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED,
         return -1;
     }
 
+    uid_t uid = (vol->target.perms.uid == -1)
+        ? getuid() : vol->target.perms.uid;
+    gid_t gid = (vol->target.perms.gid == -1)
+        ? getgid() : vol->target.perms.gid;
+
     if ((err = virDirCreate(vol->target.path, vol->target.perms.mode,
-                            vol->target.perms.uid, vol->target.perms.gid,
+                            uid, gid,
                             VIR_DIR_CREATE_FORCE_PERMS |
                             (pool->def->type == VIR_STORAGE_POOL_NETFS
                              ? VIR_DIR_CREATE_AS_UID : 0))) != 0) {