Danhua Shao wrote: > Hi, > > I have got an implementation of DTrace probes for NFS v3 client. > Webrev of my change is at: > > http://cr.opensolaris.org/~danhua/webrev/ >
Thanks for doing it in the open! Some nits: 1) Run 'hg nits' first -- it will flag you on a lot of what I will mention. nfs.d: 27 #pragma ident "%Z%%M% %I% %E% SMI" needs to go. In each file.... ----------- 126 /* define argument translator for nfsv3client*/ need a space between 't' and '*' ------------ 133 ci_protocol = ((rnode_t*)arg0)->r_server->sv_knconf->knc_protofmly == 134 "inet" ? "ipv4" : ((rnode_t*)arg0)->r_server->sv_knconf->knc_protofmly == "inet6" ? "ipv6" : "<unknown>"; Not indented correctly and line is too long. ----------------- ditto for 142 and 143. ---------- 2) Do an 'hg commit' before the webrev We want to see the comments and not 'no comments' Cdiffs <http://cr.opensolaris.org/%7Edanhua/webrev/usr/src/lib/libdtrace/common/nfs.d.cdiff.html> Udiffs <http://cr.opensolaris.org/%7Edanhua/webrev/usr/src/lib/libdtrace/common/nfs.d.udiff.html> Wdiffs <http://cr.opensolaris.org/%7Edanhua/webrev/usr/src/lib/libdtrace/common/nfs.d.wdiff.html> Sdiffs <http://cr.opensolaris.org/%7Edanhua/webrev/usr/src/lib/libdtrace/common/nfs.d.sdiff.html> Frames <http://cr.opensolaris.org/%7Edanhua/webrev/usr/src/lib/libdtrace/common/nfs.d.frames.html> Old <http://cr.opensolaris.org/%7Edanhua/webrev/usr/src/lib/libdtrace/common/nfs.d-.html> New <http://cr.opensolaris.org/%7Edanhua/webrev/usr/src/lib/libdtrace/common/nfs.d.html> Patch <http://cr.opensolaris.org/%7Edanhua/webrev/usr/src/lib/libdtrace/common/nfs.d.patch> Raw <http://cr.opensolaris.org/%7Edanhua/webrev/raw_files/new/usr/src/lib/libdtrace/common/nfs.d> *usr/src/lib/libdtrace/common/nfs.d* *** NO COMMENTS *** 22 lines changed: 22 ins; 0 del; 0 mod; 124 unchg Specific comments: +++++++++++++++++ sdt_subr.c: 98 99 /* declare nfsv3client provider */ 100 { "nfsv3client", "__nfsv3client_", &stab_attr, 0 }, 101 /* declare nfsv3client provider */ 102 Remove all of these but line 100 ---- 178 /* argument translation for nfsv3client */ 179 327 /* argument translation for nfsv3client */ 328 Remove these 4 lines ------------ 180 - 325 lines might be too long ----------- 919 920 921 922 923 924 925 926 927 928 Remove all of these ----------------- 994 -> 1130 remove all of these -------------- nfs3_vfsops.c: 1177 uint32_t * pxid; No space between '*' and 'p'. --- 1181 pxid = tsd_get(dtrace_nfsv3client_key); 1182 if (pxid == NULL) { 1183 pxid = kmem_zalloc(sizeof (uint32_t), KM_SLEEP); 1184 (void) tsd_set(dtrace_nfsv3client_key, pxid); 1185 } 1186 ASSERT( pxid != NULL); 1187 Could suggest you remove the ' ' between '( pxid' but if the kmem_zalloc is a KM_SLEEP, you do not need the ASSERT. So remove 1186. ----- 1188 *pxid = 0; Remove, the KMEM_ZALLOC zero'ed it out for you. --- 1190 and 1197 -- too long ----------- 1549 -> 1570 same as the above comments --- nfs3_vnops.c For each block, see the above comments ------- nfs_client.c See above comments for some blocks.. ----------- 2797 /* create tsd key for xid */ 2798 tsd_create(&dtrace_nfsv3client_key, nfsv3client_key_destroy); 2799 and then 2821 2822 /* destroy tsd key for xid */ 2823 tsd_destroy(&dtrace_nfsv3client_key); This may not have ever been created. How is that handled? ----- 3358 void nfsv3client_key_destroy(void *tp) 3359 { Needs to be static and a line break between void and the name... --------- nfs_subr.c: 291 292 One must go. Be careful of adding too many extra lines. ---- sdt.h: 194 /* define DTrace declaration macro for nfsv3client probes */ 209 /* define DTrace declaration macro for nfsv3client probes */ You should get rid of these lines.... ------- 195 #define DTRACE_NFSV3CLIENT_3(name, type1, arg1, type2, arg2, \ 196 type3, arg3) \ 197 DTRACE_PROBE3(__nfsv3client_##name, type1, arg1, type2, arg2, \ 198 type3, arg3); 199 200 #define DTRACE_NFSV3CLIENT_4(name, type1, arg1, type2, arg2, \ 201 type3, arg3, type4, arg4) \ 202 DTRACE_PROBE4(__nfsv3client_##name, type1, arg1, type2, arg2, \ 203 type3, arg3, type4, arg4); 204 205 #define DTRACE_NFSV3CLIENT_5(name, type1, arg1, type2, arg2, \ 206 type3, arg3, type4, arg4, type5, arg5) \ 207 DTRACE_PROBE5(__nfsv3client_##name, type1, arg1, type2, arg2, \ 208 type3, arg3, type4, arg4, type5, arg5); You should get the continuation chars to line up on the right. Most of the other entries take that care. > The provider and probes are described in attached proposal. > > Welcome comments! > > Regards, > > Danhua > ------------------------------------------------------------------------ > > _______________________________________________ > nfs-discuss mailing list > nfs-discuss at opensolaris.org
