On 03/09/2011 08:45 PM, Eric Blake wrote:
This patch intentionally doesn't change indentation, in order to
make it easier to review the real changes.

* src/util/util.h (VIR_FILE_OP_RETURN_FD, virFileOperationHook):
Delete.
(virFileOperation): Rename...
(virFileOpenAs): ...and reduce parameters.
* src/util/util.c (virFileOperationNoFork, virFileOperation):
Rename and simplify.
* src/qemu/qemu_driver.c (qemudDomainSaveFlag): Adjust caller.
* src/storage/storage_backend.c (virStorageBackendCreateRaw):
Likewise.
* src/libvirt_private.syms: Reflect rename.
---
  src/libvirt_private.syms      |    2 +-
  src/qemu/qemu_driver.c        |   20 +++-----
  src/storage/storage_backend.c |   12 ++--
  src/util/util.c               |  103 +++++++++++++----------------------------
  src/util/util.h               |   15 ++----
  5 files changed, 53 insertions(+), 99 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index c0da78e..a9f7e39 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -876,8 +876,8 @@ virFileIsExecutable;
  virFileLinkPointsTo;
  virFileMakePath;
  virFileMatchesNameSuffix;
+virFileOpenAs;
  virFileOpenTty;
-virFileOperation;
  virFilePid;
  virFileReadAll;
  virFileReadLimFD;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index a73c5b9..06bc969 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1880,11 +1880,9 @@ static int qemudDomainSaveFlag(struct qemud_driver 
*driver, virDomainPtr dom,
              goto endjob;
          }
      } else {
-        if ((fd = virFileOperation(path, O_CREAT|O_TRUNC|O_WRONLY,
-                                   S_IRUSR|S_IWUSR,
-                                   getuid(), getgid(),
-                                   NULL, NULL,
-                                   VIR_FILE_OP_RETURN_FD))<  0) {
+        if ((fd = virFileOpenAs(path, O_CREAT|O_TRUNC|O_WRONLY,
+                                S_IRUSR|S_IWUSR,
+                                getuid(), getgid(), 0))<  0) {
              /* If we failed as root, and the error was permission-denied
                 (EACCES or EPERM), assume it's on a network-connected share
                 where root access is restricted (eg, root-squashed NFS). If the
@@ -1918,7 +1916,7 @@ static int qemudDomainSaveFlag(struct qemud_driver 
*driver, virDomainPtr dom,

                  case 0:
                  default:
-                   /* local file - log the error returned by virFileOperation 
*/
+                   /* local file - log the error returned by virFileOpenAs */
                     virReportSystemError(-rc,
                                      _("Failed to create domain save file 
'%s'"),
                                          path);
@@ -1929,12 +1927,10 @@ static int qemudDomainSaveFlag(struct qemud_driver 
*driver, virDomainPtr dom,

              /* Retry creating the file as driver->user */

-            if ((fd = virFileOperation(path, O_CREAT|O_TRUNC|O_WRONLY,
-                                       S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP,
-                                       driver->user, driver->group,
-                                       NULL, NULL,
-                                       (VIR_FILE_OP_AS_UID |
-                                        VIR_FILE_OP_RETURN_FD)))<  0) {
+            if ((fd = virFileOpenAs(path, O_CREAT|O_TRUNC|O_WRONLY,
+                                    S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP,
+                                    driver->user, driver->group,
+                                    VIR_FILE_OPEN_AS_UID))<  0) {
                  virReportSystemError(-fd,
                                     _("Error from child process creating 
'%s'"),
                                       path);
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index c7c5ccd..7a3a2b8 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -364,14 +364,14 @@ virStorageBackendCreateRaw(virConnectPtr conn 
ATTRIBUTE_UNUSED,

      uid = (vol->target.perms.uid == -1) ? getuid() : vol->target.perms.uid;
      gid = (vol->target.perms.gid == -1) ? getgid() : vol->target.perms.gid;
-    operation_flags = VIR_FILE_OP_FORCE_PERMS | VIR_FILE_OP_RETURN_FD;
+    operation_flags = VIR_FILE_OPEN_FORCE_PERMS;
      if (pool->def->type == VIR_STORAGE_POOL_NETFS)
-        operation_flags |= VIR_FILE_OP_AS_UID;
+        operation_flags |= VIR_FILE_OPEN_AS_UID;

-    if ((fd = virFileOperation(vol->target.path,
-                               O_RDWR | O_CREAT | O_EXCL,
-                               vol->target.perms.mode, uid, gid,
-                               NULL, NULL, operation_flags))<  0) {
+    if ((fd = virFileOpenAs(vol->target.path,
+                            O_RDWR | O_CREAT | O_EXCL,
+                            vol->target.perms.mode, uid, gid,
+                            operation_flags))<  0) {
          virReportSystemError(-fd,
                               _("cannot create path '%s'"),
                               vol->target.path);
diff --git a/src/util/util.c b/src/util/util.c
index 6bf3219..137cdb9 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -1355,10 +1355,10 @@ virFileIsExecutable(const char *file)

  #ifndef WIN32
  /* return -errno on failure, or 0 on success */
-static int virFileOperationNoFork(const char *path, int openflags, mode_t mode,
-                                  uid_t uid, gid_t gid,
-                                  virFileOperationHook hook, void *hookdata,
-                                  unsigned int flags) {
+static int
+virFileOpenAsNoFork(const char *path, int openflags, mode_t mode,
+                    uid_t uid, gid_t gid, unsigned int flags)
+{
      int fd = -1;
      int ret = 0;
      struct stat st;
@@ -1381,7 +1381,7 @@ static int virFileOperationNoFork(const char *path, int 
openflags, mode_t mode,
                               path, (unsigned int) uid, (unsigned int) gid);
          goto error;
      }
-    if ((flags&  VIR_FILE_OP_FORCE_PERMS)
+    if ((flags&  VIR_FILE_OPEN_FORCE_PERMS)
          &&  (fchmod(fd, mode)<  0)) {
          ret = -errno;
          virReportSystemError(errno,
@@ -1389,17 +1389,7 @@ static int virFileOperationNoFork(const char *path, int 
openflags, mode_t mode,
                               path, mode);
          goto error;
      }
-    if ((hook)&&  ((ret = hook(fd, hookdata)) != 0)) {
-        goto error;
-    }
-    if (flags&  VIR_FILE_OP_RETURN_FD)
-        return fd;
-    if (VIR_CLOSE(fd)<  0) {
-        ret = -errno;
-        virReportSystemError(errno, _("failed to close new file '%s'"),
-                             path);
-        goto error;
-    }
+    return fd;

  error:
      VIR_FORCE_CLOSE(fd);
@@ -1446,35 +1436,27 @@ error:
  }

  /**
- * virFileOperation:
+ * virFileOpenAs:
   * @path: file to open or create
   * @openflags: flags to pass to open
   * @mode: mode to use on creation or when forcing permissions
   * @uid: uid that should own file on creation
   * @gid: gid that should own file
- * @hook: callback to run once file is open; see below for restrictions
- * @hookdata: opaque data for @hook
- * @flags: bit-wise or of VIR_FILE_OP_* flags
+ * @flags: bit-wise or of VIR_FILE_OPEN_* flags
   *
   * Open @path, and execute an optional callback @hook on the open file
   * description.  @hook must return 0 on success, or -errno on failure.
- * If @flags includes VIR_FILE_OP_AS_UID, then open the file while the
+ * If @flags includes VIR_FILE_OPEN_AS_UID, then open the file while the
   * effective user id is @uid (by using a child process); this allows
   * one to bypass root-squashing NFS issues.  If @flags includes
- * VIR_FILE_OP_FORCE_PERMS, then ensure that @path has those
+ * VIR_FILE_OPEN_FORCE_PERMS, then ensure that @path has those
   * permissions before the callback, even if the file already existed
- * with different permissions.  If @flags includes
- * VIR_FILE_OP_RETURN_FD, then use SCM_RIGHTS to guarantee that any
- * @hook is run in the parent, and the return value (if non-negative)
- * is the file descriptor, left open.  Otherwise, @hook might be run
- * in a child process, so it must be async-signal-safe (ie. no
- * malloc() or anything else that depends on modifying locks held in
- * the parent), no file descriptor remains open, and 0 is returned on
- * success.  Return -errno on failure.  */
-int virFileOperation(const char *path, int openflags, mode_t mode,
-                     uid_t uid, gid_t gid,
-                     virFileOperationHook hook, void *hookdata,
-                     unsigned int flags) {
+ * with different permissions.  The return value (if non-negative)
+ * is the file descriptor, left open.  Return -errno on failure.  */
+int
+virFileOpenAs(const char *path, int openflags, mode_t mode,
+              uid_t uid, gid_t gid, unsigned int flags)
+{
      struct stat st;
      pid_t pid;
      int waitret, status, ret = 0;
@@ -1487,11 +1469,10 @@ int virFileOperation(const char *path, int openflags, 
mode_t mode,
      struct iovec iov;
      int forkRet;

-    if ((!(flags&  VIR_FILE_OP_AS_UID))
+    if ((!(flags&  VIR_FILE_OPEN_AS_UID))
          || (getuid() != 0)
          || ((uid == 0)&&  (gid == 0))) {
-        return virFileOperationNoFork(path, openflags, mode, uid, gid,
-                                      hook, hookdata, flags);
+        return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags);
      }

      /* parent is running as root, but caller requested that the
@@ -1499,7 +1480,7 @@ int virFileOperation(const char *path, int openflags, 
mode_t mode,
       * following dance avoids problems caused by root-squashing
       * NFS servers. */

-    if (flags&  VIR_FILE_OP_RETURN_FD) {
+    {
          if (socketpair(AF_UNIX, SOCK_DGRAM, 0, pair)<  0) {
              ret = -errno;
              virReportSystemError(errno,
@@ -1529,7 +1510,7 @@ int virFileOperation(const char *path, int openflags, 
mode_t mode,
      }

      if (pid) { /* parent */
-        if (flags&  VIR_FILE_OP_RETURN_FD) {
+        {
              VIR_FORCE_CLOSE(pair[1]);

              do {
@@ -1565,20 +1546,13 @@ int virFileOperation(const char *path, int openflags, 
mode_t mode,
              goto parenterror;
          }
          ret = -WEXITSTATUS(status);
-        if (!WIFEXITED(status) || (ret == -EACCES) ||
-            ((flags&  VIR_FILE_OP_RETURN_FD)&&  fd == -1)) {
+        if (!WIFEXITED(status) || (ret == -EACCES) || fd == -1) {
              /* fall back to the simpler method, which works better in
               * some cases */
-            return virFileOperationNoFork(path, openflags, mode, uid, gid,
-                                          hook, hookdata, flags);
+            return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags);
          }
-        if (!ret&&  (flags&  VIR_FILE_OP_RETURN_FD)) {
-            if ((hook)&&  ((ret = hook(fd, hookdata)) != 0)) {
-                VIR_FORCE_CLOSE(fd);
-                goto parenterror;
-            }
+        if (!ret)
              ret = fd;
-        }
  parenterror:
          return ret;
      }
@@ -1630,7 +1604,7 @@ parenterror:
                               path, (unsigned int) uid, (unsigned int) gid);
          goto childerror;
      }
-    if ((flags&  VIR_FILE_OP_FORCE_PERMS)
+    if ((flags&  VIR_FILE_OPEN_FORCE_PERMS)
          &&  (fchmod(fd, mode)<  0)) {
          ret = -errno;
          virReportSystemError(errno,
@@ -1638,7 +1612,7 @@ parenterror:
                               path, mode);
          goto childerror;
      }
-    if (flags&  VIR_FILE_OP_RETURN_FD) {
+    {
          VIR_FORCE_CLOSE(pair[0]);


This VIR_FORCE_CLOSE(pair[0]) should be moved up to the very start of the child code - otherwise any error prior to this point will leave it dangling for _exit() to clean up.


          memcpy(CMSG_DATA(cmsg),&fd, sizeof(fd));

@@ -1651,17 +1625,6 @@ parenterror:
              VIR_FORCE_CLOSE(pair[1]);

A leftover from 05/15 - I think thie VIR_FORCE_CLOSE should happen at/after childerror:. That way it will always get cleaned up, whether or not there is an error at any time during child execution.

              goto childerror;
          }
-        ret = 0;
-    } else {
-        if ((hook)&&  ((ret = hook(fd, hookdata)) != 0))
-            goto childerror;
-        if (VIR_CLOSE(fd)<  0) {
-            ret = -errno;
-            virReportSystemError(errno,
-                                 _("child failed to close new file '%s'"),
-                                 path);
-            goto childerror;
-        }
      }

      ret = 0;
@@ -1783,17 +1746,15 @@ childerror:
  #else /* WIN32 */

  /* return -errno on failure, or 0 on success */
-int virFileOperation(const char *path ATTRIBUTE_UNUSED,
-                     int openflags ATTRIBUTE_UNUSED,
-                     mode_t mode ATTRIBUTE_UNUSED,
-                     uid_t uid ATTRIBUTE_UNUSED,
-                     gid_t gid ATTRIBUTE_UNUSED,
-                     virFileOperationHook hook ATTRIBUTE_UNUSED,
-                     void *hookdata ATTRIBUTE_UNUSED,
-                     unsigned int flags ATTRIBUTE_UNUSED)
+int virFileOpenAs(const char *path ATTRIBUTE_UNUSED,
+                  int openflags ATTRIBUTE_UNUSED,
+                  mode_t mode ATTRIBUTE_UNUSED,
+                  uid_t uid ATTRIBUTE_UNUSED,
+                  gid_t gid ATTRIBUTE_UNUSED,
+                  unsigned int flags ATTRIBUTE_UNUSED)
  {
      virUtilError(VIR_ERR_INTERNAL_ERROR,
-                 "%s", _("virFileOperation is not implemented for WIN32"));
+                 "%s", _("virFileOpenAs is not implemented for WIN32"));

      return -ENOSYS;
  }
diff --git a/src/util/util.h b/src/util/util.h
index 3970d58..cecd348 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -125,16 +125,13 @@ bool virFileIsExecutable(const char *file) 
ATTRIBUTE_NONNULL(1);
  char *virFileSanitizePath(const char *path);

  enum {
-    VIR_FILE_OP_NONE        = 0,
-    VIR_FILE_OP_AS_UID      = (1<<  0),
-    VIR_FILE_OP_FORCE_PERMS = (1<<  1),
-    VIR_FILE_OP_RETURN_FD   = (1<<  2),
+    VIR_FILE_OPEN_NONE        = 0,
+    VIR_FILE_OPEN_AS_UID      = (1<<  0),
+    VIR_FILE_OPEN_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)
+int virFileOpenAs(const char *path, int openflags, mode_t mode,
+                  uid_t uid, gid_t gid,
+                  unsigned int flags)
      ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK;

  enum {

Aside from the two problems with the locations of VIR_FORCE_CLOSE(pair[n]), this all looks fine, and hasn't broken either backing store or save images on root-squash NFS, so ACK with those two changes.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Reply via email to