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

Android8 #148

Merged
merged 9 commits into from Sep 28, 2017

Conversation

Projects
None yet
6 participants
@user-none
Contributor

user-none commented Sep 13, 2017

As of Android 8 (Oreo) access to net.dns# has been removed (https://developer.android.com/about/versions/oreo/android-8.0-changes.html). The reasoning given is that it, "improves privacy on the platform". Currently c-ares uses this to get the list of DNS servers.

Now the only way to access the DNS server list is by using the Connectivity Manager though Java. This adds the necessary JNI code to use the Connectivity Manager and pull the DNS server list. The old way using __system_property_get with net.dns# remains for compatibilty.

Using the Connectivity Manager requires the ACCESS_NETWORK_STATE permission to be set on the app. Existing applications most likely are not setting this and keeping the previous method as a fallback will at the very least ensure those apps don't break on older versions of Android. They will need to add this permission for Android 8 compatibility.

Included in the patch are two initalization functions which are required. The JVM must be registered as well as the Connectivity Manager itself. There is no way to get the Connectivity Manager except though Java. Either being passed down to C directly or by passing in an Android Context which can be used to get the Connectivity Manager. Examples are provided in the documentation.

@user-none

This comment has been minimized.

Contributor

user-none commented Sep 13, 2017

Referencing issue #111

@bradh352 bradh352 requested review from bagder, daviddrysdale and bradh352 Sep 13, 2017

@bradh352

Looks good to me. Good man page.

#endif
#endif /* __JNI_COMMON_H__ */

This comment has been minimized.

@gjasny

gjasny Sep 13, 2017

Contributor

The header guard comment is not matching

@coveralls

This comment has been minimized.

coveralls commented Sep 13, 2017

Coverage Status

Coverage remained the same at 95.382% when pulling f9ee0ee on user-none:android8 into 9ef37fe on c-ares:master.

@coveralls

This comment has been minimized.

coveralls commented Sep 13, 2017

Coverage Status

Coverage remained the same at 95.382% when pulling 67d056b on user-none:android8 into 9ef37fe on c-ares:master.

@coveralls

This comment has been minimized.

coveralls commented Sep 14, 2017

Coverage Status

Coverage increased (+0.08%) to 95.46% when pulling 67d056b on user-none:android8 into 9ef37fe on c-ares:master.

@coveralls

This comment has been minimized.

coveralls commented Sep 14, 2017

Coverage Status

Coverage remained the same at 95.382% when pulling 67d056b on user-none:android8 into 9ef37fe on c-ares:master.

@coveralls

This comment has been minimized.

coveralls commented Sep 14, 2017

Coverage Status

Coverage increased (+0.08%) to 95.46% when pulling 67d056b on user-none:android8 into 9ef37fe on c-ares:master.

@bradh352

This comment has been minimized.

Member

bradh352 commented Sep 27, 2017

@bagder @daviddrysdale any feedback on this PR?

}
if (res != JNI_OK || env == NULL)
goto done;

This comment has been minimized.

@daviddrysdale

daviddrysdale Sep 28, 2017

Member

Idle thought: it might be helpful to add a block comment here with the equivalent Java code to the JNI stuff below.

/* 
JNI invocations below are intended to recreate the following Java snippet:
import android.net.ConnectivityManager
import android.net.LinkProperties
import java.util.List
import java.net.InetAddress
...
NetworkInfo active_network = connMgr.getActiveNetwork();
LinkProperties link_properties = connMgr.getLinkProperties(active_network);
List<InetAddress> server_list = props.getDnsServers();
for (int i = 0; i < server_list.size(); i++ {
  InetAddress server = server_list.get(i);
  String str = server.getHostAddress()
  // append str to results
}
*/
}
.Ed
.SH AVAILABILITY
This function was first introduced in c-ares version 1.?.?.

This comment has been minimized.

@daviddrysdale

daviddrysdale Sep 28, 2017

Member

I suspect we'll forget to come back and update this, so maybe fill it in as 1.14.0 as a best guess?

This comment has been minimized.

@bagder

bagder Sep 28, 2017

Member

Yes, please make a best guess to cover for us missing to update this later... =)

be present in the Android application.
.PP
Android older than 8 do not need to to be initalized as they
are less restritive. However, this is a run time not compile time

This comment has been minimized.

@daviddrysdale

daviddrysdale Sep 28, 2017

Member

"restrictive"

@daviddrysdale

This comment has been minimized.

Member

daviddrysdale commented Sep 28, 2017

Looks good as far as I can tell -- I don't really speak Java nor Android. It does look safe for not affecting any other platforms or existing code, which is nicely reassuring.

Thanks!

@bagder

This comment has been minimized.

Member

bagder commented Sep 28, 2017

I'll just second @daviddrysdale here as I don't speak any of those lingos either. It would be useful to get feedback from another actual Android user on this approach, but lacking that I think we can proceed with this PR as suggested.

@bagder

bagder approved these changes Sep 28, 2017

user-none added some commits Sep 28, 2017

@user-none

This comment has been minimized.

Contributor

user-none commented Sep 28, 2017

I corrected the typo and added the equivelent Java code as an example.

@bradh352 bradh352 merged commit 1abf13e into c-ares:master Sep 28, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@coveralls

This comment has been minimized.

coveralls commented Sep 28, 2017

Coverage Status

Coverage remained the same at 95.382% when pulling 6dfd6d6 on user-none:android8 into 9ef37fe on c-ares:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment