[libvirt] [PATCHv4 1/2] util: refactor virFileOpenAs

Laine Stump laine at laine.org
Fri Feb 3 21:49:29 UTC 2012


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!




More information about the libvir-list mailing list