[nfs-discuss] Code Review for NFS v3 client DTrace provider

[Tom Haynes]

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" : 
"";

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 

 
Udiffs 

 
Wdiffs 

 
Sdiffs 

 
Frames 

 
Old 

 
New 

 
Patch 

 
Raw 

 
*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 d

[nfs-discuss] Code Review for NFS v3 client DTrace provider

[Danhua Shao]

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/

The provider and probes are described in attached proposal.

Welcome comments!

Regards,

Danhua
-- next part --
An embedded and charset-unspecified text was scrubbed...
Name: proposal-nfsv3client.txt
URL: