On Thu, Jun 11, 2015 at 11:15:17AM +0200, Erik Skultety wrote:

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) {

The old condition was wrong: child would call _exit(-EACCESS) and we would the compare -(-EACCESS) to -EACCESS

- /* 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. + */

What is the original logic referenced here?

+ 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.

This comment references irrelevant functions and seems redundant.

+ * 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);

Do we really need this hunk? If we fail with EACCES, parent should call the NoFork function which should return success / report error.

goto childerror; } +

The space ajdustments would be better in a separate patch.

/* 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; + }

And flipping the return value to unsigned should be separate from adding the new error message in the parent. Jan