Official reviewer needed: Review request for fix for 7200297: agent code does not handle multiple dll_dir paths correctly

[David Holmes]

Hi Bill,

A few minor comments, some of which you've touched on with Dmitry.

David
------


share/back/debugInit.c

  40 #include "sys.h"

Shouldn't that be <sys.h> ?

---

src/share/back/transport.c

        *  Note: Java property sun.boot.library.path contains a single 
directory.
+      *  Note: Above incorrect since 6819213 fixed. Dll_dir is the 
first entry
+      *        and -Dsun.boot.library.path entries are appended.

Better to just change the original comment than to keep it and say it 
isn't true.

---

src/solaris/back/linker_md.c

  113         return;

Adding the return is superfluous. Arguably the whole method should be a 
chained if-else with no returns. Stylistically you have now mixed 
styles: either use a return, or use an else, but not both.

---

src/solaris/demo/jvmti/hprof/hprof_md.c

426         return;

Same comment as for linker_md.c

And why didn't you move *holder = '\0'; in this version?

Ditto src/windows/demo/jvmti/hprof/hprof_md.c

---

src/windows/back/linker_md.c

123         return;

Ditto previous comments.

---


On 27/11/2012 3:45 AM, BILL PITTORE wrote:
> Have a couple of reviews but still need official reviewer to pass muster.
>
> thanks,
> bill
>
>
>
> On 11/22/2012 7:51 AM, Dmitry Samersoff wrote:
>> Bill,
>>
>> Looks good for me.
>>
>> -Dmitry
>>
>> On 2012-11-21 23:31, BILL PITTORE wrote:
>>> 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
>>>>>>
>>>>>>
>>
>
>