Review request for fix for 7200297

[BILL PITTORE]

Hi Dmitry,

On 11/21/2012 4:56 AM, Dmitry Samersoff wrote:
> Bill,
>
>
> Few nits:
>
> 1.
>
> dll_build_name is exactly the same for all locations could we place it
> to some common place?
It looked somewhat intentional to have the agents and the hprof code be 
self-contained. I looked at using a common file but the makefile changes 
and source tree changes seemed a bit much. Hprof code is "unsupported 
demo" code and jdwp is supported agent so I went with the current scheme 
of having the code be self-contained.
> Also  file_exists is not necessary and could be
> replaced with just ::access(buffer,R_OK) (Windows has it as well)
I'll go with the access suggestion above, less code.
> but if you prefer to keep it:
>
> strlen(filename)==0 could be a *filename == 0
>
>
> 2. We don't need else after return in all *_md.c files
>     e.g. linker_md.c:122
Semantically, you're correct. I think in terms of code readability I 
like the 'else' since it makes it clear to the reader that there are two 
different cases. C compiler will 'do the right thing'.
Updated the webrev at 
http://cr.openjdk.java.net/~bpittore/7200297/webrev.03/

thanks,
bill
>
> Otherwise looks good.
>
> -Dmitry
>
> On 2012-11-20 01:10, BILL PITTORE wrote:
>> Have gotten reviews from Serguei Spitsyn for the changes, made some
>> improvements and need an official reviewer to check it out.
>>
>> http://cr.openjdk.java.net/~bpittore/7200297/webrev.02
>>
>> thanks,
>> bill
>>
>>
>>
>> On 11/14/2012 5: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
>>>
>>>
>>
>