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.pitt...@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 >>>> >>>> >>> >> > -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * Give Rabbit time, and he'll always get the answer
