Services4User review

[Greg Hudson]

Here are my comments from my (hopefully) final review of the S4U code.
Please address the following and then merge with the latest rev of the
trunk:

  * add_pa_data_element appears to leave out_padata in an invalid state
when the if (copy) block fails to allocate memory.  While you are there,
please rename "out_padata" since it is an in/out parameter, not an
output parameter.

  * kvno should check for -P without -k after parsing all of the
options, so that option order is not important.

  * kvno.c:226 should use &server instead of &in_creds.server, I believe
(that is, you missed a case when altering the logic).

  * The new kvno options should be documented in the man page.

  * s4u_gss.glue.c:kg_get_evidence_ticket is unused; it should be
removed unless there's a reason for it that I couldn't see.

  * The kfree.c changes to krb5_free_checksum_contents and
krb5_free_unparsed_name are unnecessary and undesirable; please revert
them (or explain them if I am misunderstanding).

I have some other notes, which I am not going to require changes for:

  * I wasn't sure what the get_in_tkt.c changes were for.  They relate
to changing the server name for referrals in an as-req, which seems only
peripherally related to S4U at most.

  * I wasn't able to intuit what "gcvt" was supposed to stand for, even
after you added the comment to krb5int_send_tgs.

  * There's some mech logic in s4u_gss_glue.c (conversion between OID
membership in a list and prerfc_mech/rfc_mech flags) which appears to be
common with acquire_cred.c and should be factored out.

  * Going forward, the preferred pattern for handling output parameters
is to (1) initialize them to NULL (or something appropriate for
non-pointer types), and (2) not touch them after that until a successful
return is guaranteed.  This facilitates static analysis tools like
Coverity in determining the correspondence between return values and
allocation of return pointers.

  * In future work, please briefly document the purpose of new internal
functions with a comment above the function definition.

  * I got slightly lost in the mechglue changes, so I wasn't able to
review those as carefully as the rest of the code.