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

Kevin Coffman
2010-03-16T21:51:09Z

On Tue, Mar 16, 2010 at 4:14 PM, Trond Myklebust
<trond.myklebust@fys.uio.no> 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:

>> >> +
>> >> + =A0GSS_KRB5_SLACK_CHECK;
>> > =A0 =A0 =A0 =A0 =A0 ^^^^^^^^^^^^^^^^^^^^^
>> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 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 \
> =A0 =A0 =A0 =A0GSS_KRB5_TOK_HDR_LEN =A0 =A0 =A0/* gss token header */=
=A0 =A0 =A0 =A0 \
> =A0 =A0 =A0 =A0+ GSS_KRB5_MAX_CKSUM_LEN =A0/* gss token checksum */ =A0=
=A0 =A0 \
> =A0 =A0 =A0 =A0+ GSS_KRB5_MAX_BLOCKSIZE =A0/* confounder */ =A0 =A0 =A0=
=A0 =A0 =A0 =A0 \
> =A0 =A0 =A0 =A0.... \
> =A0 =A0 =A0 =A0)
>
> 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.

fwiw, I agree!

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

I'll look back and see if there is a reason, or if this is just wrong.
I believe there is another checksum, so I think it is correct. But
supporting comments do seem necessary.

I'll be travelling with limited network access until Monday, so I
might not give a prompt reply.

K.C.