From owner-freebsd-current@FreeBSD.ORG Wed Jan 12 10:27:51 2005 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 37C9916A4CE for ; Wed, 12 Jan 2005 10:27:51 +0000 (GMT) Received: from critter.freebsd.dk (f170.freebsd.dk [212.242.86.170]) by mx1.FreeBSD.org (Postfix) with ESMTP id 310DF43D5C for ; Wed, 12 Jan 2005 10:27:50 +0000 (GMT) (envelope-from phk@critter.freebsd.dk) Received: from critter.freebsd.dk (localhost [127.0.0.1]) by critter.freebsd.dk (8.13.1/8.13.1) with ESMTP id j0CARns0035192 for ; Wed, 12 Jan 2005 11:27:49 +0100 (CET) (envelope-from phk@critter.freebsd.dk) To: current@freebsd.org From: Poul-Henning Kamp Date: Wed, 12 Jan 2005 11:27:48 +0100 Message-ID: <35191.1105525668@critter.freebsd.dk> Sender: phk@critter.freebsd.dk Subject: [TEST/REVIEW] vnode_if* patch X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 12 Jan 2005 10:27:51 -0000 This patch (from perforce, use "cd /sys ; patch -p3 < foo") changes the generated vnode interface to improve test-coverage and type checking. Please review and test. Changes: Generate non-inlined VOP_FOO_AP() functions which take an argument structure pointer. Put all KASSERTS and checks into VOP_FOO_AP(). Add check for correct argument type. VOP_FOO() is still inline, but now only packs up the arguments and calls the corresponding VOP_FOO_AP() function. Put a pointer to the VOP_FOO_AP() function in the vnode description structure for VCALL() to use. In deadfs and unionfs use VOP_FOO_AP() instead of VCALL(). Remove now unused vdesc_offset and VOFFSET(). diff -ur -x compile -x _* freebsd/src/sys/fs/deadfs/dead_vnops.c phk/phk_dev/sys/fs/deadfs/dead_vnops.c --- freebsd/src/sys/fs/deadfs/dead_vnops.c Thu Jan 6 21:43:36 2005 +++ phk/phk_dev/sys/fs/deadfs/dead_vnops.c Wed Jan 12 10:57:13 2005 @@ -176,7 +176,7 @@ if (!chkvnlock(ap->a_vp)) return (ENOTTY); /* XXX: Doesn't this just recurse back here ? */ - return (VCALL(ap->a_vp, VOFFSET(vop_ioctl), ap)); + return (VOP_IOCTL_AP(ap)); } @@ -203,7 +203,7 @@ } if (!chkvnlock(vp)) return (0); - return (VCALL(vp, VOFFSET(vop_lock), ap)); + return (VOP_LOCK_AP(ap)); } /* diff -ur -x compile -x _* freebsd/src/sys/fs/nullfs/null_vnops.c phk/phk_dev/sys/fs/nullfs/null_vnops.c --- freebsd/src/sys/fs/nullfs/null_vnops.c Mon Jan 10 16:42:38 2005 +++ phk/phk_dev/sys/fs/nullfs/null_vnops.c Wed Jan 12 10:57:13 2005 @@ -296,7 +296,7 @@ * with the modified argument structure. */ if (vps_p[0] && *vps_p[0]) - error = VCALL(*(vps_p[0]), descp->vdesc_offset, ap); + error = VCALL(ap); else { printf("null_bypass: no map for %s\n", descp->vdesc_name); error = EINVAL; diff -ur -x compile -x _* freebsd/src/sys/fs/umapfs/umap_vnops.c phk/phk_dev/sys/fs/umapfs/umap_vnops.c --- freebsd/src/sys/fs/umapfs/umap_vnops.c Thu Jan 6 21:43:38 2005 +++ phk/phk_dev/sys/fs/umapfs/umap_vnops.c Wed Jan 12 10:57:13 2005 @@ -197,7 +197,7 @@ * Call the operation on the lower layer * with the modified argument structure. */ - error = VCALL(*(vps_p[0]), descp->vdesc_offset, ap); + error = VCALL(ap); /* * Maintain the illusion of call-by-value diff -ur -x compile -x _* freebsd/src/sys/fs/unionfs/union_vnops.c phk/phk_dev/sys/fs/unionfs/union_vnops.c --- freebsd/src/sys/fs/unionfs/union_vnops.c Tue Jan 11 09:53:01 2005 +++ phk/phk_dev/sys/fs/unionfs/union_vnops.c Wed Jan 12 11:12:21 2005 @@ -829,7 +829,7 @@ vp = un->un_lowervp; } ap->a_vp = vp; - return (VCALL(vp, VOFFSET(vop_close), ap)); + return (VOP_CLOSE_AP(ap)); } /* @@ -872,7 +872,7 @@ if ((vp = union_lock_upper(un, td)) != NULLVP) { ap->a_vp = vp; - error = VCALL(vp, VOFFSET(vop_access), ap); + error = VOP_ACCESS_AP(ap); union_unlock_upper(vp, td); return(error); } @@ -888,7 +888,7 @@ if ((un->un_vnode->v_mount->mnt_flag & MNT_RDONLY) == 0) ap->a_mode &= ~VWRITE; - error = VCALL(vp, VOFFSET(vop_access), ap); + error = VOP_ACCESS_AP(ap); if (error == 0) { struct union_mount *um; @@ -896,7 +896,7 @@ if (um->um_op == UNMNT_BELOW) { ap->a_cred = um->um_cred; - error = VCALL(vp, VOFFSET(vop_access), ap); + error = VOP_ACCESS_AP(ap); } } VOP_UNLOCK(vp, 0, td); @@ -1119,7 +1119,7 @@ struct vnode *ovp = OTHERVP(ap->a_vp); ap->a_vp = ovp; - return (VCALL(ovp, VOFFSET(vop_lease), ap)); + return (VOP_LEASE_AP(ap)); } static int @@ -1136,7 +1136,7 @@ struct vnode *ovp = OTHERVP(ap->a_vp); ap->a_vp = ovp; - return (VCALL(ovp, VOFFSET(vop_ioctl), ap)); + return (VOP_IOCTL_AP(ap)); } static int @@ -1151,7 +1151,7 @@ struct vnode *ovp = OTHERVP(ap->a_vp); ap->a_vp = ovp; - return (VCALL(ovp, VOFFSET(vop_poll), ap)); + return (VOP_POLL_AP(ap)); } static int @@ -1594,7 +1594,7 @@ if ((uvp = union_lock_upper(un, td)) != NULLVP) { ap->a_vp = uvp; - error = VCALL(uvp, VOFFSET(vop_readdir), ap); + error = VOP_READDIR_AP(ap); union_unlock_upper(uvp, td); } return(error); @@ -1618,7 +1618,7 @@ KASSERT(vp != NULL, ("union_readlink: backing vnode missing!")); ap->a_vp = vp; - error = VCALL(vp, VOFFSET(vop_readlink), ap); + error = VOP_READLINK_AP(ap); union_unlock_other(vp, td); return (error); @@ -1785,7 +1785,7 @@ KASSERT(vp != NULL, ("union_pathconf: backing vnode missing!")); ap->a_vp = vp; - error = VCALL(vp, VOFFSET(vop_pathconf), ap); + error = VOP_PATHCONF_AP(ap); union_unlock_other(vp, td); return (error); @@ -1804,7 +1804,7 @@ register struct vnode *ovp = OTHERVP(ap->a_vp); ap->a_vp = ovp; - return (VCALL(ovp, VOFFSET(vop_advlock), ap)); + return (VOP_ADVLOCK_AP(ap)); } @@ -1851,7 +1851,7 @@ vp = union_lock_other(un, ap->a_td); ap->a_vp = vp; - error = VCALL(vp, VOFFSET(vop_getacl), ap); + error = VOP_GETACL_AP(ap); union_unlock_other(vp, ap->a_td); return (error); @@ -1873,7 +1873,7 @@ vp = union_lock_other(un, ap->a_td); ap->a_vp = vp; - error = VCALL(vp, VOFFSET(vop_setacl), ap); + error = VOP_SETACL_AP(ap); union_unlock_other(vp, ap->a_td); return (error); @@ -1892,7 +1892,7 @@ struct vnode *ovp = OTHERVP(ap->a_vp); ap->a_vp = ovp; - return (VCALL(ovp, VOFFSET(vop_aclcheck), ap)); + return (VOP_ACLCHECK_AP(ap)); } static int @@ -1910,7 +1910,7 @@ vp = union_lock_other(un, ap->a_td); ap->a_vp = vp; - error = VCALL(vp, VOFFSET(vop_closeextattr), ap); + error = VOP_CLOSEEXTATTR_AP(ap); union_unlock_other(vp, ap->a_td); return (error); @@ -1934,7 +1934,7 @@ vp = union_lock_other(un, ap->a_td); ap->a_vp = vp; - error = VCALL(vp, VOFFSET(vop_getextattr), ap); + error = VOP_GETEXTATTR_AP(ap); union_unlock_other(vp, ap->a_td); return (error); @@ -1957,7 +1957,7 @@ vp = union_lock_other(un, ap->a_td); ap->a_vp = vp; - error = VCALL(vp, VOFFSET(vop_listextattr), ap); + error = VOP_LISTEXTATTR_AP(ap); union_unlock_other(vp, ap->a_td); return (error); @@ -1977,7 +1977,7 @@ vp = union_lock_other(un, ap->a_td); ap->a_vp = vp; - error = VCALL(vp, VOFFSET(vop_openextattr), ap); + error = VOP_OPENEXTATTR_AP(ap); union_unlock_other(vp, ap->a_td); return (error); @@ -1999,7 +1999,7 @@ vp = union_lock_other(un, ap->a_td); ap->a_vp = vp; - error = VCALL(vp, VOFFSET(vop_deleteextattr), ap); + error = VOP_DELETEEXTATTR_AP(ap); union_unlock_other(vp, ap->a_td); return (error); @@ -2022,7 +2022,7 @@ vp = union_lock_other(un, ap->a_td); ap->a_vp = vp; - error = VCALL(vp, VOFFSET(vop_setextattr), ap); + error = VOP_SETEXTATTR_AP(ap); union_unlock_other(vp, ap->a_td); return (error); @@ -2043,7 +2043,7 @@ vp = union_lock_other(un, ap->a_td); ap->a_vp = vp; - error = VCALL(vp, VOFFSET(vop_setlabel), ap); + error = VOP_SETLABEL_AP(ap); union_unlock_other(vp, ap->a_td); return (error); diff -ur -x compile -x _* freebsd/src/sys/kern/vfs_init.c phk/phk_dev/sys/kern/vfs_init.c --- freebsd/src/sys/kern/vfs_init.c Fri Jan 7 09:23:49 2005 +++ phk/phk_dev/sys/kern/vfs_init.c Wed Jan 12 00:04:51 2005 @@ -89,32 +89,6 @@ * Routines having to do with the management of the vnode table. */ -/* - * XXX: hack alert - */ -int -vcall(struct vnode *vp, u_int off, void *ap) -{ - struct vop_vector *vop = vp->v_op; - vop_bypass_t **bpt; - int rc; - - for(;;) { - bpt = (void *)((u_char *)vop + off); - if (vop != NULL && *bpt == NULL && vop->vop_bypass == NULL) { - vop = vop->vop_default; - continue; - } - break; - } - KASSERT(vop != NULL, ("No VCALL(%p...)", vp)); - if (*bpt != NULL) - rc = (*bpt)(ap); - else - rc = vop->vop_bypass(ap); - return (rc); -} - struct vfsconf * vfs_byname(const char *name) { diff -ur -x compile -x _* freebsd/src/sys/sys/vnode.h phk/phk_dev/sys/sys/vnode.h --- freebsd/src/sys/sys/vnode.h Fri Jan 7 09:24:14 2005 +++ phk/phk_dev/sys/sys/vnode.h Wed Jan 12 11:12:21 2005 @@ -409,6 +409,17 @@ #define VDESC_VPP_WILLRELE 0x0200 /* + * A generic structure. + * This can be used by bypass routines to identify generic arguments. + */ +struct vop_generic_args { + struct vnodeop_desc *a_desc; + /* other random data follows, presumably */ +}; + +typedef int vop_bypass_t(struct vop_generic_args *); + +/* * VDESC_NO_OFFSET is used to identify the end of the offset list * and in places where no such field exists. */ @@ -418,9 +429,9 @@ * This structure describes the vnode operation taking place. */ struct vnodeop_desc { - int vdesc_offset; /* offset in vector,first for speed */ char *vdesc_name; /* a readable name for debugging */ int vdesc_flags; /* VDESC_* flags */ + vop_bypass_t *vdesc_call; /* Function to call */ /* * These ops are used by bypass routines to map and locate arguments. @@ -451,14 +462,6 @@ #define VOPARG_OFFSETTO(s_type, s_offset, struct_p) \ ((s_type)(((char*)(struct_p)) + (s_offset))) -/* - * A generic structure. - * This can be used by bypass routines to identify generic arguments. - */ -struct vop_generic_args { - struct vnodeop_desc *a_desc; - /* other random data follows, presumably */ -}; #ifdef DEBUG_VFS_LOCKS /* @@ -521,9 +524,8 @@ /* * This call works for vnodes in the kernel. */ -#define VCALL(a, b, c) vcall((a), (b), (c)) +#define VCALL(c) ((c)->a_desc->vdesc_call(c)) #define VDESC(OP) (& __CONCAT(OP,_desc)) -#define VOFFSET(OP) (VDESC(OP)->vdesc_offset) /* * VMIO support inline @@ -674,7 +676,6 @@ int vop_stdcreatevobject(struct vop_createvobject_args *ap); int vop_stddestroyvobject(struct vop_destroyvobject_args *ap); int vop_stdgetvobject(struct vop_getvobject_args *ap); -int vcall(struct vnode *vp, u_int off, void *ap); void vfree(struct vnode *); void vput(struct vnode *vp); diff -ur -x compile -x _* freebsd/src/sys/tools/vnode_if.awk phk/phk_dev/sys/tools/vnode_if.awk --- freebsd/src/sys/tools/vnode_if.awk Fri Jan 7 09:24:14 2005 +++ phk/phk_dev/sys/tools/vnode_if.awk Wed Jan 12 10:57:13 2005 @@ -62,25 +62,27 @@ function printp(s) {print s > pfile;} function printq(s) {print s > qfile;} -function add_debug_code(name, arg, pos) +function add_debug_code(name, arg, pos, ind) { if (arg == "vpp") - arg = "*vpp"; + star = "*"; + else + star = ""; if (lockdata[name, arg, pos] && (lockdata[name, arg, pos] != "-")) { if (arg ~ /^\*/) { - printh("\tif ("substr(arg, 2)" != NULL) {"); + printc(ind"if ("substr(arg, 2)" != NULL) {"); } - printh("\tASSERT_VI_UNLOCKED("arg", \""uname"\");"); + printc(ind"ASSERT_VI_UNLOCKED("star"a->a_"arg", \""uname"\");"); # Add assertions for locking if (lockdata[name, arg, pos] == "L") - printh("\tASSERT_VOP_LOCKED("arg", \""uname"\");"); + printc(ind"ASSERT_VOP_LOCKED(" star "a->a_"arg", \""uname"\");"); else if (lockdata[name, arg, pos] == "U") - printh("\tASSERT_VOP_UNLOCKED("arg", \""uname"\");"); + printc(ind"ASSERT_VOP_UNLOCKED(" star "a->a_"arg", \""uname"\");"); else if (0) { # XXX More checks! } if (arg ~ /^\*/) { - printh("\t}"); + printc("ind}"); } } } @@ -88,18 +90,18 @@ function add_debug_pre(name) { if (lockdata[name, "pre"]) { - printh("#ifdef DEBUG_VFS_LOCKS"); - printh("\t"lockdata[name, "pre"]"(&a);"); - printh("#endif"); + printc("#ifdef DEBUG_VFS_LOCKS"); + printc("\t"lockdata[name, "pre"]"(a);"); + printc("#endif"); } } function add_debug_post(name) { if (lockdata[name, "post"]) { - printh("#ifdef DEBUG_VFS_LOCKS"); - printh("\t"lockdata[name, "post"]"(&a, rc);"); - printh("#endif"); + printc("#ifdef DEBUG_VFS_LOCKS"); + printc("\t"lockdata[name, "post"]"(a, rc);"); + printc("#endif"); } } @@ -159,8 +161,6 @@ if (qfile) { printq(common_head) - printq("struct vop_generic_args;") - printq("typedef int vop_bypass_t(struct vop_generic_args *);\n") } if (hfile) { @@ -176,9 +176,9 @@ "#include \n" \ "\n" \ "struct vnodeop_desc vop_default_desc = {\n" \ - " 1,\t\t\t/* special case, vop_default => 1 */\n" \ " \"default\",\n" \ " 0,\n" \ + " (void *)(uintptr_t)vop_panic,\n" \ " NULL,\n" \ " VDESC_NO_OFFSET,\n" \ " VDESC_NO_OFFSET,\n" \ @@ -196,8 +196,6 @@ $2 !~ /^[a-z]+$/ || $3 !~ /^[a-z]+$/ || \ $4 !~ /^.$/ || $5 !~ /^.$/ || $6 !~ /^.$/) continue; - if ($3 == "vpp") - $3 = "*vpp"; lockdata["vop_" $2, $3, "Entry"] = $4; lockdata["vop_" $2, $3, "OK"] = $5; lockdata["vop_" $2, $3, "Error"] = $6; @@ -280,9 +278,10 @@ ctrargs = 6; else ctrargs = numargs; - ctrstr = "\tCTR" ctrargs "(KTR_VOP, " ctrstr ")\""; - for (i = 0; i < ctrargs; ++i) - ctrstr = ctrstr ", " args[i]; + ctrstr = "\tCTR" ctrargs "(KTR_VOP,\n\t " ctrstr ")\",\n\t "; + ctrstr = ctrstr "a->a_" args[0]; + for (i = 1; i < ctrargs; ++i) + ctrstr = ctrstr ", a->a_" args[i]; ctrstr = ctrstr ");"; if (pfile) { @@ -299,44 +298,30 @@ for (i = 0; i < numargs; ++i) printh("\t" t_spc(types[i]) "a_" args[i] ";"); printh("};"); + printh(""); # Print out extern declaration. printh("extern struct vnodeop_desc " name "_desc;"); + printh(""); - # Print out function. + # Print out function prototypes. + printh("int " uname "_AP(struct " name "_args *);"); + printh(""); printh("static __inline int " uname "("); for (i = 0; i < numargs; ++i) { printh("\t" t_spc(types[i]) args[i] \ (i < numargs - 1 ? "," : ")")); } - printh("{\n\tstruct " name "_args a;"); - printh("\tint rc;"); + printh("{"); + printh("\tstruct " name "_args a;"); + printh(""); printh("\ta.a_gen.a_desc = VDESC(" name ");"); for (i = 0; i < numargs; ++i) printh("\ta.a_" args[i] " = " args[i] ";"); - for (i = 0; i < numargs; ++i) - add_debug_code(name, args[i], "Entry"); - add_debug_pre(name); - printh("\t{") - printh("\t\tstruct vop_vector *vop = "args[0]"->v_op;") - printh("\t\twhile(vop != NULL && vop->"name" == NULL && vop->vop_bypass == NULL)") - printh("\t\t\tvop = vop->vop_default;") - printh("\t\tKASSERT(vop != NULL, (\"No "name"(%p...)\", "args[0]"));") - printh("\t\tif (vop->"name" != NULL)") - printh("\t\t\trc = vop->"name"(&a);") - printh("\t\telse") - printh("\t\t\trc = vop->vop_bypass(&a.a_gen);") - printh("\t}") - printh(ctrstr); - printh("if (rc == 0) {"); - for (i = 0; i < numargs; ++i) - add_debug_code(name, args[i], "OK"); - printh("} else {"); - for (i = 0; i < numargs; ++i) - add_debug_code(name, args[i], "Error"); + printh("\treturn (" uname "_AP(&a));"); printh("}"); - add_debug_post(name); - printh("\treturn (rc);\n}"); + + printh(""); } if (cfile) { @@ -362,10 +347,40 @@ printc("\tVDESC_NO_OFFSET"); printc("};"); + # Print out function. + printc("\nint\n" uname "_AP(struct " name "_args *a)"); + printc("{"); + printc("\tint rc;"); + printc("\tstruct vnode *vp = a->a_" args[0]";"); + printc("\tstruct vop_vector *vop = vp->v_op;"); + printc(""); + printc("\tKASSERT(a->a_gen.a_desc == VDESC(" name "),"); + printc("\t (\"Wrong a_desc in " name "(%p, %p)\", vp, a));"); + printc("\twhile(vop != NULL && \\"); + printc("\t vop->"name" == NULL && vop->vop_bypass == NULL)") + printc("\t\tvop = vop->vop_default;") + printc("\tKASSERT(vop != NULL, (\"No "name"(%p, %p)\", vp, a));") + for (i = 0; i < numargs; ++i) + add_debug_code(name, args[i], "Entry", "\t"); + add_debug_pre(name); + printc("\tif (vop->"name" != NULL)") + printc("\t\trc = vop->"name"(a);") + printc("\telse") + printc("\t\trc = vop->vop_bypass(&a->a_gen);") + printc(ctrstr); + printc("\tif (rc == 0) {"); + for (i = 0; i < numargs; ++i) + add_debug_code(name, args[i], "OK", "\t\t"); + printc("\t} else {"); + for (i = 0; i < numargs; ++i) + add_debug_code(name, args[i], "Error", "\t\t"); + printc("\t}"); + add_debug_post(name); + printc("\treturn (rc);"); + printc("}\n"); + # Print out the vnodeop_desc structure. printc("struct vnodeop_desc " name "_desc = {"); - # offset - printc("\toffsetof(struct vop_vector, "name"),"); # printable name printc("\t\"" name "\","); # flags @@ -381,6 +396,8 @@ releflags = "0"; printc("\t" releflags vppwillrele ","); + # function to call + printc("\t(void*)(uintptr_t)" uname "_AP,"); # vp offsets printc("\t" name "_vp_offsets,"); # vpp (if any) @@ -393,6 +410,7 @@ printc("\t" find_arg_with_type("struct componentname *") ","); # transport layer information printc("\tNULL,\n};\n"); + } } -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 phk@FreeBSD.ORG | TCP/IP since RFC 956 FreeBSD committer | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence.