> http://cr.opensolaris.org/~gtb/6791302/webrev-O/
More comments:
- usr/src/uts/common/rpc/sec_gss/svc_rpcsec_gss.c
All calls to cmn_err(CE_NOTE, "gtb: ..."... must go. But I'm sure
you knew that.
- usr/src/uts/common/rpc/sec_gss/svc_rpcsec_gss.c:858-860
I don't think the cstyle requires it, but I prefer not putting curly
braces around single statement bodies. In any case, be consistent:
858 + if (output_token.length != 0) {
859 + GSS_COPY_BUFFER(call_res.token, output_token);
860 + }
903 + if (free_mech_type && mech_type)
904 + kgss_free_oid(mech_type);
I know, it was like that in the original...
Plus, GSS_COPY_BUFFER already puts curly braces in there:
#define GSS_COPY_BUFFER(dest, src) { \
(dest).length = (src).length; \
(dest).value = (src).value; }
(For a macro used exactly twice that's a very ugly macro. Any reason
why it can just eval to (dest) = (src)? Maybe dest and src are of
diff types?)
- Total nit: you can set *no_dispatch much earlier in
rpcsec_gss_init(). It's OK to set it to TRUE and return an error.
- usr/src/uts/common/rpc/sec_gss/svc_rpcsec_gss.c:715-716
I don't get this comment. Can you explain a bit more?
- usr/src/uts/common/rpc/sec_gss/svc_rpcsec_gss.c:757-760
Another comment I don't understand. What's "Server_creds" and in
what way was it "right"?
- usr/src/uts/common/rpc/sec_gss/svc_rpcsec_gss.c:808
"We have a failure... Don't dispatch. ..."
Well, yes, but we never dispatch except in successful RPCSEC_GSS_DATA
cases...
- usr/src/uts/common/rpc/sec_gss/svc_rpcsec_gss.c:1306-1307
"Kernel client" -- do you mean our NFS/RPCSEC_GSS client? Just
remove this comment, even if you can't find a client to test this
code path with, or perhaps just say that. (And no "XXX" -- GKs no
likey.)
- usr/src/uts/common/rpc/sec_gss/svc_rpcsec_gss.c:1340-1345
1340 + (void) svc_sendreply(rqst->rq_xprt, xdr_void, NULL);
1341 + *no_dispatch = TRUE;
1342 + ASSERT(client_data->ref_cnt > 0);
1343 + client_data->ref_cnt--;
1344 + client_data->stale = TRUE;
1345 + mutex_exit(&client_data->clm);
Do we want to call svc_sendreply() while holding a mutex?? See all
other calls to svc_sendreply(), as well as the call to
kgss_accept_sec_context() (since it upcalls, and therefore blocks).
- usr/src/uts/common/rpc/sec_gss/svc_rpcsec_gss.c:rpcsec_gss_data()
Hmmm, we do quite a bit of work while holding client_data->clm.
Specifically, the calls to check_verf() and set_response_verf()
should probably be done without holding that lock, but note that
check_seq *must* be called with that lock held.
If you want to file a separate CR for this, go ahead.
Done.
Nico
--
