On Tue, 2010-03-16 at 16:49 -0400, Steve Dickson wrote: > > On 03/15/2010 11:58 AM, Trond Myklebust wrote: > > On Mon, 2010-03-15 at 08:20 -0400, steved@xxxxxxxxxx wrote: > >> @@ -1317,15 +1317,21 @@ gss_wrap_req_priv(struct rpc_cred *cred, struct gss_cl_ctx *ctx, > >> inpages = snd_buf->pages + first; > >> snd_buf->pages = rqstp->rq_enc_pages; > >> snd_buf->page_base -= first << PAGE_CACHE_SHIFT; > >> - /* Give the tail its own page, in case we need extra space in the > >> - * head when wrapping: */ > >> + /* > >> + * Give the tail its own page, in case we need extra space in the > >> + * head when wrapping: > >> + * > >> + * call_allocate() allocates twice the slack space required > >> + * by the authentication flavor to rq_callsize. > >> + * For GSS, slack is GSS_CRED_SLACK. > >> + */ > > > > I'm all for improving the comments in the code, but could we please make > > that a separate patch. > Really? I'm curious... what is not clear about the above diff The comments are unrelated to the functionality changes of the patch, and are distracting when you are just trying to figure out those changes. Splitting out the comments (except those directly related to code that is being changed) therefore helps readability. > >> + > >> + GSS_KRB5_SLACK_CHECK; > > ^^^^^^^^^^^^^^^^^^^^^ > > Why is this a macro? > To hide the ugliness of the BUILD_BUG_ON() macro which I think > is a good thing... I would rather see that one line verse the 10 > or so lines it hiding... I wouldn't. I can accept putting that _sum_ into a macro, but expanding the BUILD_BUG_ON. IOW: #define GSS_KRB5_MAX_SLACK_NEEDED \ GSS_KRB5_TOK_HDR_LEN /* gss token header */ \ + GSS_KRB5_MAX_CKSUM_LEN /* gss token checksum */ \ + GSS_KRB5_MAX_BLOCKSIZE /* confounder */ \ .... \ ) and then inserting the following line above BUILD_BUG_ON(GSS_KRB5_MAX_SLACK_NEEDED > RPC_MAX_AUTH_SIZE); That makes it obvious what is being checked and why. Speaking of obvious. Exactly why is GSS_KRB5_TOK_HDR_LEN and GSS_KRB5_MAX_CKSUM_LEN being included twice in that calculation? The second two instances have no comments explaining their presence... -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
