<div dir="ltr"><div><div><div><div><div>Hi,<br><br></div>Just a comment from the maintainer of the Solaris/illumos port. As the shared posix <br></div>implementation is for bsd and linux, please consider replacing the wrapping '#ifndef AIX'<br></div>with '#if defined(BSD) || defined(LINUX)'. Not only does this make the lives of those of<br></div>us maintaining external ports much easier, it also far more clearly expresses the intent<br>of the code.<br><br></div>Thanks!<br><div><div><div><br></div></div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 21, 2024 at 3:21 PM Sonia Zaldana Calles <<a href="mailto:szaldana@openjdk.org">szaldana@openjdk.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi folks,<br>
<br>
This PR aims to unify a lot of the shared code between ```attachListener_linux.cpp``` and ```attachListener_bsd.cpp```. <br>
<br>
Some key points:<br>
<br>
1. AIX: <br>
<br>
```attachListener_aix.cpp``` has some key differences from linux/bsd, so I have reduced the scope of the issue to linux and bsd only. As a result, there's an ifndef AIX statement wrapping the whole file. Unifying AIX could be a separate issue.<br>
<br>
2. Peer credentials: <br>
<br>
In ```AttachListener::dequeue```, bsd and linux use different APIs to obtain peer credentials. Linux uses ```getsockopt```, while bsd uses ```getpeerid```. <br>
<br>
Bsd doesn't have [SO_PEERCRED ](<a href="https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/getsockopt.2.html" rel="noreferrer" target="_blank">https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/getsockopt.2.html</a>). While Linux has ```getpeerid``` implemented on top of ```getsockopt```, it comes in the [bsd compat lib](<a href="https://cgit.freedesktop.org/libbsd/tree/src/getpeereid.c" rel="noreferrer" target="_blank">https://cgit.freedesktop.org/libbsd/tree/src/getpeereid.c</a>). <br>
<br>
To avoid adding further dependencies, I have kept both API calls separated by an ifdef macro. <br>
<br>
Additionally, there are some slight differences in the logging messages in this area for bsd and linux. It's possible to coalesce them should this make more sense. <br>
<br>
3. ```AttachListener::is_init_trigger()```<br>
<br>
Please refer to [mailing list discussion](<a href="https://mail.openjdk.org/pipermail/hotspot-runtime-dev/2024-March/068413.html" rel="noreferrer" target="_blank">https://mail.openjdk.org/pipermail/hotspot-runtime-dev/2024-March/068413.html</a>) for details. <br>
<br>
Per the discussion, I tried removing the check for ".attach_pid" in the current working directory (more closely resembling what the bsd code does). However, I noted this caused many failure across linux hotspot tests. See GHA job with failures [here](<a href="https://github.com/SoniaZaldana/jdk/actions/runs/8349574000" rel="noreferrer" target="_blank">https://github.com/SoniaZaldana/jdk/actions/runs/8349574000</a>). <br>
<br>
Therefore, I opted for the Linux approach in this PR. This means we do the check in the current working directory across bsd and linux, although this has been removed from bsd before. This seemingly doesn't break any bsd tests.<br>
<br>
5. Minimal JVM Variant: <br>
<br>
I came across an error in the minimal JVM build:<br>
` error: invalid use of incomplete type ‘class AttachOperation’<br>
   80 | class PosixAttachOperation : public AttachOperation {`<br>
<br>
After a bit of digging, I found that we only get complete definition of AttachOperation inside the directive ```#IF INCLUDE_SERVICES``` [here](<a href="https://github.com/openjdk/jdk/blob/master/src/hotspot/share/services/attachListener.hpp#L138" rel="noreferrer" target="_blank">https://github.com/openjdk/jdk/blob/master/src/hotspot/share/services/attachListener.hpp#L138</a> ). <br>
<br>
I added the same directive for the ```attachListener_posix.cpp``` file but I’m not entirely sure why this wasn't an issue before. The JVM minimal build compiles now, but I'd appreciate any feedback on this change.<br>
<br>
Testing: <br>
- [x] Hotspot test suite passes<br>
<br>
Please let me know your thoughts, <br>
Sonia<br>
<br>
-------------<br>
<br>
Commit messages:<br>
 - 8324683: Unify AttachListener code for Posix platforms<br>
<br>
Changes: <a href="https://git.openjdk.org/jdk/pull/18283/files" rel="noreferrer" target="_blank">https://git.openjdk.org/jdk/pull/18283/files</a><br>
  Webrev: <a href="https://webrevs.openjdk.org/?repo=jdk&pr=18283&range=00" rel="noreferrer" target="_blank">https://webrevs.openjdk.org/?repo=jdk&pr=18283&range=00</a><br>
  Issue: <a href="https://bugs.openjdk.org/browse/JDK-8324683" rel="noreferrer" target="_blank">https://bugs.openjdk.org/browse/JDK-8324683</a><br>
  Stats: 1692 lines in 3 files changed: 589 ins; 1103 del; 0 mod<br>
  Patch: <a href="https://git.openjdk.org/jdk/pull/18283.diff" rel="noreferrer" target="_blank">https://git.openjdk.org/jdk/pull/18283.diff</a><br>
  Fetch: git fetch <a href="https://git.openjdk.org/jdk.git" rel="noreferrer" target="_blank">https://git.openjdk.org/jdk.git</a> pull/18283/head:pull/18283<br>
<br>
PR: <a href="https://git.openjdk.org/jdk/pull/18283" rel="noreferrer" target="_blank">https://git.openjdk.org/jdk/pull/18283</a><br>
</blockquote></div><br clear="all"><br><span class="gmail_signature_prefix">-- </span><br><div dir="ltr" class="gmail_signature">-Peter Tribble<br><a href="http://www.petertribble.co.uk/" target="_blank">http://www.petertribble.co.uk/</a> - <a href="http://ptribble.blogspot.com/" target="_blank">http://ptribble.blogspot.com/</a></div>