Re: Splitting <fs>_access to two internal routines

[Elad Efrat <elad%NetBSD.org@localhost>]

David Laight wrote:

> These sort of diffs are difficult to read - since the 'new' code
> and 'old' code that diff compares end up being different functions!

That's intended, I don't want to introduce new functionality, but
rather split it to two functions. I'm not sure how to diff without
having that effect on the outcome. :)

I've attached a file with just the new functions added and the
modified ufs_access, though, to make it easier to read.

> So I can't see what ufs_check_possible() actually looks like.
> However the other parts end up being:
>
> > +ufs_check_permitted(struct vnode *vp, struct inode *ip, mode_t mode,
> > +    kauth_cred_t cred)
> > +{
> > +
> > +       return genfs_can_access(vp->v_type, ip->i_mode & ALLPERMS, ip->i_uid,
> > +           ip->i_gid, mode, cred);
> > +}
> > +
> > +int
> > +ufs_access(void *v)
> > +{
> ...
> > +       error = ufs_check_possible(vp, ip, mode, ap->a_cred);
> > +       if (error)
> > +               return error;
> > +
> > +       return ufs_check_permitted(vp, ip, mode, ap->a_cred);
> >  }
>
> This looks like a complete waste of cpu cycles stacking arguments
> to functions.

The cred to ufs_check_possible() was dropped; other than that, we need
the vnode type, inode flags, and access mode in both, and the
credentials in ufs_check_permitted(). Whether we pass the structures or
just the values is of less concern to me... that's exactly why I posted
it here -- to get feedback on "if we do introduce these functions, what
are the proper prototypes for them?".

Due to lack of consistency across file-systems, we won't be able to make
them all the same, but we can achieve a small about of "versions".
Either way, being private helps a lot to play with the arguments of
choice without affecting anything else.

Thanks,

-e.

static int
ufs_check_possible(struct vnode *vp, struct inode *ip, mode_t mode)
{
#ifdef QUOTA
        int error;
#endif /* QUOTA */

        /*
         * Disallow write attempts on read-only file systems;
         * unless the file is a socket, fifo, or a block or
         * character device resident on the file system.
         */
        if (mode & VWRITE) {
                switch (vp->v_type) {
                case VDIR:
                case VLNK:
                case VREG:
                        if (vp->v_mount->mnt_flag & MNT_RDONLY)
                                return (EROFS);
#ifdef QUOTA
                        fstrans_start(vp->v_mount, FSTRANS_SHARED);
                        error = getinoquota(ip);
                        fstrans_done(vp->v_mount);
                        if (error != 0)
                                return error;
#endif
                        break;
                case VBAD:
                case VBLK:
                case VCHR:
                case VSOCK:
                case VFIFO:
                case VNON:
                default:
                        break;
                }
        }

        /* If it is a snapshot, nobody gets access to it. */
        if ((ip->i_flags & SF_SNAPSHOT))
                return (EPERM);
        /* If immutable bit set, nobody gets to write it. */
        if ((mode & VWRITE) && (ip->i_flags & IMMUTABLE))
                return (EPERM);

        return 0;
}

static int
ufs_check_permitted(struct vnode *vp, struct inode *ip, mode_t mode,
    kauth_cred_t cred)
{

        return genfs_can_access(vp->v_type, ip->i_mode & ALLPERMS, ip->i_uid,
            ip->i_gid, mode, cred);
}

int
ufs_access(void *v)
{
        struct vop_access_args /* {
                struct vnode    *a_vp;
                int             a_mode;
                kauth_cred_t    a_cred;
        } */ *ap = v;
        struct vnode    *vp;
        struct inode    *ip;
        mode_t          mode;
        int             error;

        vp = ap->a_vp;
        ip = VTOI(vp);
        mode = ap->a_mode;

        error = ufs_check_possible(vp, ip, mode);
        if (error)
                return error;

        error = ufs_check_permitted(vp, ip, mode, ap->a_cred);

        return error;
}