RFR: 8161448: 4 JNI exception pending defect groups in DiagnosticCommandImpl.c

[David Holmes]

On 25/08/2016 6:19 PM, Harsha Wardhana B wrote:
> Hello All,
>
> Please find below the revised webrev below.
>
> http://cr.openjdk.java.net/~hb/8161448/webrev.01/

Functional changes seem okay. Exception management seems consistent.

You have made numerous other incidental changes which not only make it 
harder to examine this incrementally, but in most cases are wrong:

  36                         "JMX interface to diagnostic framework 
notifications "
   37                 "is not supported by this VM");

The indentation is wrong after splitting the line. The strings should 
line up.

   62   /* According to ISO C it is perfectly legal for malloc to return 
zero if
   63    * called with a zero argument */

A multi-line comment should end with the */ on its own line.

   70   dcmdArgInfoCls = (*env)->FindClass(
   71           env, 
"com/sun/management/internal/DiagnosticCommandArgumentInfo");

This indent is wrong, the original was correct.

  183       args = getDiagnosticCommandArgumentInfoArray(
  184               env, (*env)->GetObjectArrayElement(env,commands,i),
  185               dcmd_info_array[i].num_arguments);

Indent is wrong, original was correct.

207       obj = JNU_NewObjectByName(
  208               env, 
"com/sun/management/internal/DiagnosticCommandInfo",
  209 
"(Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;"
  210               "Ljava/lang/String;Ljava/lang/String;Ljava/lang/String;"
  211               "ZLjava/util/List;)V",
  212               jname, jdesc, jimpact,
  213               dcmd_info_array[i].permission_class==NULL?NULL:
  214 
(*env)->NewStringUTF(env,dcmd_info_array[i].permission_class),
  215               dcmd_info_array[i].permission_name==NULL?NULL:
  216 
(*env)->NewStringUTF(env,dcmd_info_array[i].permission_name),
  217               dcmd_info_array[i].permission_action==NULL?NULL:
  218 
(*env)->NewStringUTF(env,dcmd_info_array[i].permission_action),
  219               dcmd_info_array[i].enabled, args);

Again indent is wrong (yes those really long strings are a pain but 
that's life) and the original was more correct ie NewObjectByName(env on 
first line, then other args line up underneath.

However if you are going to fix layout in this bit of code then please 
add spaces around the operators ie:

cmd_info_array[i].permission_class == NULL ? NULL :
 
(*env)->NewStringUTF(env,dcmd_info_array[i].permission_class),

Thanks,
David

> Regards
>
> Harsha
>
>
> On Wednesday 24 August 2016 11:48 AM, Harsha Wardhana B wrote:
>> Hi David,
>>
>>
>> On Tuesday 23 August 2016 12:41 PM, David Holmes wrote:
>>> Hi Harsha,
>>>
>>> On 22/08/2016 6:30 PM, Harsha Wardhana B wrote:
>>>> Hello All,
>>>>
>>>> Please review the below parfait fixes for DiagnosticCommandImpl.c
>>>>
>>>> Issue : https://bugs.openjdk.java.net/browse/JDK-8161448
>>>>
>>>> webrev : http://cr.openjdk.java.net/~hb/8161448/webrev.00/
>>>
>>> Looks functionally correct, but I wouldn't complain if you wanted to
>>> use a macro to reduce the duplication and verbosity. Even the:
>>>
>>>  109     if (obj == NULL) {
>>>  110       free(dcmd_arg_info_array);
>>>  111       return NULL;
>>>
>>> can be replaced by an exception-check as that is the only time
>>> JNU_NewObjectByName can return NULL.
>> I am not sure if using macro here is good practice since it will be
>> specific to the function and it will encapsulate the local variable
>> within it. Also, it will reduce code readability. Hence I would like
>> to leave it as is.
>>>
>>> I also noticed an issue that was flagged in some other JNI code lately:
>>>
>>>  213       if (obj == NULL) {
>>>  214           free(dcmd_info_array);
>>>  215           JNU_ThrowOutOfMemoryError(env, 0);
>>>  216           return NULL;
>>>
>>> If obj == NULL then there is already a pending exception and we
>>> should not be throwing OOME.
>>>
>> Sure. This needs to be fixed.
>>> Thanks,
>>> David
>>>
>>>> Regards
>>>>
>>>> Harsha
>>>>
>> Harsha
>