[libvirt] [PATCH v2 0/3] Fix and tweak 'uknown cause' error in virDirCreate
Erik Skultety (3): util: virfile: Fix 'unknown cause' error if NFS mount point creation fails util: virDirCreate: Child now exits with positive errno-code virfile: virDirCreate: Insert blank lines to assure slightly better readability src/util/virfile.c | 44 +++++++++++++++++++++++++++++++++++--------- 1 file changed, 35 insertions(+), 9 deletions(-) -- 1.9.3
This happens if user requires creation of a directory with specified UID/GID permissions. To accomplish this, we use fork approach and set particular UID/GID permissions in child process. However, child process doesn't have a valid descriptor to a logfile (this is prohibited explicitly) and since parent process doesn't handle negative exit codes from child in any way, 'uknown cause' error is returned to the user. Commit 92d9114e tweaked the way we handle child errors when using fork approach to set specific permissions (features originally introduced by 98f6f381). The same logic should be used to create directories with specified permissions as well. https://bugzilla.redhat.com/show_bug.cgi?id=1230137 --- src/util/virfile.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 5ff4668..5e678fe 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2384,11 +2384,27 @@ 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 + * introduced and explained by commit 98f6f381. + */ + 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); + if (WIFEXITED(status)) + ret = -WEXITSTATUS(status); + else + ret = -EACCES; } + parenterror: return ret; } -- 1.9.3
Previous patch of this series proposed a fix to virDirCreate, so that parent process reports an error if child process failed its task. However our logic still permits the child to exit with negative errno followed by a check of the status on the parent side using WEXITSTATUS which, being POSIX compliant, takes the lower 8 bits of the exit code and returns is to the caller. However, by taking 8 bits from a negative exit code (two's complement) the status value we read and append to stream is '2^8 - abs(original exit code)' which doesn't quite reflect the real cause when compared to the meaning of errno values. --- src/util/virfile.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 5e678fe..91e460f 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -2413,12 +2413,12 @@ virDirCreate(const char *path, /* set desired uid/gid, then attempt to create the directory */ if (virSetUIDGID(uid, gid, groups, ngroups) < 0) { - ret = -errno; + ret = errno; goto childerror; } if (mkdir(path, mode) < 0) { - ret = -errno; - if (ret != -EACCES) { + ret = errno; + if (ret != EACCES) { /* in case of EACCES, the parent will retry */ virReportSystemError(errno, _("child failed to create directory '%s'"), path); @@ -2428,13 +2428,13 @@ virDirCreate(const char *path, /* check if group was set properly by creating after * setgid. If not, try doing it with chown */ if (stat(path, &st) == -1) { - ret = -errno; + ret = errno; virReportSystemError(errno, _("stat of '%s' failed"), path); goto childerror; } if ((st.st_gid != gid) && (chown(path, (uid_t) -1, gid) < 0)) { - ret = -errno; + ret = errno; virReportSystemError(errno, _("cannot chown '%s' to group %u"), path, (unsigned int) gid); @@ -2447,6 +2447,10 @@ virDirCreate(const char *path, goto childerror; } childerror: + if ((ret & 0xff) != ret) { + VIR_WARN("unable to pass desired return value %d", ret); + ret = 0xff; + } _exit(ret); } -- 1.9.3
--- This could easily be pushed under trivial rule, right? src/util/virfile.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/util/virfile.c b/src/util/virfile.c index 91e460f..61f6e4d 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; @@ -2416,6 +2417,7 @@ virDirCreate(const char *path, ret = errno; goto childerror; } + if (mkdir(path, mode) < 0) { ret = errno; if (ret != EACCES) { @@ -2425,6 +2427,7 @@ virDirCreate(const char *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) { @@ -2433,6 +2436,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, @@ -2440,12 +2444,14 @@ 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: if ((ret & 0xff) != ret) { VIR_WARN("unable to pass desired return value %d", ret); -- 1.9.3
On Tue, Jun 16, 2015 at 02:28:52PM +0200, Erik Skultety wrote:
Erik Skultety (3): util: virfile: Fix 'unknown cause' error if NFS mount point creation fails util: virDirCreate: Child now exits with positive errno-code virfile: virDirCreate: Insert blank lines to assure slightly better readability
ACK series if you exchange the first two patches. Both the original logic assume the exit status to be +errno, not -errno, so that change should be first. Jan
On 06/16/2015 04:23 PM, Ján Tomko wrote:
On Tue, Jun 16, 2015 at 02:28:52PM +0200, Erik Skultety wrote:
Erik Skultety (3): util: virfile: Fix 'unknown cause' error if NFS mount point creation fails util: virDirCreate: Child now exits with positive errno-code virfile: virDirCreate: Insert blank lines to assure slightly better readability
ACK series if you exchange the first two patches. Both the original logic assume the exit status to be +errno, not -errno, so that change should be first.
Jan
Thank you, patches are now pushed. Erik
participants (2)
-
Erik Skultety -
Ján Tomko