Review request for fix for 7200297

[BILL PITTORE]

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/db28c70d/attachment.html