Re: [PATCH 01/22] gss_krb5: introduce encryption type framework

[Steve Dickson <SteveD@redhat.com>]

On 03/16/2010 05:14 PM, Trond Myklebust wrote:
> 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@redhat.com 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.
I guess.. but at the end of the day it all ends up in the
same place... 

The comments will be broken into a separate patch... 

> 
>>>> +
>>>> +	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.

The above BUILD_BUG_ON() will be added... 


steved.