Max,
Thanks a lot for the review, please find comments in line.
Webrev updated: http://cr.openjdk.java.net/~valeriep/8031003/webrev.01
On 03/17/14 01:47, Wang Weijun wrote:
NativeUtil.h:
88: How about puts(s) or printf("%s", s) (in case s includes "%")?
Sure.
NativeUtil.c:
514-516: not necessary?
Ok.
539-543: Why not TRACEn here?
Didn't want to add TRACE4 just for this one call. Anyway, changed the
2nd one to just 3 arguments and used TRACE3 for both calls.
639-659: It looks like if cbytes == NULL then the function returns NULL with no
exception throwing and this would break something in GSSLibStub.c. But actually
cbytes will never be NULL there, right? How about remove the cbytes == NULL
check at all?
Hmm, getJavaBuffer is an utility method, so supposedly, I think it's
reasonable to return NULL with no exception when cbytes is either NULL
or GSS_C_NO_BUFFER. So, I prefer to keep the check of cbytes == NULL
check since we have no control over the underlying native GSS lib. The
result of getJavaBuffer(...) calls are mostly returned as results. I'd
expect if the native buffer is null or empty, the error code should
indicate the cause and corresponding exception would be thrown.
644: I have no idea why cbytes->length != 0 is checked. Why cannot it be an
empty string?
It's not that it can't be empty. But rather just that I don't see the
need to distinctish between NULL and byte[0] (yet).
GSSLibStub.c:
92: This need not be a jboolean, just use an int.
536: I guess getJavaString on 529 already released outNameBuf?
Good catch, I missed this one when changing code back and forth.
1103: This is not only "cleanup", it is "error".
Agreed.
1105: Not safe to releaseName/releaseCred here?
I think it should be done but the earlier code isn't doing that. I've
added code to release srcName, targetName, and delCred if error is
encountered.
Thanks,
Valerie
Thanks
Max
On Mar 15, 2014, at 6:11, Valerie (Yu-Ching) Peng<valerie.p...@oracle.com>
wrote:
Max,
Can you please help reviewing the fix for 8031003: [Parfait] warnings from
jdk/src/share/native/sun/security/jgss/wrapper: JNI exception pending?
I ended up re-factoring the code to clean things up. So, the changes are
somewhat extensive. I have also replaced the debug callbacks with native printf
calls.
The webrev is:
http://cr.openjdk.java.net/~valeriep/8031003/webrev.00/
Thanks,
Valerie
