More duplicate code, vnode locking question

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

Hi,

Another piece of duplicated code in our file-systems is a snippet like
the following, taken from ffs_vfsops.c:

    381         /*
    382          * If mount by non-root, then verify that user has necessary
    383          * permissions on the device.
    384          */
    385         if (error == 0 && kauth_authorize_generic(l->l_cred,
    386             KAUTH_GENERIC_ISSUSER, NULL) != 0) {
    387                 accessmode = VREAD;
    388                 if (update ?
    389                     (mp->mnt_iflag & IMNT_WANTRDWR) != 0 :
    390                     (mp->mnt_flag & MNT_RDONLY) == 0)
    391                         accessmode |= VWRITE;
    392                 vn_lock(devvp, LK_EXCLUSIVE | LK_RETRY);
    393                 error = VOP_ACCESS(devvp, accessmode, l->l_cred);
    394                 VOP_UNLOCK(devvp, 0);
    395         }
        
Here, too, I would like to remove all duplicates and add a single
function to be used from all places.

However, this time vnode locking is involved, and I'm not sure what
the rules are about functions that are not in the vnode interface
(vnode_if?) that lock/unlock vnodes.

In other words, what I have in mind is adding the following:

        int
        common_mount_allowed(kauth_cred_t cred, struct vnode *vp,
            bool vnode_locked, int mode)
        {
                /* if root, always allow. */
                ...

                if (!vnode_locked)
                        vn_lock(vp, LK_EXCLUSIVE | LK_RETRY);

                error = VOP_ACCESS(vp, mode, cred);

                if (!vnode_locked)
                        VOP_UNLOCK(vp, 0);

                return (error);
        }

and replacing the above snippet from ffs_vfsops.c with something like:

        if (error == 0) {
                accessmode = VREAD;
                ...
                /* devvp isn't locked at this point, so pass "false". */
                error = common_mount_allowed(l->l_cred, devvp, false,
                    accessmode);
        }

Is this considered okay? or do we avoid such things?

Also, it seems that some file-systems call VOP_OPEN() without devvp
being locked. Compare, for example, ffs_mount() and efs_mount(). Is
this possible, or am I missing something? :)

Thanks,

-e.