8277489: Rewrite JAAS UnixLoginModule with FFM #28931
Conversation
|
👋 Welcome back weijun! A progress list of the required criteria for merging this PR into |
|
@wangweij This change is no longer ready for integration - check the PR body for details. |
Webrevs
|
|
/reviewers 2 reviewer |
| try (Arena scope = Arena.ofConfined()) { | ||
| int groupnum = getgroups(0, MemorySegment.NULL); | ||
| if (groupnum == -1) { | ||
| throw new RuntimeException("getgroups returns " + groupnum); |
There was a problem hiding this comment.
Not a must-have, but would be nice to have a FFM for errno to provide better diagnostics.
There was a problem hiding this comment.
Good idea. There is a Linker.Option.captureCallState("errno") feature and I'll see how to use it.
There was a problem hiding this comment.
It looks like there has been prior work with this, at least with macos, e.g.: src/java.base/macosx/classes/jdk/internal/ffi/generated/errno/errno_h.java
| throw new RuntimeException("getgroups returns " + groupnum); | ||
| } | ||
|
|
||
| var gs = scope.allocate(gid_t, groupnum); |
There was a problem hiding this comment.
Should this be the size of gid_t * groupnum as the first argument and the byte alignment as the second?
There was a problem hiding this comment.
This is MemorySegment allocate(MemoryLayout elementLayout, long count) in SegmentAllocator.java.
| var gs = scope.allocate(gid_t, groupnum); | ||
| groupnum = getgroups(groupnum, gs); | ||
| if (groupnum == -1) { | ||
| throw new RuntimeException("getgroups returns " + groupnum); |
There was a problem hiding this comment.
Not a must-have, but would be nice to have a FFM for errno to provide better diagnostics.
| groups[i] = gs.getAtIndex(gid_t, i); | ||
| } | ||
|
|
||
| var resbuf = passwd.allocate(scope); |
There was a problem hiding this comment.
Wouldn't this be 1024 as well?
There was a problem hiding this comment.
This is the MemorySegment allocate(SegmentAllocator allocator) method in the generated passwd.java. If I understand correctly, the 2nd argument to getpwuid_r should be the pointer to a single passwd struct and the 3rd is to a big enough memory to hold contents of extra pointers inside passwd.
There was a problem hiding this comment.
Yes, ideally the 3rd argument would be the size of platform's maximum pwd struct buffer size, which at least can found found on linux and macos with _SC_GETPW_R_SIZE_MAX using sysconf. The sysconf FFM would not be a must-have, but would be a nice addition (along with errno) as part of the core libraries in the future.
| var resbuf = passwd.allocate(scope); | ||
| var pwd = scope.allocate(C_POINTER); | ||
| var pwd_buf = scope.allocate(1024); | ||
| int out = getpwuid_r(getuid(), resbuf, pwd_buf, pwd_buf.byteSize(), pwd); |
There was a problem hiding this comment.
nit: resbuf is usually name pwd, which is used to reference the passwd attributes in pwd_buf.
There was a problem hiding this comment.
I was using the old names in the C code. They could be confusing. I'll fix them.
| username = passwd.pw_name(resbuf).getString(0); | ||
| } catch (Throwable t) { | ||
| var error = new UnsatisfiedLinkError("FFM calls failed"); | ||
| error.initCause(t); |
There was a problem hiding this comment.
Do you want to check with getCause() here as well?
There was a problem hiding this comment.
getCause of t? I'll just pass the whole t back to the caller.
| * echo "#include <unistd.h> " > $HEADER_NAME | ||
| * echo "#include <spwd.h>" >> $HEADER_NAME | ||
| * | ||
| * |
There was a problem hiding this comment.
nit: extra line, but please ignore if jextract generated.
There was a problem hiding this comment.
Thanks for the comments. I've pushed a new commit that removes all jextract-generated code and go fully manual. Is this better?
BTW, I haven't studied the use of errorno yet. Also, I have a TODO item at the beginning of UnixLoginModule::login. Please tell me your opinion on my concern.
There was a problem hiding this comment.
Looks better, thank you.
Re: check for window platform earlier - yes, I think that this would be appropriate, but I haven't worked through how this would fallback to the existing windows implementation.
Re: other unix platforms - if FFM sysconf is implemented then allocation would not be left to chance on these systems. Regarding the pwd structure - I would be very surprised if the core attributes would be any different.
smemery
left a comment
smemery
left a comment
There was a problem hiding this comment.
BTW, I don't believe that this change warrants new benchmarks.
3 participants
Rewrite the native calls with FFM.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28931/head:pull/28931$ git checkout pull/28931Update a local copy of the PR:
$ git checkout pull/28931$ git pull https://git.openjdk.org/jdk.git pull/28931/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 28931View PR using the GUI difftool:
$ git pr show -t 28931Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28931.diff
Using Webrev
Link to Webrev Comment