[libvirt] [PATCH] virfile: Report useful error fork approach to create NFS mount point fails

[Erik Skultety]

Commit 92d9114e tweaked the way we handle child errors when using fork
approach to set specific permissions. The same logic should be used to
create directories with specified permissions as well, otherwise the parent
process doesn't report any useful error "unknown cause" while still returning
negative errcode.

https://bugzilla.redhat.com/show_bug.cgi?id=1230137
---
 src/util/virfile.c | 48 +++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 39 insertions(+), 9 deletions(-)

diff --git a/src/util/virfile.c b/src/util/virfile.c
index 5ff4668..7675eeb 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -2376,6 +2376,7 @@ virDirCreate(const char *path,
     if (pid) { /* parent */
         /* wait for child to complete, and retrieve its exit code */
         VIR_FREE(groups);
+
         while ((waitret = waitpid(pid, &status, 0)) == -1 && errno == EINTR);
         if (waitret == -1) {
             ret = -errno;
@@ -2384,11 +2385,33 @@ virDirCreate(const char *path,
                                  path);
             goto parenterror;
         }
-        if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES) {
-            /* fall back to the simpler method, which works better in
-             * some cases */
-            return virDirCreateNoFork(path, mode, uid, gid, flags);
+
+        /*
+         * If waitpid succeeded, but if the child exited abnormally or
+         * reported non-zero status, report failure, except for EACCES where
+         * we try to fall back to non-fork method as in the original logic.
+         */
+        if (!WIFEXITED(status) || (WEXITSTATUS(status)) != 0) {
+            if (WEXITSTATUS(status) == EACCES)
+                return virDirCreateNoFork(path, mode, uid, gid, flags);
+            char *msg = virProcessTranslateStatus(status);
+            virReportError(VIR_ERR_INTERNAL_ERROR,
+                           _("child failed to create '%s': %s"),
+                           path, msg);
+            VIR_FREE(msg);
+            /* Use child exit status if possible; otherwise,
+             * just use -EACCES, since by our original failure in
+             * the non fork+setuid path would have been EACCES or
+             * EPERM by definition (see qemuOpenFileAs after the
+             * first virFileOpenAs failure), but EACCES is close enough.
+             * Besides -EPERM is like returning fd == -1.
+             */
+            if (WIFEXITED(status))
+                ret = -WEXITSTATUS(status);
+            else
+                ret = -EACCES;
         }
+
  parenterror:
         return ret;
     }
@@ -2400,15 +2423,14 @@ virDirCreate(const char *path,
         ret = -errno;
         goto childerror;
     }
+
     if (mkdir(path, mode) < 0) {
         ret = -errno;
-        if (ret != -EACCES) {
-            /* in case of EACCES, the parent will retry */
-            virReportSystemError(errno, _("child failed to create directory '%s'"),
-                                 path);
-        }
+        virReportSystemError(errno, _("child failed to create directory '%s'"),
+                             path);
         goto childerror;
     }
+
     /* check if group was set properly by creating after
      * setgid. If not, try doing it with chown */
     if (stat(path, &st) == -1) {
@@ -2417,6 +2439,7 @@ virDirCreate(const char *path,
                              _("stat of '%s' failed"), path);
         goto childerror;
     }
+
     if ((st.st_gid != gid) && (chown(path, (uid_t) -1, gid) < 0)) {
         ret = -errno;
         virReportSystemError(errno,
@@ -2424,13 +2447,20 @@ virDirCreate(const char *path,
                              path, (unsigned int) gid);
         goto childerror;
     }
+
     if (mode != (mode_t) -1 && chmod(path, mode) < 0) {
         virReportSystemError(errno,
                              _("cannot set mode of '%s' to %04o"),
                              path, mode);
         goto childerror;
     }
+
  childerror:
+    ret = -ret;
+    if ((ret & 0xff) != ret) {
+        VIR_WARN("unable to pass desired return value %d", ret);
+        ret = 0xff;
+    }
     _exit(ret);
 }
 
-- 
1.9.3