Hi Yasumasa,

  84     // It maps the LWPID in the host to it in the container.

"it" -> "the PID"

 286     // Get LWPID in the host from the container's LWPID.
 287     public int getHostPID(int id) {
 288         try {
 289             return nspidMap.get(id);
 290         } catch (NullPointerException e) {
 291             return -1;
 292         }
 293     }

What is the source of the NPE here? Is it because nspidMap was never initialized because the process is not in a container? In that case I think you should be checking for null rather than having an NPE be part of normal execution.

  42             int hostPID = ((LinuxDebuggerLocal)debugger).getHostPID(pid);
  43             if (hostPID != -1) {
  44                 pid = hostPID;
  45             }

A comment here would be helpful.

The rest looks good. I should probably run it through some internal testing. Let me know when you have a final webrev.

thanks,

Chris

On 7/18/18 5:59 AM, Yasumasa Suenaga wrote:
PING:

Could you review it?

   JBS:    https://bugs.openjdk.java.net/browse/JDK-8205992
   webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8205992/webrev.01/

This change has been reviewed by Jini.
We need a Reviewer.


Thanks,

Yasumasa


On 2018/07/12 13:42, Yasumasa Suenaga wrote:
Thanks Jini,

I uploaded new webrev. It contains some comments and removing extra space.

http://cr.openjdk.java.net/~ysuenaga/JDK-8205992/webrev.01/


Yasumasa



2018-07-12 2:32 GMT+09:00 Jini George <jini.geo...@oracle.com>:
Hi Yasumasa,

This looks good to me except for one nit. And some more comments would help. For e.g., it would help to say that NSPidMap is to map the host to container
lwpids.

The nit:

*
http://cr.openjdk.java.net/~ysuenaga/JDK-8205992/webrev.00/src/jdk.hotspot.agent/linux/native/libsaproc/LinuxDebuggerLocal.c.sdiff.html
Line 253: extra space after the parentheses

Thanks,
Jini.

On 7/4/2018 4:34 AM, Yasumasa Suenaga wrote:

PING: Could you review it?

   JBS: https://bugs.openjdk.java.net/browse/JDK-8205992
   webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8205992/webrev.00/



Thanks,

Yasumasa


On 2018/06/28 22:12, Yasumasa Suenaga wrote:

Hi all,

Please review this change.

   JBS: https://bugs.openjdk.java.net/browse/JDK-8205992
   webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8205992/webrev.00/

I tried to attach jhsdb to java process in docker container from
container host, but it couldn't.
jcmd supports PID namespace in JDK-8193710, but jhsdb hasn't yet.

SA gets LWP ID via thread stack and funcs in libthread_db.so, but they returns PIDs in container - they are different from host's PID. So I added the code to scan /proc/<PID>/task to get all LWP IDs and they are kept in a
Map in LinuxDebuggerLocal.

Also SA_ALTROOT is set to /proc/<PID>/root if SA detects debuggee runs in
container. It helps SA to parse binaries in container.

This change has been pushed to submit repo, and it was failed on OS X
(mach5-one-ysuenaga-JDK-8205992-20180628-1015-28963).
But I guess it causes JDK-8205906. This change affects to Linux only.

Could you review it?


Thanks,

Yasumasa




Reply via email to