Rename virFileCreate to virFileOperation, add hook function

It turns out it is also useful to be able to perform other operations
on a file created while running as a different uid (eg, write things
to that file), and possibly to do this to a file that already
exists. This patch adds an optional hook function to the renamed (for
more accuracy of purpose) virFileOperation; the hook will be called
after the file has been opened (possibly created) and gid/mode
checked/set, before closing it.

As with the other operations on the file, if the VIR_FILE_OP_AS_UID
flag is set, this hook function will be called in the context of a
child process forked from the process that called virFileOperation.
The implication here is that, while all data in memory is available to
this hook function, any modification to that data will not be seen by
the caller - the only indication in memory of what happened in the
hook will be the return value (which the hook should set to 0 on
success, or one of the standard errno values on failure).

Another piece of making the function more flexible was to add an
"openflags" argument. This arg should contain exactly the flags to be
passed to open(2), eg O_RDWR | O_EXCL, etc.

In the process of adding the hook to virFileOperation, I also realized
that the bits to fix up file owner/group/mode settings after creation
were being done in the parent process, which could fail, so I moved
them to the child process where they should be.

* src/util/util.[ch]: rename and rework virFileCreate-->virFileOperation,
  and redo flags in virDirCreate
* storage/storage_backend.c, storage/storage_backend_fs.c: update the
  calls to virFileOperation/virDirCreate to reflect changes in the API,
  but don't yet take advantage of the hook.

src/storage/storage_backend.c

@@ -290,10 +290,13 @@ virStorageBackendCreateRaw(virConnectPtr conn ATTRIBUTE_UNUSED,
         return -1;
     }
 
-    if ((createstat = virFileCreate(vol->target.path, vol->target.perms.mode,
-                                    vol->target.perms.uid, vol->target.perms.gid,
-                                    (pool->def->type == VIR_STORAGE_POOL_NETFS
-                                     ? VIR_FILE_CREATE_AS_UID : 0))) < 0) {
+    if ((createstat = virFileOperation(vol->target.path, O_RDWR | O_CREAT | O_EXCL,
+                                       vol->target.perms.mode,
+                                       vol->target.perms.uid, vol->target.perms.gid,
+                                       NULL, NULL,
+                                       VIR_FILE_OP_FORCE_PERMS |
+                                       (pool->def->type == VIR_STORAGE_POOL_NETFS
+                                        ? VIR_FILE_OP_AS_UID : 0))) < 0) {
         virReportSystemError(createstat,
                              _("cannot create path '%s'"),
                              vol->target.path);

src/storage/storage_backend_fs.c

@@ -533,9 +533,9 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED,
                             pool->def->target.perms.mode,
                             pool->def->target.perms.uid,
                             pool->def->target.perms.gid,
-                            VIR_FILE_CREATE_ALLOW_EXIST |
+                            VIR_DIR_CREATE_FORCE_PERMS | VIR_DIR_CREATE_ALLOW_EXIST |
                             (pool->def->type == VIR_STORAGE_POOL_NETFS
-                             ? VIR_FILE_CREATE_AS_UID : 0)) != 0)) {
+                             ? VIR_DIR_CREATE_AS_UID : 0)) != 0)) {
         virReportSystemError(err, _("cannot create path '%s'"),
                              pool->def->target.path);
         goto error;
@@ -779,8 +779,9 @@ static int createFileDir(virConnectPtr conn ATTRIBUTE_UNUSED,
 
     if ((err = virDirCreate(vol->target.path, vol->target.perms.mode,
                             vol->target.perms.uid, vol->target.perms.gid,
+                            VIR_DIR_CREATE_FORCE_PERMS |
                             (pool->def->type == VIR_STORAGE_POOL_NETFS
-                             ? VIR_FILE_CREATE_AS_UID : 0))) != 0) {
+                             ? VIR_DIR_CREATE_AS_UID : 0))) != 0) {
         virReportSystemError(err, _("cannot create path '%s'"),
                              vol->target.path);
         return -1;

src/util/util.c

@@ -1212,15 +1212,15 @@ int virFileExists(const char *path)
 }
 
 
