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. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org