-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added QueryWorkingSetEx to PSAPI #1454
Conversation
We'll call my failed Builds and commits an fair trade for Author Amendments (And I say this because I feel bad about spamming your email), and not because my IDE wouldn't lint my code due to poor project configuration on my part. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for continuing to work this. A few more comments.
contrib/platform/test/com/sun/jna/platform/win32/PsapiTest.java
Outdated
Show resolved
Hide resolved
762b37d
to
3084361
Compare
@dbwiddis I believe that wraps up the changes. |
@Crain-32 Thanks, I'm on vacation but did a quick glance and it looks good. Could you also add an entry to the CHANGES.MD file similar to others where a new method was added? We don't have a regular release cycle, typically it's done when there have been substantive changes, so I wouldn't expect it for a couple of months. You can add this mapping to your own program temporarily while awaiting the next version, though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the change log, a few more comments.
* @return If the function succeeds, the return value is nonzero. | ||
* @see <a href="https://docs.microsoft.com/en-us/windows/win32/api/psapi/nf-psapi-queryworkingsetex">MSDN</a> | ||
*/ | ||
boolean QueryWorkingSetEx(HANDLE hProcess, PSAPI_WORKING_SET_EX_INFORMATION pv, DWORD cb); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pv
should be a Pointer
as the user is passing an array.
Also to simplify mappings for users, use int
for cb
rather than DWORD
.
@FieldOrder({"Flags", "Data"}) | ||
class PSAPI_WORKING_SET_EX_BLOCK extends Structure { | ||
public ULONG_PTR Flags; | ||
public ULONG[] Data = new ULONG[Native.POINTER_SIZE == 8 ? 1 : 2]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be an array of ULONG_PTR
.
for (int l = 0; l <= maskLength - rightShiftAmount; l++) { | ||
bitMask |= 1 << l; | ||
} | ||
return (int) ((innerValue >> rightShiftAmount) & bitMask); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't right, the bitmask is on the higher bits per lines 417-419, but you are shifting to the lowest bits before masking. You need to either:
- shift first and then mask the lowest "length" bits, or
- mask the specific bits and then shift. Using the
>>>
operator will save you the need to do special treatment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think this next iteration will be correct.
private int getBitFieldValue(final int maskLength, final int rightShiftAmount) {
long bitMask = 0;
// Having this Conditional be <= was going give the mask an extra bit.
for (int l = 0; l < maskLength; l++) {
bitMask |= 1 << l;
}
return (int) ((innerValue >>> rightShiftAmount) & bitMask);
}
Now if maskLength = 4
and rightShiftAmount = 3
innerValue = 101100111000 // Assume Leading Ones so the Unsigned Shift Prevents Sign Flipping
// Construct Mask
1111
// Shift
101100111000 >> 3 = 101100111
//Apply Mask
101100111 &
000001111
------------
000000111
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree if you are shifting first before applying the bitmask, then you are using a 0-indexed array and <
is appropriate. Ultimately you probably should have tested this (and I will when you commit) with some debug output to convince yourself it works.
MEMORY_BASIC_INFORMATION mbi = new MEMORY_BASIC_INFORMATION(); | ||
try { | ||
SIZE_T bytesRead = Kernel32.INSTANCE.VirtualQueryEx(selfHandle, Pointer.NULL, mbi, new SIZE_T(mbi.size())); | ||
assertTrue("It should read correctly", bytesRead.intValue() != 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's "It"? Let's make the messages stand alone. And this should be an assertNotEqual()
.
Looking good, getting closer. Haven't seen the commit for the changes that you thumbsed-up. In your next commit, we tend to like the change logs to reference the PR rather than the issue, and to include the full package path (with the prefixes abbreviated |
I have one comment: I'm surprised over the getters for the attributes of |
Sorry for not pushing my changes yet. I didn't get it as aligned as I wanted to, so I didn't push yet. Hoping to do that tonight. @matthiasblaesing It doesn't follow Java Bean Conventions, but from my perspective these would ideally be |
e27e18a
to
de4fd2e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, minor nit with = rather than - in the changelog.
I'll be away from internet for a few days so I'll let @matthiasblaesing decide about get*/set* or the record-style getters and whether they should be lowercased. I'm happy either way.
de4fd2e
to
d4ccd5c
Compare
JNA was created way before 17, compiles on 8 and is theoretically compatible with 6. Even is records came into consideration, they don't match, most of the JNA binds are mutable and it already a liability, that the public fields are used as carrier for the native values. That prevents moving from read-eagerly (when transitioning between native and java) to read-when-needed. Not sure if the original authors of JNA agree, but looking back, I would not map over public field and keep this implementation detail hidden. What is more many other bindings exists in the |
With respect, if you wish to lint the Function names using a convention not mentioned in the Contribution Doc, feel free to create a new PR that swaps them to your preferred convention. The PR's implementation works fine, and as the only person who has ever mentioned needing |
@Crain-32 it took you most probably longer to write a reply than simply adjusting the names according to normal java convention. For me it is a waste of time to discuss it and will fix the code afterwards. |
d4ccd5c
to
5f3b5e1
Compare
As no other classes in |
5f3b5e1
to
4ce1914
Compare
At the risk again stepping on toes. I don't see the value of moving the functions out, the original location looked good. They are tightly bound to the `object and only map the documented semantics of the structure. See for example these: jna/contrib/platform/src/com/sun/jna/platform/win32/Ddeml.java Lines 435 to 442 in 999c60a
jna/contrib/platform/src/com/sun/jna/platform/win32/Sspi.java Lines 626 to 633 in 999c60a
jna/contrib/platform/src/com/sun/jna/platform/win32/OaIdl.java Lines 246 to 280 in 999c60a
jna/contrib/platform/src/com/sun/jna/platform/win32/WinNT.java Lines 373 to 380 in 999c60a
jna/contrib/platform/src/com/sun/jna/platform/win32/WinCrypt.java Lines 354 to 370 in 999c60a
|
9f3961a
to
c184082
Compare
As the build issues are resolved, I'll write my final comments I have on this.
I thought the original location was good as well. However as per the Contribution Guideline changes today, I followed "the surrounding code" and removed all class functions. I personally don't see the value of Jean Bean Naming on Data Classes.
If you mean the function names didn't match what public record foo(Integer bar) {
public Integer retrieveBar() {
return this.bar(); // this is valid, and the expected function call to get the value.
}
}
Oh it did! Being asked to change perfectly fine function names to an unstated convention on a PR with a working Build is rather annoying. When people get annoyed they tend to do what you say instead of what you want. You said to follow conventions and the surrounding code, and I did that to the fullest I could. (From the Contribution PR Today)
As I mentioned in the second comment of the PR my IDE wasn't indexing anything for me. Your "simple request" is not right click and refactor the function for me, it's instead jumping between different files and doing Commits/Pushes to use your Pipeline to catch symbol errors for me. If my behavior comes off as unprofessional, please understand that my desire was to provide this function in JNA for when others need it, which from my perspective I had already achieved. The team has done a fantastic Job of making JNA expandable from outside the project, which I will keep in mind the next time I need something. With a working PR, I have no intentions of continuing to Iterate. If there are changes you'd like to see before merging, feel free to create a PR on my Fork. |
c184082
to
ea7fe7d
Compare
@dbwiddis please wait. The definition is not correct - I'm currently trying to create a real unittest and see if my modified bindings work. |
ea7fe7d
to
f0e8dcc
Compare
Hey @Crain-32, first off, thank you for your contribution so far. Even if you chose not to complete the PR, the work you did is helpful and your contributions are appreciated. As the number of active maintainers is small and we do this all on our volunteer time (and I'm popping in briefly from vacation during some down time) it's always helpful when the community can do more work.
JNA is a community-maintained project, and I myself have been annoyed at the lack of overall convention requirements (beyond some checkstyle tests). This is where the convention becomes "try to match how it's been done in the past as much as possible." Unstated, but at least somewhat consistent. Multiple other places in JNA when there are convenience methods for members of a class they use getters. Even in the bitfield example you cited in your research, there were (a lot more) getters. There are some benefits to the Java Bean notation. For example, Jackson's ObjectMapper can automatically assume field names with them, for one, which is very helpful when serializing/deserializing in a REST model. (Yes it can be worked around, but the "easy" assumption is generally better to follow.) Records are fun, but some of the newer and shinier stuff is going to be in JDK 21 with a whole new foreign function and memory interface that will probably be preferred to JNA for new projects, while JNA continues to support the old JDK 6+ world. Anyway, I pushed a commit going back to how I think was discussed. Leaving this PR for further review/discussion. |
@Crain-32 @dbwiddis: Based on an older commit from this branch I build a new PR, which from my perspective fixes issues found here. This started as a "just change the naming of the getters" change. But then I had a look at the data structure definitions again and found, that The change that drops Considering how to test this, I realized, that we need a way to change a single attribute from the memory properties. The easiest seemed to be the virtual memory lock. The corresponding function were not yet bound, which was fixed by: In the test cases a big problem was, that the structures were not read after the native call, so they basicly were no-ops. In both the positive and the negative case are checked and by locking the memory location and doing a second read it ensured, that the read is not some random value. |
But the documentation showed a union that clearly indicated ULONG_PTR covering a 64-bit field. WIth the variable size structure it's effectively the same thing for a single one, but with the windows docs mapping would consist of two. Although digging further it does indicate that second 32-bit field isn't there on 32-bit systems. More strange is that Flags is listed differently than the nameless union. So is the _INFORMATION structure 2 * POINTER_SIZE or 3 * POINTER_SIZE? |
This is the structure from the windows sdk headers: typedef union _PSAPI_WORKING_SET_EX_BLOCK {
ULONG_PTR Flags;
union {
struct {
ULONG_PTR Valid : 1;
ULONG_PTR ShareCount : 3;
ULONG_PTR Win32Protection : 11;
ULONG_PTR Shared : 1;
ULONG_PTR Node : 6;
ULONG_PTR Locked : 1;
ULONG_PTR LargePage : 1;
ULONG_PTR Reserved : 7;
ULONG_PTR Bad : 1;
#if defined(_WIN64)
ULONG_PTR ReservedUlong : 32;
#endif
};
struct {
ULONG_PTR Valid : 1; // Valid = 0 in this format.
ULONG_PTR Reserved0 : 14;
ULONG_PTR Shared : 1;
ULONG_PTR Reserved1 : 15;
ULONG_PTR Bad : 1;
#if defined(_WIN64)
ULONG_PTR ReservedUlong : 32;
#endif
} Invalid;
};
} PSAPI_WORKING_SET_EX_BLOCK, *PPSAPI_WORKING_SET_EX_BLOCK; What I see is this:
Does that make sense? |
I completely missed that _BLOCK was a union, and the headers I found didn’t include the |
Thanks for the contribution. Closing this PR as the change commits will be included in #1459. |
This MR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [net.java.dev.jna:jna](https://github.com/java-native-access/jna) | compile | minor | `5.12.1` -> `5.13.0` | --- ### Release Notes <details> <summary>java-native-access/jna</summary> ### [`v5.13.0`](https://github.com/java-native-access/jna/blob/HEAD/CHANGES.md#Release-5130) [Compare Source](java-native-access/jna@5.12.1...5.13.0) \================ ## Features - [#​1454](java-native-access/jna#1454): Add `c.s.j.p.win32.Psapi.QueryWorkingSetEx` and associated Types - [@​crain-32](https://github.com/Crain-32). - [#​1459](java-native-access/jna#1459): Add `VirtualLock` and `VirtualUnlock` in `c.s.j.p.win32.Kernel32` - [@​matthiasblaesing](https://github.com/matthiasblaesing). - [#​1471](java-native-access/jna#1471): Add `c.s.j.p.win32.Advapi32Util#isCurrentProcessElevated` and associated Types - [@​dbwiddis](https://github.com/dbwiddis). - [#​1474](java-native-access/jna#1474): Add `c.s.j.p.win32.WbemCli#IWbemClassObject.IWbemQualifierSet`, `IWbemServices.GetObject`, `IWbemContext.SetValue` and associated methods - [@​rchateauneu](https://github.com/rchateauneu). - [#​1482](java-native-access/jna#1482): Add multilingual support of `Kernel32Util.formatMessage` - [@​overpathz](https://github.com/overpathz). - [#​1490](java-native-access/jna#1490): Adds support for a custom `SymbolProvider` in `NativeLibrary` & `Library` - [@​soywiz](https://github.com/soywiz). - [#​1491](java-native-access/jna#1491): Update libffi to v3.4.4 - [@​matthiasblaesing](https://github.com/matthiasblaesing). - [#​1487](java-native-access/jna#1487): Add 'uses' information to OSGI metadata in MANIFEST.MF to improve stability of package resolution - [@​sratz](https://github.com/sratz). ## Bug Fixes - [#​1452](java-native-access/jna#1452): Fix memory allocation/handling for error message generation in native library code (`dispatch.c`) - [@​matthiasblaesing](https://github.com/matthiasblaesing). - [#​1460](java-native-access/jna#1460): Fix win32 variant date conversion in DST offest window and with millisecond values - [@​eranl](https://github.com/eranl). - [#​1472](java-native-access/jna#1472): Fix incorrect bitmask in `c.s.j.Pointer#createConstant(int)` - [@​dbwiddis](https://github.com/dbwiddis). - [#​1481](java-native-access/jna#1481): Fix NPE in NativeLibrary when unpacking from classpath is disabled - [@​trespasserw](https://github.com/trespasserw). - [#​1489](java-native-access/jna#1489): Fixes typo in `OpenGL32Util#wglGetProcAddress`, instead of parameter `procName` the hardcoded value `wglEnumGpusNV` was used - [@​soywiz](https://github.com/soywiz). </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR is behind base branch, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4yNC4wIiwidXBkYXRlZEluVmVyIjoiMzQuMjQuMCJ9-->
Added QueryWorkingSetEx to PSAPI