Hi
Heres the latest version of my patch. I have learned so much these last
couple of days. I'm very grateful...
Webrev: http://cr.openjdk.java.net/~farvidsson/8056242/webrev.05/index.html
Dmitry: I don't really know how to test the BSD changes. Do you have an
environment for that?
Cheers
/F
On 2014-09-01 15:12, Staffan Larsen wrote:
Thanks for doing these changes. Here are comments on the new version:
os_windows.cpp
- Why not rename enumerate_modules() to get_loaded_modules_info() and update
all caller to use the new unified method?
- At the same time, can we get rid of the pid parameter to enumerate_modules()?
It looks like all callers set it the same value.
- nit: looks like the <= and > used to be aligned in these if-statements:
1380 if (base_addr <= pmod->addr &&
1381 top_address > pmod->addr) {
1436 if (base_addr <= (address)_locate_jvm_dll &&
1437 top_address > (address)_locate_jvm_dll) {
os_solaris.cpp
- I think print_dll_info() should be re-implemented in terms of
get_loaded_modules_info() so that there is only one implementation of this
logic.
- get_loaded_modules_info() references os::print_dll_info() which looks like a
mistake?
os_linux.cpp
- get_loaded_modules_info() could use some blank lines to split it up into
logical chunks of code.
- I think “r” is enough for fopen() - ie drop the “t”
- Can the fgets+sscanf be replaced by a call to fscanf? That would avoid the
‘line’ variable and any possible problems of lines not fitting into the space
allocated.
os_bsd.cpp
- Maybe add a comment about module_top_addr being 0?
Thanks,
/Staffan
On 1 sep 2014, at 14:05, Fredrik Arvidsson <[email protected]> wrote:
Hi
Here is an updated version of the patch. Thanks for the comments.
Webrev: http://cr.openjdk.java.net/~farvidsson/8056242/webrev.01/index.html
/Fredrik
On 2014-08-28 16:21, Staffan Larsen wrote:
Hi Fredrik,
A couple of comments:
- I would prefer if the new callback was unified with the one that exists on
Windows so that we have only on callback-based API for listing dynamic
libraries.
- If you do that, then would you also clean up enumerate_modules() on windows
to get rid of the non-NT support?
- Smaller: I think “address” is a better type to use instead of “u8”.
Thanks
/Staffan
On 28 aug 2014, at 15:54, Fredrik Arvidsson <[email protected]>
wrote:
Hi
Please help me review this small enhancement.
Webrev: http://cr.openjdk.java.net/~farvidsson/8056242/webrev.00/index.html
Bug: https://bugs.openjdk.java.net/browse/JDK-8056242
Cheers
/F
