[Replicant] [PATCH] fix repwifi connection not reported to userspace apps. fix reverse-tether connection not reported to userspace apps.
Wolfgang Wiedmeyer
wreg at wiedmeyer.de
Tue May 23 18:49:14 UTC 2017
Hi,
Fil writes:
> Dear Replicant fellows,
>
> As reported on the tracker I've finished my work on issue #1792.
> Here-below is my very first patch to replicant (hurray!!!).
Thank you very much for working on this issue! It's great to see you
dive into the core Android code after your great work on the RepWifi
app! The USB networking part of the fix may also be interesting for
other Android-based distros.
> It is my very first patch to anything, to say the truth, so I just hope everything was done properly and I hope it works as expected.
> Please pardon me if I messed up with anything while packing up the patch.
This is a big patch and IMHO it fixes a quite complicated issue, so
congrats :)
I only did an initial review for now and I didn't yet test the
patch. I'd do this with a v2 then if you send one.
Did you generate the patch with git format-patch as described in the
developer guide[1]? Please include a commit message that shortly
describes what the problem is and how the patch fixes it. You can shorten
the title, e.g. "Report RepWifi and Reverse Tethering connections to
apps". If you have issues with git send-email, you can also paste the
patch in the mail. It's only important to have a patch that can be
applied with git am.
This may sound annoying and pedantic, but please try to use the same
coding style rules as the surrounding code. It helps a lot with reading
and understanding the code. The Android documentation has a section
about the rules[2]. It doesn't have to be perfect and it looks like you
already adhered to most of it in most parts, but consistency is missing
in other parts. I noticed that the indentation is off with too many
spaces and spaces are missing before and after braces, e.g. "} else {"
instead of "}else{" or "class MyClass {" instead of "class
MyClass{". You might also want to check the style rules for comments and
unneeded empty lines. There is some unnecessary whitespace. You should
see this as red bars when viewing the diff of your changes, e.g. with
git diff.
I added further questions and suggestions below.
> Here it is:
>
> ---
> api/current.txt | 2 +
> api/system-current.txt | 2 +
> core/java/android/net/NetworkInfo.java | 268 +++++++++++++++++++++
> .../com/android/server/ConnectivityService.java | 11 +
> 4 files changed, 283 insertions(+)
>
> diff --git a/api/current.txt b/api/current.txt
> index c45cc3dbbf4..2a3ab95c4bf 100644
> --- a/api/current.txt
> +++ b/api/current.txt
> @@ -18372,9 +18372,11 @@ package android.net {
> method public android.net.NetworkInfo.DetailedState getDetailedState();
> method public java.lang.String getExtraInfo();
> method public java.lang.String getReason();
> + method public static android.net.NetworkInfo getRepWifiNetworkInfo();
> method public android.net.NetworkInfo.State getState();
> method public int getSubtype();
> method public java.lang.String getSubtypeName();
> + method public static android.net.NetworkInfo getTetherNetworkInfo();
> method public int getType();
> method public java.lang.String getTypeName();
> method public boolean isAvailable();
> diff --git a/api/system-current.txt b/api/system-current.txt
> index 1e7b94e883e..12f87e9d2a6 100644
> --- a/api/system-current.txt
> +++ b/api/system-current.txt
> @@ -19885,9 +19885,11 @@ package android.net {
> method public android.net.NetworkInfo.DetailedState getDetailedState();
> method public java.lang.String getExtraInfo();
> method public java.lang.String getReason();
> + method public static android.net.NetworkInfo getRepWifiNetworkInfo();
> method public android.net.NetworkInfo.State getState();
> method public int getSubtype();
> method public java.lang.String getSubtypeName();
> + method public static android.net.NetworkInfo getTetherNetworkInfo();
> method public int getType();
> method public java.lang.String getTypeName();
> method public boolean isAvailable();
Please try to avoid changing the public API. This can have some
unintended consequences and could break things. You probably saw the big
warning message. It may be fine in your case as you don't change
existing methods and only add new ones, but it still could cause
issues. It looks like getTetherNetworkInfo() and getRepWifiNetworkInfo()
are only called once. In the worst case, would it at least be possible
to move them to the class they are called from (ConnectivityService)?
> diff --git a/core/java/android/net/NetworkInfo.java b/core/java/android/net/NetworkInfo.java
> index af7a4658808..8f61c83d989 100644
> --- a/core/java/android/net/NetworkInfo.java
> +++ b/core/java/android/net/NetworkInfo.java
> @@ -21,6 +21,9 @@ import android.os.Parcel;
>
> import com.android.internal.annotations.VisibleForTesting;
>
> +import java.io.IOException;
> +import java.io.InputStream;
> +import java.util.ArrayList;
> import java.util.EnumMap;
>
> /**
> @@ -471,4 +474,269 @@ public class NetworkInfo implements Parcelable {
> return new NetworkInfo[size];
> }
> };
> +
> + // ****** Extension methods for RepWifi **********
> + // Introduced by Fil Bergamo, 2017-05-16.
> + //
No need to add this here. This is what Git is for. It takes care of
authorship and date of your changes. Git blame can also be used to check
attribution over time. You can also remove the corresponding end
statement and the same statements in the ConnectivityService class.
> + public static NetworkInfo getRepWifiNetworkInfo(){
> + return RepWifiConnection.getNetworkInfo(RepWifiConnection.INTERFACE_NAME_WIFI);
> + }
> +
> + public static NetworkInfo getTetherNetworkInfo(){
> + return RepWifiConnection.getNetworkInfo(RepWifiConnection.INTERFACE_NAME_TETHER);
> + }
> +
> + static class RepWifiConnection{
> +
> + static final String INTERFACE_NAME_WIFI = "wlan0";
> + static final String INTERFACE_NAME_TETHER = "rndis0";
> +
> + static class ShellCommand {
> +
> + protected String _cmdOut = "";
> + protected String _cmdTxt = "";
> +
> + public ShellCommand(String commandText) {
> + this._cmdTxt = commandText;
> + }
> +
> + public int execute() throws Exception {
> +
> + if (this._cmdTxt == null) {
> + return -9;
Why return -9 here?
> + }
> +
> + java.lang.Process cmd = Runtime.getRuntime().exec(this._cmdTxt);
Are all commands then run as root?
> +
> + InputStream os = cmd.getInputStream();
> + InputStream es = cmd.getErrorStream();
> +
> + StringBuilder sb = new StringBuilder();
> +
> + sb.append(getStringFromStream(es));
> + sb.append(getStringFromStream(os));
> +
> + int res = cmd.waitFor();
> +
> + // re-read the output, in case it was empty when first tried
> + sb.append(getStringFromStream(es));
> + sb.append(getStringFromStream(os));
> +
> + this._cmdOut = sb.toString();
> +
> + return res;
> +
> + }
> +
> + protected String getStringFromStream(InputStream s) throws IOException {
> +
> + StringBuilder sb = new StringBuilder();
> + while ((s.available() > 0)) {
> + int b = s.read();
> + if (b >= 0) {
> + sb.append((char) b);
> + } else {
> + break;
> + }
> + }
> +
> + return sb.toString();
> +
> + }
> +
> + public String getOutput() {
> + return this._cmdOut;
> + }
> +
> + }
Are you sure that you need to introduce all these methods for shell
commands and string manipulation? Did you check if there are already
similar methods available that you could import and use instead? This
would make the patch smaller and possibly avoid bugs.
> + public static NetworkInfo getNetworkInfo(String interfaceName){
> +
> + NetworkInfo netInfo;
> + if (interfaceName.equals(INTERFACE_NAME_WIFI)){
> + netInfo = new NetworkInfo(ConnectivityManager.TYPE_WIFI, 0, "WIFI", "");
> +
> + }else if (interfaceName.equals(INTERFACE_NAME_TETHER)){
> + netInfo = new NetworkInfo(ConnectivityManager.TYPE_ETHERNET, 0,"ETHERNET", "");
> +
> + }else{
> + return null;
> + }
> +
> + netInfo.mState = getState(interfaceName);
> + netInfo.mDetailedState = getDetailedState(interfaceName);
> + netInfo.mIsAvailable = isAvailable(interfaceName);
> + return netInfo;
> +
> + }
> +
> + public static boolean isAvailable(String interfaceName){
> +
> + String[] ifaces = getAvailableInterfaces();
> + if (ifaces == null){
> + return false;
> + }
> +
> + if (ifaces.length == 0){
> + return false;
> + }
You can make this shorter with
if (ifaces == null || ifaces.length == 0) {
> + for (String i : ifaces){
> + if (i.equals(interfaceName)){
> + return true;
> + }
> + }
> +
> + return false;
> +
> + }
> +
> + public static State getState(String interfaceName){
> +
> + if (interfaceName.equals(INTERFACE_NAME_WIFI) && (! isWpaSupplicantRunning())){
> + return State.DISCONNECTED;
> + }
> +
> + String gw = getGateway(interfaceName);
> + if (gw == null){
> + return State.DISCONNECTED;
> + }
> + else{
> + //assuming the following:
> + //if there's a gateway set for the interface,
> + //then there's a route for the interface,
> + //then RepWifi or reverse-tether must have established that route,
> + //then RepWifi or reverse-tether is connected.
> + return State.CONNECTED;
> + }
> +
> + }
> +
> + public static DetailedState getDetailedState(String interfaceName){
> +
> + State currentState = getState(interfaceName);
> + if (currentState == State.CONNECTED){
> + return DetailedState.CONNECTED;
> + }
> + else{
> + return DetailedState.DISCONNECTED;
> + }
> +
> + }
> +
> + private static boolean isWpaSupplicantRunning(){
> +
> + boolean retval = false;
> +
> + try {
> +
> + ShellCommand su = new ShellCommand("pidof wpa_supplicant");
> + if (su.execute() == 0){
> +
> + if (su.getOutput().trim().equals("")){
> + retval = false;
> + }else{
> + retval = true;
> + }
> +
> + }else {
> + retval = false;
> + }
> +
> +
> + } catch (Exception e) {
> + retval = false;
Can't you directly return true or false instead of only setting retval?
It looks to me like retval is not needed here.
> + }
> +
> + return retval;
> +
> + }
Is there no method for checking if a service is running and that doesn't
need root?
> + protected static String getGateway(String interfaceName){
> +
> + try {
> +
> + //doesn't need root (tested)
> + ShellCommand cmd = new ShellCommand("ip route show dev " + interfaceName);
> + if (cmd.execute() != 0){
> + return null;
> + }
> +
> + //read command output
> + String out = cmd.getOutput();
> + if (out == null){
> + return null;
> + }
> +
> + String[] lines = out.split("\n");
> +
> + for (String l : lines){
> +
> + if (l.contains("default via")){
> +
> + String[] f = l.split(" ");
> + if (f.length > 2){
> +
> + //found route's address:
> + return f[2];
> +
> + }
> + }
> + }
> +
> + return null;
> +
> + } catch (Exception e) {
> + return null;
> + }
> +
> + }
> +
> + protected static String[] getAvailableInterfaces(){
> +
> + try {
> +
> + // No need for root for "ip link"
> + // tested 2017-03-24 - Fil
> + ShellCommand cmd = new ShellCommand("ip link");
> + if (cmd.execute() == 0){
> +
> + String out = cmd.getOutput();
> + if (out == null || out.contains("\n") == false){
> + return null;
> + }
> +
> + ArrayList<String> list = new ArrayList<String>();
> +
> + String[] lines = out.split("\n");
> + for (String l : lines){
> +
> + String[] fields = l.split(":");
> + if (fields.length != 3){
> + continue;
> + }
> +
> + String interfName = fields[1].trim();
> + list.add(interfName);
> +
> + }
> +
> + String[] retArr = new String[list.size()];
> + retArr = list.toArray(retArr);
> +
> + return retArr;
> +
> + }
> + else{
> + return null;
> + }
> +
> + } catch (Exception e) {
> + return null;
> + }
> +
> + }
> +
> + }
> + //***** End of Extension methods for RepWifi***************
> }
> diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java
> index 1489fd8713a..bf7165682fe 100644
> --- a/services/core/java/com/android/server/ConnectivityService.java
> +++ b/services/core/java/com/android/server/ConnectivityService.java
> @@ -1017,6 +1017,17 @@ public class ConnectivityService extends IConnectivityManager.Stub
> final int uid = Binder.getCallingUid();
> NetworkState state = getUnfilteredActiveNetworkState(uid);
> NetworkInfo ni = getFilteredNetworkInfo(state.networkInfo, state.linkProperties, uid);
> +
> + //***** Begin Extension for RepWifi *******
> + //introduced by Fil Bergamo 2017-05-16
> + if (ni == null){
> + ni = NetworkInfo.getRepWifiNetworkInfo();
> + }
> + if (ni == null || ni.isAvailable() == false || ni.getState() != NetworkInfo.State.CONNECTED){
> + ni = NetworkInfo.getTetherNetworkInfo();
> + }
> + //***** End of Extension for RepWifi. *****
> +
> maybeLogBlockedNetworkInfo(ni, uid);
> return ni;
> }
Looks like a good solution as long as there is no HAL implementation we
could use instead or a solution that fixes it at lower levels with the
system services, as you wrote. I'd assume that these methods aren't called
often by apps and it doesn't make things noticeable slower.
Best regards,
Wolfgang
[1] https://redmine.replicant.us/projects/replicant/wiki/DeveloperGuide#Code-hosting-and-submitting-patches
[2] https://source.android.com/source/code-style#java-style-rules
--
Website: https://fossencdi.org
OpenPGP: 0F30 D1A0 2F73 F70A 6FEE 048E 5816 A24C 1075 7FC4
Key download: https://wiedmeyer.de/keys/ww.asc
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.osuosl.org/pipermail/replicant/attachments/20170523/508c92c4/attachment.asc>
More information about the Replicant
mailing list