Code Review Request: 8031003: [Parfait] warnings from jdk/src/share/native/sun/security/jgss/wrapper: JNI exception pending

[Valerie (Yu-Ching) Peng]

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.peng at 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