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

[Ján Tomko]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150615/fa2c4f07/attachment-0001.sig>