Hi Yasumasa,
I just took a quick look at this. I understand a little about how the
attach mechanism works, but am by no means an expert, and find myself
easily lost in some of the logic. I'll look again after others have also
contributed comments. Here area few comments.
I don't understand the need for the following in
attach_listener_thread_entry()
428 AttachListener::set_state(AL_NOT_INITIALIZED);
There is no path to this statement since the only way out of the loop is
the following:
How are errors in AttachListener::init() properly propagated. I see
is_init_trigger() does the following:
548 if (os::Posix::matches_effective_uid_or_root(st.st_uid)) {
549 init();
550 log_trace(attach)("Attach triggered by %s", fn);
551 return true;
So "true" is returned even though init() maybe have failed, but then
check_socket_file() doesn't even check the result from is_init_trigger():
509 is_init_trigger();
510 return true;
The SIGBREAK code does check the is_init_trigger() result, but since
init() failures are not propagated to is_init_trigger(), the result may
not be accurate.
Have you done any testing of the error handling by forcing errors to happen?
The following code needs a comment to indicate that the current state is
AL_INITIALIZED, and if the socket file was removed it needs to be
recreated and a new socket opened.
379 } else if (AttachListener::check_socket_file()) {
380 continue;
381 }
thanks,
Chris
On 7/4/19 6:27 AM, Yasumasa Suenaga wrote:
Hi all,
Please review this change:
JBS: https://bugs.openjdk.java.net/browse/JDK-8225690
webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8225690/webrev.00/
This issue has been discussed on [1] and [2].
This webrev passed tests on submit repo
(mach5-one-ysuenaga-JDK-8225690-20190704-1214-3626907).
It includes the fix for JDK-8225193. They relate each other, so I fix
them together.
Thanks,
Yasumasa
[1]
https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-July/028585.html
[2]
https://mail.openjdk.java.net/pipermail/serviceability-dev/2019-June/028418.html
