RFR: JDK-8057057: Cleanup DIO JNI code

[Alexey Konstantinov]

Hi Jen,

I agree with your comments. The changes look fine to me then.

Thanks,
Alexey

----- Original Message -----
From: jen.dority at oracle.com
To: dio-dev at openjdk.java.net
Sent: Tuesday, September 2, 2014 10:08:37 PM GMT +04:00 Abu Dhabi / Muscat
Subject: Re: RFR: JDK-8057057: Cleanup DIO JNI code


On 9/2/2014 1:45 PM, Alexey Konstantinov wrote:
> Hi Jen,
>
> jni_i2c.cpp:
>   55     jfieldID addressID = deviceNumberID ?
>   56             env->GetFieldID(configClass, "address", "I") :
>   57             NULL;
>   58     jint address = addressID ?
>   59             env->GetIntField(config, addressID) :
>   60             0;
>   61
>   62     jfieldID addressSizeID = addressID ?
>   63             env->GetFieldID(configClass, "addressSize", "I") :
>   64             NULL;
>   65     jint addressSize = addressSizeID ?
>   66             env->GetIntField(config, addressSizeID) :
>   67             0;
> Looks like in line 62 address should be tested instead of addressID.
I disagree. The address field is just an int so it is perfectly 
acceptable for its value to be 0. It's the fieldIDs being NULL that is 
significant.
>
> jni_i2c.cpp:
>   84         device_reference device = INVALID_DEVICE_REFERENCE;
>   85         if (result == JAVACALL_DIO_OK) {
>   86             device = createDeviceReference(handle, _i2c_close,
>   87                                            javacall_i2c_lock, 
> javacall_i2c_unlock);
>   88             if (device == INVALID_DEVICE_REFERENCE) {
>   89                 javacall_i2c_close(handle);
>   90                 result = JAVACALL_DIO_OUT_OF_MEMORY;
>   91             } else {
>   92                 result = saveDeviceReferenceToDeviceObject(env, 
> obj, device);
>   93             }
>   94         }
>   95     }
>   96
>   97     if (env->ExceptionCheck() != JNI_TRUE) {
>   98         checkJavacallFailure(env, result);
>   99     }
> Would you agree with that the implementation would be even more robust 
> if there were a call to javacall_i2c_close(handle) to handle an 
> exception that might happen in line 92?
Technically, yes. But I think that's a step to far along the road of 
trying to recover and so kind of outside the scope of these changes. The 
purpose of this changeset is to avoid calling JNI functions while an 
exception is pending as that behavior usually results in native crashes. 
What yuou're pointing out is a separate, existing weakness possibly 
present in other areas. As such, I'd prefer to address it in a separate 
JIRA issue & patch.

Jen

>
> Thanks,
> Alexey
>
> On 9/2/2014 8:08 PM, Jen Dority wrote:
>> issue: https://bugs.openjdk.java.net/browse/JDK-8057057
>> webrev: http://cr.openjdk.java.net/~jld/8057057/
>>
>> Hi All,
>>
>> Need a review for a bunch of JNI fixes. The code was missing 
>> Exception/NULL checks in a bunch of key places and I even found a 
>> missing return value.
>>
>> Thanks,
>> Jen
>