New GSS-preauth plugin available for testing

Alejandro Perez Mendez alex at um.es
Wed Sep 5 07:58:32 EDT 2012


Hi Luke,

I already addressed most of your comments. Thanks again for helping me. 
There are a few of them that I didn't address for several reasons (see 
comments below).

> Hi Alejandro,
>
> It's fantastic to see this plugin developed.
>
> I haven't attempted to build or use the code, or analyse the logic, but here are a few comments on style that might help with eventual upstream integration.
>
> * LGPL may not be a compatible license if your intention is to have it distributed alongside MIT
> * s/cledentials/credentials/
> * requested_flags might more conventionally be a macro rather than a static variable (renamed to suit)
> * might be cleaner to use an unnamed struct or make gsspreauth_req_context_t more similar to gssapi_req_context_t
> * for use_default_gss_cred/federated, Greg would probably prefer an int (I've been picked up for using bitfields before)

Do you mean using a single int variable, and then using flags for the 
options? Why is that better? To maintain somehow struct size when adding 
new options?

> * use k5alloc() instead of malloc
> * I don't know if this is strictly required by the coding guidelines, but (say in client_plugin_fini), I believe the convention is to have a space on either side of the assignment operator, and a space before braces (unless they're on a line by themselves)
> * there are a bunch of lines with trailing spaces that "git diff" flags

How do I know what are these files, and how do I put them into their 
original shape? I'm quite new using git, sorry :)

> * in client_gic_opt(), may be nicer to use braces or eliminate newlines between branches
> * check the return value of malloc() (and use k5alloc()). eg. get_name_from_gss_credential(), create_first_request()
> * don't use magic numbers (e.g. 200 bytes for prf_in), define a constant macro somewhere
> * I would prefer initialising maj_stat to GSS_C_COMPLETE rather than 0
> * perhaps use krb5_principal_compare() rather than strcmp()

I don't think that's really necessary, since I'm comparing KRB names 
with GSS names. The common point is actually the text format.

Regards,
Alejandro


More information about the krbdev mailing list