-static int virFileCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid,
-                               unsigned int flags) {
-    int open_flags = O_RDWR | O_CREAT | ((flags & VIR_FILE_CREATE_ALLOW_EXIST)
-                                          ? 0 : O_EXCL);
+static int virFileOperationNoFork(const char *path, int openflags, mode_t mode,
+                                  uid_t uid, gid_t gid,
+                                  virFileOperationHook hook, void *hookdata,
+                                  unsigned int flags) {
     int fd = -1;
     int ret = 0;
     struct stat st;
 
-    if ((fd = open(path, open_flags, mode)) < 0) {
+    if ((fd = open(path, openflags, mode)) < 0) {
         ret = errno;
         virReportSystemError(errno, _("failed to create file '%s'"),
                              path);
@@ -1238,13 +1238,17 @@ static int virFileCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t g
                              path, uid, gid);
         goto error;
     }
-    if (fchmod(fd, mode) < 0) {
+    if ((flags & VIR_FILE_OP_FORCE_PERMS)
+        && (fchmod(fd, mode) < 0)) {
         ret = errno;
         virReportSystemError(errno,
                              _("cannot set mode of '%s' to %04o"),
                              path, mode);
         goto error;
     }
+    if ((hook) && ((ret = hook(fd, hookdata)) != 0)) {
+        goto error;
+    }
     if (close(fd) < 0) {
         ret = errno;
         virReportSystemError(errno, _("failed to close new file '%s'"),
@@ -1259,13 +1263,13 @@ error:
     return ret;
 }
 
-static int virDirCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gid,
+static int virDirCreateNoFork(const char *path, mode_t mode, uid_t uid, gid_t gid,
                               unsigned int flags) {
     int ret = 0;
     struct stat st;
 
     if ((mkdir(path, mode) < 0)
-        && !((errno == EEXIST) && (flags & VIR_FILE_CREATE_ALLOW_EXIST)))
+        && !((errno == EEXIST) && (flags & VIR_DIR_CREATE_ALLOW_EXIST)))
        {
         ret = errno;
         virReportSystemError(errno, _("failed to create directory '%s'"),
@@ -1285,7 +1289,8 @@ static int virDirCreateSimple(const char *path, mode_t mode, uid_t uid, gid_t gi
                              path, uid, gid);
         goto error;
     }
-    if (chmod(path, mode) < 0) {
+    if ((flags & VIR_DIR_CREATE_FORCE_PERMS)
+        && (chmod(path, mode) < 0)) {
         ret = errno;
         virReportSystemError(errno,
                              _("cannot set mode of '%s' to %04o"),
@@ -1297,18 +1302,20 @@ error:
 }
 
 #ifndef WIN32
-int virFileCreate(const char *path, mode_t mode,
-                  uid_t uid, gid_t gid, unsigned int flags) {
+int virFileOperation(const char *path, int openflags, mode_t mode,
+                     uid_t uid, gid_t gid,
+                     virFileOperationHook hook, void *hookdata,
+                     unsigned int flags) {
     struct stat st;
     pid_t pid;
     int waitret, status, ret = 0;
     int fd;
 
-    if ((!(flags & VIR_FILE_CREATE_AS_UID))
+    if ((!(flags & VIR_FILE_OP_AS_UID))
         || (getuid() != 0)
-        || ((uid == 0) && (gid == 0))
-        || ((flags & VIR_FILE_CREATE_ALLOW_EXIST) && (stat(path, &st) >= 0))) {
-        return virFileCreateSimple(path, mode, uid, gid, flags);
+        || ((uid == 0) && (gid == 0))) {
+        return virFileOperationNoFork(path, openflags, mode, uid, gid,
+                                      hook, hookdata, flags);
     }
 
     /* parent is running as root, but caller requested that the
@@ -1338,34 +1345,8 @@ int virFileCreate(const char *path, mode_t mode,
         if (!WIFEXITED(status) || (ret == EACCES)) {
             /* fall back to the simpler method, which works better in
              * some cases */
-            return virFileCreateSimple(path, mode, uid, gid, flags);
-        }
-        if (ret != 0) {
-            goto parenterror;
-        }
-
-        /* check if group was set properly by creating after
-         * setgid. If not, try doing it with chown */
-        if (stat(path, &st) == -1) {
-            ret = errno;
-            virReportSystemError(errno,
-                                 _("stat of '%s' failed"),
-                                 path);
-            goto parenterror;
-        }
-        if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) {
-            ret = errno;
-            virReportSystemError(errno,
-                                 _("cannot chown '%s' to group %u"),
-                                 path, gid);
-            goto parenterror;
-        }
-        if (chmod(path, mode) < 0) {
-            ret = errno;
-            virReportSystemError(errno,
-                                 _("cannot set mode of '%s' to %04o"),
-                                 path, mode);
-            goto parenterror;
+            return virFileOperationNoFork(path, openflags, mode, uid, gid,
+                                          hook, hookdata, flags);
         }
 parenterror:
         return ret;
@@ -1395,7 +1376,7 @@ parenterror:
                              uid, path);
         goto childerror;
     }
-    if ((fd = open(path, O_RDWR | O_CREAT | O_EXCL, mode)) < 0) {
+    if ((fd = open(path, openflags, mode)) < 0) {
         ret = errno;
         if (ret != EACCES) {
             /* in case of EACCES, the parent will retry */
@@ -1405,6 +1386,29 @@ parenterror:
         }
         goto childerror;
     }
+    if (fstat(fd, &st) == -1) {
+        ret = errno;
+        virReportSystemError(errno, _("stat of '%s' failed"), path);
+        goto childerror;
+    }
+    if ((st.st_gid != gid)
+        && (fchown(fd, -1, gid) < 0)) {
+        ret = errno;
+        virReportSystemError(errno, _("cannot chown '%s' to (%u, %u)"),
+                             path, uid, gid);
+        goto childerror;
+    }
+    if ((flags & VIR_FILE_OP_FORCE_PERMS)
+        && (fchmod(fd, mode) < 0)) {
+        ret = errno;
+        virReportSystemError(errno,
+                             _("cannot set mode of '%s' to %04o"),
+                             path, mode);
+        goto childerror;
+    }
+    if ((hook) && ((ret = hook(fd, hookdata)) != 0)) {
+        goto childerror;
+    }
     if (close(fd) < 0) {
         ret = errno;
         virReportSystemError(errno, _("child failed to close new file '%s'"),
@@ -1423,11 +1427,11 @@ int virDirCreate(const char *path, mode_t mode,
     int waitret;
     int status, ret = 0;
 
-    if ((!(flags & VIR_FILE_CREATE_AS_UID))
+    if ((!(flags & VIR_DIR_CREATE_AS_UID))
         || (getuid() != 0)
         || ((uid == 0) && (gid == 0))
-        || ((flags & VIR_FILE_CREATE_ALLOW_EXIST) && (stat(path, &st) >= 0))) {
-        return virDirCreateSimple(path, mode, uid, gid, flags);
+        || ((flags & VIR_DIR_CREATE_ALLOW_EXIST) && (stat(path, &st) >= 0))) {
+        return virDirCreateNoFork(path, mode, uid, gid, flags);
     }
 
     int forkRet = virFork(&pid);
@@ -1451,34 +1455,11 @@ int virDirCreate(const char *path, mode_t mode,
         if (!WIFEXITED(status) || (ret == EACCES)) {
             /* fall back to the simpler method, which works better in
              * some cases */
-            return virDirCreateSimple(path, mode, uid, gid, flags);
+            return virDirCreateNoFork(path, mode, uid, gid, flags);
         }
         if (ret != 0) {
             goto parenterror;
         }
-
-        /* check if group was set properly by creating after
-         * setgid. If not, try doing it with chown */
-        if (stat(path, &st) == -1) {
-            ret = errno;
-            virReportSystemError(errno,
-                                 _("stat of '%s' failed"),
-                                 path);
-            goto parenterror;
-        }
-        if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) {
-            ret = errno;
-            virReportSystemError(errno,
-                                 _("cannot chown '%s' to group %u"),
-                                 path, gid);
-            goto parenterror;
-        }
-        if (chmod(path, mode) < 0) {
-            virReportSystemError(errno,
-                                 _("cannot set mode of '%s' to %04o"),
-                                 path, mode);
-            goto parenterror;
-        }
 parenterror:
         return ret;
     }
@@ -1513,20 +1494,45 @@ parenterror:
         }
         goto childerror;
     }
+    /* check if group was set properly by creating after
+     * setgid. If not, try doing it with chown */
+    if (stat(path, &st) == -1) {
+        ret = errno;
+        virReportSystemError(errno,
+                             _("stat of '%s' failed"), path);
+        goto childerror;
+    }
+    if ((st.st_gid != gid) && (chown(path, -1, gid) < 0)) {
+        ret = errno;
+        virReportSystemError(errno,
+                             _("cannot chown '%s' to group %u"),
+                             path, gid);
+        goto childerror;
+    }
+    if ((flags & VIR_DIR_CREATE_FORCE_PERMS)
+        && chmod(path, mode) < 0) {
+        virReportSystemError(errno,
+                             _("cannot set mode of '%s' to %04o"),
+                             path, mode);
+        goto childerror;
+    }
 childerror:
     _exit(ret);
 }
 
 #else /* WIN32 */
 
-int virFileCreate(const char *path, mode_t mode,
-                  uid_t uid, gid_t gid, unsigned int flags) {
-    return virFileCreateSimple(path, mode, uid, gid, flags);
+int virFileOperation(const char *path, int openflags, mode_t mode,
+                  uid_t uid, gid_t gid,
+                  virFileOperationHook hook, void *hookdata,
+                  unsigned int flags) {
+    return virFileOperationNoFork(path, openflags, mode, uid, gid,
+                                  hook, hookdata, flags);
 }
 
 int virDirCreate(const char *path, mode_t mode,
                  uid_t uid, gid_t gid, unsigned int flags) {
-    return virDirCreateSimple(path, mode, uid, gid, flags);
+    return virDirCreateNoFork(path, mode, uid, gid, flags);
 }
 #endif
 

src/util/util.h

@@ -110,13 +110,22 @@ char *virFindFileInPath(const char *file);
 int virFileExists(const char *path);
 
 enum {
-    VIR_FILE_CREATE_NONE        = 0,
-    VIR_FILE_CREATE_AS_UID      = (1 << 0),
-    VIR_FILE_CREATE_ALLOW_EXIST = (1 << 1),
+    VIR_FILE_OP_NONE        = 0,
+    VIR_FILE_OP_AS_UID      = (1 << 0),
+    VIR_FILE_OP_FORCE_PERMS = (1 << 1),
 };
+typedef int (*virFileOperationHook)(int fd, void *data);
+int virFileOperation(const char *path, int openflags, mode_t mode,
+                     uid_t uid, gid_t gid,
+                     virFileOperationHook hook, void *hookdata,
+                     unsigned int flags) ATTRIBUTE_RETURN_CHECK;
 
-int virFileCreate(const char *path, mode_t mode, uid_t uid, gid_t gid,
-                  unsigned int flags) ATTRIBUTE_RETURN_CHECK;
+enum {
+    VIR_DIR_CREATE_NONE        = 0,
+    VIR_DIR_CREATE_AS_UID      = (1 << 0),
+    VIR_DIR_CREATE_FORCE_PERMS = (1 << 1),
+    VIR_DIR_CREATE_ALLOW_EXIST = (1 << 2),
+};
 int virDirCreate(const char *path, mode_t mode, uid_t uid, gid_t gid,
                  unsigned int flags) ATTRIBUTE_RETURN_CHECK;
 int virFileMakePath(const char *path) ATTRIBUTE_RETURN_CHECK;