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 RegConnectRegistry to Advapi32 mappings #935

Merged

Conversation

cxcorp
Copy link
Contributor

@cxcorp cxcorp commented Mar 5, 2018

Adds the RegConnectRegistry function mapping to Advapi32. This function is used to connect to remote machines' registries.

I added a test case for the function, but to be completely fair, I'm not 100% sure if the \\localhost UNC path works. The test case could be edited to pass in null for the name, but then the test doesn't guarantee that the first parameter is a string.

@matthiasblaesing
Copy link
Member

Thank you - in general this looks good. Two things I'd like to ask before a merge:

  • was this code tests in a real setting with a remote attach? If not, could you please do it?
  • the unittest looks sane - I assume it fails, if the remote registry service is not started - so if there is a separate error code, that indicates that, it might be worth checking for, if the error is unspecific, the test should be marked manual (See the SAFEARRAY tests, there is one I marked to be run only manually)

@cxcorp
Copy link
Contributor Author

cxcorp commented Mar 8, 2018

Hiya! Thanks for taking a look at the PR.

I just tested this on my localhost with the Remote Registry service started. It works as intended:

image

A user on StackOverflow also claimed (in a comment) that the mapping works.

As for the errors, it seems to return ERROR_BAD_NETPATH (0x35) if the remote service is disabled or if the path is just garbage. Doesn't seem to differentiate between the service being disabled and being unable to connect to the host. If attempting to use access a HKEY which is not one of the three supported ones, it returns ERROR_INVALID_HANDLE (0x6). I don't have an environment to test workgroup misconfigurations, lacking privileges or invalid credentials in, so I don't know which errors they might return. However, a Symantec support page I found suggests (in my opinion) that RegConnectRegistry may also return ACCESS_DENIED (0x5) if the user lacks privileges to the registry on the machine.

Should I make the test check for ERROR_BAD_NETPATH and leave a comment that it assumes remote registry is disabled?

@matthiasblaesing
Copy link
Member

Please adjust the unittest to check for ERROR_BAD_NETPATH and output a describing message to System.err, this way an interested party can see the result. This should not cause the unittest to fail.

Please also update CHANGES.md in the toplevel directory. You should add an entry to the features section for 5.0.0 describing your contribution. Have a look at the over entries and follow that pattern.

@matthiasblaesing
Copy link
Member

@cxcorp can I help you with the necessary changes?

@cxcorp
Copy link
Contributor Author

cxcorp commented Mar 28, 2018 via email

@matthiasblaesing
Copy link
Member

matthiasblaesing commented Mar 28, 2018 via email

@cxcorp
Copy link
Contributor Author

cxcorp commented Mar 30, 2018

Hiya, I made changes to the test, docs and CHANGES.md. Could you take a look?

@matthiasblaesing matthiasblaesing merged commit f99b613 into java-native-access:master Mar 31, 2018
@matthiasblaesing
Copy link
Member

Thank you - looks good and is merged to master.

@cxcorp cxcorp deleted the reg-connect-registry branch March 31, 2018 12:39
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.

2 participants