Review request for fix for 7200297

[serguei.spitsyn at oracle.com]

Looks good.

One more reviewer is needed, right?

Thanks,
Serguei

On 11/16/12 12:26 PM, BILL PITTORE wrote:
>
>
> On 11/15/2012 2:49 PM, serguei.spitsyn at oracle.com wrote:
>> Bill,
>>
>> It looks good.
>>
>> Minor:
>>   The line "int n;" is not needed and can be deleted in the files:
>> ||*src/solaris/back/linker_md.c*
>> ||*src/windows/demo/jvmti/hprof/hprof_md.c*
>> *    src/windows/back/linker_md.c*
>> *src/windows/demo/jvmti/hprof/hprof_md.c*
>>
>> ||*
>> *
> Oops. Missed that one. Fixed now in webrev.02
>
>> **Similar simplification can be done:
>> *  src/windows/demo/jvmti/hprof/hprof_md.c*
> Did that but did not save file to disk before running webrev.
>
>
>>
>>
>> I do not have a reviewer status, but you can list as a reviewer. :)
>> Thank you for fixing it!
>>
> Ok, will do.
>
> thanks,
> bill
>
>> Thanks,
>> Serguei
>>
>>
>>
>> On 11/15/12 11:27 AM, BILL PITTORE wrote:
>>> Hi Serguei,
>>>
>>> Thanks for the review and your excellent suggestion at some 
>>> simplification in dll_build_name. Applied the changes, built, test 
>>> and all is still working. New webrev at:
>>> http://cr.openjdk.java.net/~bpittore/7200297/webrev.01/
>>>
>>> Are you an official reviewer?
>>>
>>> thanks,
>>> bill
>>>
>>>
>>> On 11/15/2012 4:00 AM, serguei.spitsyn at oracle.com wrote:
>>>>
>>>> ||Hi Bill,
>>>>
>>>> *src/share/back/debugInit.c*
>>>>
>>>> **271 JVMTI_FUNC_PTR(gdata->jvmti,GetSystemProperty)
>>>>
>>>>    Space is missed after comma.
>>>>
>>>> ||*src/share/back/error_messages.c
>>>> *
>>>>
>>>>   74 /* May be called before NPT is initialized so don't fault */
>>>>
>>>>     The comment seems to belong to the else statement.
>>>>
>>>> ||*src/share/back/transport.c
>>>> *||*src/share/demo/jvmti/hprof/hprof.h*||*
>>>> src/share/demo/jvmti/hprof/hprof_init.c*
>>>>
>>>>     No comments.
>>>>
>>>> ||*src/solaris/back/linker_md.c
>>>> *
>>>>
>>>> ***Lines 75-96:*
>>>>
>>>>    It seems the *if/else* is not needed as the while loop would 
>>>> serve the else as well.
>>>>
>>>> *     73 int n;*
>>>>
>>>>   Line #73 can be removed as "n" is not used.
>>>>
>>>> *   81 if ((p - pathname) == 0) {*
>>>>
>>>>    Nit: Replace *(p - pathname) == 0*   with *p == pathname*
>>>>
>>>> *   119 char *p;*
>>>>
>>>>    Line #119 can be removed as "p" is not used.
>>>>
>>>>
>>>> ||*src/solaris/demo/jvmti/hprof/hprof_md.c
>>>> *
>>>>
>>>> ***Lines 395-418:*
>>>>
>>>>      The same comments as for the *linker_md.c*.
>>>>
>>>>
>>>> ||*src/solaris/npt/npt_md.h
>>>> *
>>>>
>>>>     No comments.
>>>>
>>>> ||*src/windows/back/linker_md.c
>>>> *
>>>>
>>>>    The same comments as for the *linker_md.c*.
>>>>
>>>>
>>>> ||*src/windows/demo/jvmti/hprof/hprof_md.c
>>>> *
>>>>
>>>>
>>>> ****Lines 381-407:*
>>>> *
>>>>
>>>>      The same comments as for the *linker_md.c*.
>>>>
>>>> *
>>>> *
>>>>
>>>> ||*src/windows/npt/npt_md.h
>>>> *
>>>>
>>>>     No comments.
>>>>
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 11/14/12 2:27 PM, bill.pittore at oracle.com wrote:
>>>>> This bug has to do with the jdwp and hprof agents not parsing the 
>>>>> sun.boot.library.path (dll_dir) correctly since the fix for 
>>>>> 6819213 went in some years ago. This bug popped up while working 
>>>>> on a particular platform that does not have the ability to set 
>>>>> LD_LIBRARY_PATH before running the VM. As documented in the bug, 
>>>>> on most platforms even if the sun.boot.library.path consists of 
>>>>> multiple paths and the jdwp or hprof code fails to load a 
>>>>> dependent lib, the system falls back to using LD_LIBRARY_PATH so 
>>>>> the failure is masked. On some other platforms, this failover 
>>>>> doesn't exist so we get an error when trying to load jdwp and 
>>>>> hprof dependent libs.
>>>>>
>>>>> Some notes on a couple of files.
>>>>> *
>>>>> debugInit.c, hprof_init.c*:
>>>>> Rearranged the init order so that the jvmti pointer gets 
>>>>> initialized before attempting to load the npt shared library.
>>>>>
>>>>> *linker_md.c, hprof_md.c*
>>>>> Much of the platform code in hprof and jdwp is duplicated and this 
>>>>> change is no different. Based on the code in hotspot 
>>>>> os_solaris/windows.cpp it parses the boot library path and 
>>>>> attempts to find the library.
>>>>> *
>>>>> error_messages.c*
>>>>> Fixed a bug in the error message code that blindly dereferenced 
>>>>> the npt pointer even if it wasn't set because of an error in loading.
>>>>>
>>>>> Webrev: http://cr.openjdk.java.net/~bpittore/7200297/webrev.00/
>>>>>
>>>>> Ran the ute nsk/jdwp nsk/jvmti nsk/hprof tests, the JCK jdwp/jvmti 
>>>>> tests and some command line testing on windows and linux.
>>>>>
>>>>> thanks,
>>>>> bill
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20121116/65fcfe38/attachment-0001.html