Re: [libvirt] [PATCH v5 2/5] util: Create virFileAccessibleAs function
On 10/21/2011 07:12 AM, Michal Privoznik wrote:
This function checks if a given path is accessible under
given uid and gid.
---
src/util/util.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
src/util/util.h | 3 ++
2 files changed, 86 insertions(+), 0 deletions(-)
diff --git a/src/util/util.c b/src/util/util.c
index af27344..7343485 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -671,6 +671,77 @@ virFileIsExecutable(const char *file)
}
#ifndef WIN32
+/* Check that a file is accessible under certain
+ * user& gid.
+ * @mode can be F_OK, or a bitwise combination of R_OK, W_OK, and X_OK.
+ * see 'man access' for more details.
+ * Returns 0 on success, -errno on fail,
+ *>0 on signaled child.
That's awkward. How about just stating:
Returns 0 on success, -1 on fail with errno set.
+ */
+int
+virFileAccessibleAs(const char *path, int mode,
+ uid_t uid, gid_t gid)
+{
+ pid_t pid = 0;
+ int waitret, status, ret = 0;
+ int forkRet = 0;
+ bool do_fork = (uid != getuid() || gid != getgid());
+
+ if (do_fork)
+ forkRet = virFork(&pid);
The control flow is a bit hard to follow. While it looks correct, I
think it would be easier to read as follows (it also matches my proposal
to return -1 with errno set, when encountering error):
if (uid == getuid() && gid == getgid())
return access(path, mode);
forkRet = virFork(&pid);
... code without reference to do_fork ...
+ if (pid) { /* parent */
+ do {
+ if ((waitret = waitpid(pid,&status, 0))< 0) {
+ ret = -errno;
+ virKillProcess(pid, SIGKILL);
+ return ret;
+ }
+ } while (!WIFEXITED(status)&& !WIFSIGNALED(status));
Use virPidWait() (from command.h), not this hand-rolled do/while loop.
In particular, your loop will only ever execute exactly once, since
status will always satisfy either WIFEXITED or WIFSIGNALED if waitpid()
was successful; and you fail to handle EINTR as a reason to retry waitpid.
Returning a positive value for a signaled child is awkward; I would just
treat that as failure, by setting errno to EIO and returning -1. The
likelihood of the child dying by signal is super-slim, not to mention
that I don't see how the client can behave any better by knowing that
the child process died than if it just reacted as if access were denied
in the first place.
@@ -993,6 +1064,18 @@ childerror:
#else /* WIN32 */
+int
+virFileAccessibleAs(const char *path ATTRIBUTE_UNUSED,
+ int mode ATTRIBUTE_UNUSED,
+ uid_t uid ATTRIBUTE_UNUSED,
+ git_t gid ATTRIBUTE_UNUSED)
+{
+ virUtilError(VIR_ERR_INTERNAL_ERROR,
+ "%s", _("virFileAccessibleAs is not implemented for
WIN32"));
+
+ return -ENOSYS;
Rather than failing with -ENOSYS, can we at least try access(path,
mode), ignoring uid and gid? It may have false negatives (fail in cases
where a different uid would succeed), but is more likely to be useful
than just blindly declaring failure.
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org