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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list