Skip to content
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

Add mapping for EnumProcesses to Psapi #1214

Closed

Conversation

T-Svensson
Copy link
Contributor

No description provided.

@T-Svensson T-Svensson force-pushed the Psapi.EnumProcesses branch 2 times, most recently from eadcb25 to 9c27512 Compare June 26, 2020 20:04
Copy link
Contributor

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your submission! I've left a few comments inline.

@T-Svensson
Copy link
Contributor Author

Please review again @dbwiddis, I think I covered all your comments.

Copy link
Contributor

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. A few more comments.

@T-Svensson
Copy link
Contributor Author

Since there was no additional comment on my questions above, I've uploaded a new changeset that addresses the other topics.

@dbwiddis
Copy link
Contributor

It looks generally good to me. I don't seem to have the powers (yet) to merge this so I'll let @matthiasblaesing take it from here, if he has any other changes.

@T-Svensson T-Svensson force-pushed the Psapi.EnumProcesses branch 2 times, most recently from 758a829 to 8895113 Compare June 29, 2020 18:53
@T-Svensson
Copy link
Contributor Author

I decided to add 2 more functions to the utility to extract the name of the process.
Looking forward to your feedback!

Copy link
Contributor

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience. A few more comments.

@T-Svensson
Copy link
Contributor Author

Unless there are any obvious errors in the last push, I'll consider this PR ready for merge.

Copy link
Contributor

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few last changes.

int[] pids = PsapiUtil.enumProcesses();

assertNotNull("List should not be null", pids);
assertTrue("List should contain my pid", Arrays.stream(pids).anyMatch(pid -> pid == myPid));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We retain compatibility with Java 1.6, so as neat as streams and lambdas are, you have to do it the old fashioned way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you aim to also have the test classes in Java6, I suggest that the Eclipse project is set to have Java6 as the target and the build.xml should also check ./test-classes, not just ./classes.
Anyway, I'll push a fix for this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are one or two exceptions in the test classes that require Java 8, which is why the project includes a higher compatibility level in the config files, but we otherwise maintain the standard.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I know this because I did the same thing not too long ago!)

int size = 0;
IntByReference lpdwSize = new IntByReference();
do {
size = size + 1024;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since WinDef.MAX_PATH should work in most cases, let's use that as the initial size. You can initialize on line 914 and increment (I'm ok with += 1024) after the query conditional.

Added utility method QueryFullProcessImageName that takes a PID.
Fixed QueryFullProcessImageName to properly handle paths longer than 260
characters that can happen on Win10 / paths with UNC.
@dbwiddis
Copy link
Contributor

dbwiddis commented Jul 1, 2020

Thanks for the updates. I'll commit this later today.

@dbwiddis
Copy link
Contributor

dbwiddis commented Jul 1, 2020

Merged in 22dc037

@dbwiddis dbwiddis closed this Jul 1, 2020
@T-Svensson T-Svensson deleted the Psapi.EnumProcesses branch July 11, 2020 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants