3 Feb
2012
3 Feb
'12
9: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!