[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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

Attachment: signature.asc
Description: OpenPGP digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]