Services4User review

[Nicolas Williams]

On Mon, Aug 24, 2009 at 04:49:51PM -0400, Greg Hudson wrote:
> * Do not put parens around return expressions:
>   - g_acquire_cred_imp_cred.c
>   - g_acquire_cred_imp_name.c
>   (These may be the result of copying existing code.)

Putting parens around return expressions is a Solaris C-style dictum.
If this code is based on Solaris' libgss, then I recommend adhering to
the Solaris C-style if possible, for now.

http://opensolaris.org/os/community/on/
http://opensolaris.org/os/community/documentation/getting_started_docs/cstyle.ms.pdf
http://opensolaris.org/os/community/documentation/getting_started_docs/cstyle.ms

That applies to just a few of your other comments (e.g., put a space
between sizeof and the parenthesized type expression).

> * In krb5_gss_cred_id_rec you introduce some bitfields where none were
> present.  I don't know if it is important to contain the size of id_rec
> structures, and using bitfields can expand the size of the code which
> processes those fields, which at times can be more undesirable than
> allocating a bit more memory in an infrequently-used structure.  I am
> not sure how the balance comes out in this case, but please be aware of
> the tradeoff.

The *gss_*rec* structs are all private.  Their size and layout can be
changed at any time without trouble.

I prefer bitfields where possible, and for several reasons:

 - I wouldn't presume to be able to write more optimal code than the
   compiler -- hard data is needed (and even then, there's more than one
   compiler).

 - These flags are not referenced in performance sensitive fastpaths.

 - Bitfields are clearer, more elegant, and easier to use that CPP
   symbols and bit-wise expressions.

Nico
--