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

Steve Dickson
2010-03-16T21:48:01Z

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.