On 02/01/2012 11:36 PM, Laine Stump wrote:
virFileOpenAs previously would only try opening a file as the current
user, or as a different user, but wouldn't try both methods in a
single call. This made it cumbersome to use as a replacement for
open(2). Additionally, it had a lot of historical baggage that led to
it being difficult to understand.
This patch refactors virFileOpenAs in the following ways:
All current consumers of virFileOpenAs should retain their present
behavior (after a few minor changes to their setup code and
arguments).
Yep, sure looks like it.
---
src/libxl/libxl_driver.c | 4 +-
src/qemu/qemu_driver.c | 8 +-
src/storage/storage_backend.c | 12 +-
src/util/util.c | 352 +++++++++++++++++++++++++++--------------
src/util/util.h | 6 +-
5 files changed, 246 insertions(+), 136 deletions(-)
I think we've hit a winner for refactoring it into something that can be
further modified while investigating those modifications, making it more
powerful for new uses, and without breaking behavior in the refactor.
+/* virFileOpenForceOwnerMode() - an internal utility function called
+ * only by virFileOpenAs(). Sets the owner and mode of the file
+ * opened as "fd" if it's not correct AND the flags say it should be
+ * forced. */
static int
-virFileOpenAsNoFork(const char *path, int openflags, mode_t mode,
- uid_t uid, gid_t gid, unsigned int flags)
+virFileOpenForceOwnerMode(const char *path, int fd, mode_t mode,
+ uid_t uid, gid_t gid, unsigned int flags)
{
- if ((flags& VIR_DIR_CREATE_FORCE_PERMS)
-&& (chmod(path, mode)< 0)) {
+ if ((flags& VIR_FILE_OPEN_FORCE_MODE)&&
+ ((mode& (S_IRWXU|S_IRWXG|S_IRWXO)) !=
+ (st.st_mode& (S_IRWXU|S_IRWXG|S_IRWXO)))&&
Technically, this limits us to a mask of 00777,
+ (fchmod(fd, mode)< 0)) {
ret = -errno;
virReportSystemError(errno,
_("cannot set mode of '%s' to %04o"),
while this error message implied that we could request a mode with mask
07777. But since none of the callers are passing sticky bits, I think
we can adjust that later if we ever find a reason that we need bits from
07000 modified, and leave well enough alone in this patch.
+/* virFileOpenForked() - an internal utility function called only by
+ * virFileOpenAs(). It forks, then the child does setuid+setgid to
+ * given uid:gid and attempts to open the file, while the parent just
+ * calls recvfd to get the open fd back from the child. returns the
+ * fd, or -errno if there is an error. */
+static int
+virFileOpenForked(const char *path, int openflags, mode_t mode,
+ uid_t uid, gid_t gid, unsigned int flags)
{
pid_t pid;
forkRet = virFork(&pid);
-
- if (pid< 0) {
- ret = -errno;
- return ret;
- }
+ if (pid< 0)
+ return -errno;
if (pid) { /* parent */
You know, the diff for this patch is so crazy already that we might as
well make maintenance slightly easier. That is, right now, we have:
fork
if fail
return error
if parent {
wait for child
do stuff, which has several return calls
return
}
child
do stuff, which might go to childerror
childerr:
_exit
But sequentially, since the parent is waiting on the child, it might be
easier to read as:
fork
if child {
do stuff, which might go to childerror
childerror:
_exit
}
parent
if fork failed, go to cleanup
wait for child
do more stuff, which might go to cleanup
cleanup:
return
In other words, if you want to rearrange this section of code, now might
be a good time to do it, and I wouldn't even mind if you squashed it in
to this one rather than using a separate patch. But that's all
cosmetic, it wouldn't change the code you actually have.
if (!WIFEXITED(status) || (ret = -WEXITSTATUS(status)) == -EACCES ||
fd == -1) {
/* fall back to the simpler method, which works better in
* some cases */
VIR_FORCE_CLOSE(fd);
- flags&= ~VIR_FILE_OPEN_AS_UID;
- return virFileOpenAsNoFork(path, openflags, mode, uid, gid, flags);
+ if (flags& VIR_FILE_OPEN_NOFORK) {
+ /* If we had already tried opening w/o fork+setuid and
+ * failed, no sense trying again. Just set return the
+ * original errno that we got at that time (by
+ * definition, always either EACCES or EPERM - EACCES
+ * is close enough).
+ */
+ return -EACCES;
Fair enough. I don't think the slight loss in information will hurt;
the end result is still a reasonable failure message.
ACK.