7:28 a.m.
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
7:28 a.m.
New subject: [libvirt] [PATCH v2 1/3] util: virfile: Fix 'unknown cause' error if NFS mount point creation fails
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
7:28 a.m.
New subject: [libvirt] [PATCH v2 2/3] util: virDirCreate: Child now exits with positive errno-code
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
7:28 a.m.
New subject: [libvirt] [PATCH v2 3/3] virfile: virDirCreate: Insert blank lines to assure slightly better readability
---
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
9:23 a.m.
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
9:30 a.m.
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
3448
days inactive
3448
days old
5 comments
2 participants
participants (2)
-
Erik Skultety -
Ján Tomko