3:49 p.m.
On 02/03/2012 03:41 PM, Eric Blake wrote:
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.
reordered and pushed. Thanks!