OSDN Git Service

Frameworks: Clean up SystemProperties
authorAndreas Gampe <agampe@google.com>
Sat, 29 Jul 2017 21:14:39 +0000 (14:14 -0700)
committerAndreas Gampe <agampe@google.com>
Thu, 31 Aug 2017 01:37:48 +0000 (18:37 -0700)
Clean up SystemProperties.java. Add annotations.

Clean up SystemProperties.cpp. Refactor for proper C++11.
Make sure C-string key construction is properly reused. Use
android::base functionality for actual reading.

Fix the test script to refer to the right location. Add some
test coverage.

(cherry picked from commit 2e6b9cb56320a86f0c33da890f667e5c76c8285d)

Test: m
Test: frameworks/base/core/tests/systemproperties/run_core_systemproperties_test.sh --rebuild
Merged-In: I490577370da985f600fb1117e3c818d3f68bad5f
Change-Id: I490577370da985f600fb1117e3c818d3f68bad5f

core/java/android/os/SystemProperties.java
core/jni/android_os_SystemProperties.cpp
core/tests/systemproperties/run_core_systemproperties_test.sh
core/tests/systemproperties/src/android/os/SystemPropertiesTest.java

index 5a79a54..560b4b3 100644 (file)
@@ -16,6 +16,8 @@
 
 package android.os;
 
+import android.annotation.NonNull;
+import android.annotation.Nullable;
 import android.util.Log;
 import android.util.MutableInt;
 
@@ -43,17 +45,12 @@ public class SystemProperties {
 
     public static final int PROP_VALUE_MAX = 91;
 
+    @GuardedBy("sChangeCallbacks")
     private static final ArrayList<Runnable> sChangeCallbacks = new ArrayList<Runnable>();
 
     @GuardedBy("sRoReads")
-    private static final HashMap<String, MutableInt> sRoReads;
-    static {
-        if (TRACK_KEY_ACCESS) {
-            sRoReads = new HashMap<>();
-        } else {
-            sRoReads = null;
-        }
-    }
+    private static final HashMap<String, MutableInt> sRoReads =
+            TRACK_KEY_ACCESS ? new HashMap<>() : null;
 
     private static void onKeyAccess(String key) {
         if (!TRACK_KEY_ACCESS) return;
@@ -90,9 +87,11 @@ public class SystemProperties {
      * <b>WARNING:</b> Do not use this method if the value may not be a valid UTF string! This
      * method will crash in native code.
      *
+     * @param key the key to lookup
      * @return an empty string if the {@code key} isn't found
      */
-    public static String get(String key) {
+    @NonNull
+    public static String get(@NonNull String key) {
         if (TRACK_KEY_ACCESS) onKeyAccess(key);
         return native_get(key);
     }
@@ -103,68 +102,82 @@ public class SystemProperties {
      * <b>WARNING:</b> Do not use this method if the value may not be a valid UTF string! This
      * method will crash in native code.
      *
+     * @param key the key to lookup
+     * @param def the default value in case the property is not set or empty
      * @return if the {@code key} isn't found, return {@code def} if it isn't null, or an empty
      * string otherwise
      */
-    public static String get(String key, String def) {
+    @NonNull
+    public static String get(@NonNull String key, @Nullable String def) {
         if (TRACK_KEY_ACCESS) onKeyAccess(key);
         return native_get(key, def);
     }
 
     /**
-     * Get the value for the given key, and return as an integer.
+     * Get the value for the given {@code key}, and return as an integer.
+     *
      * @param key the key to lookup
      * @param def a default value to return
      * @return the key parsed as an integer, or def if the key isn't found or
      *         cannot be parsed
      */
-    public static int getInt(String key, int def) {
+    public static int getInt(@NonNull String key, int def) {
         if (TRACK_KEY_ACCESS) onKeyAccess(key);
         return native_get_int(key, def);
     }
 
     /**
-     * Get the value for the given key, and return as a long.
+     * Get the value for the given {@code key}, and return as a long.
+     *
      * @param key the key to lookup
      * @param def a default value to return
      * @return the key parsed as a long, or def if the key isn't found or
      *         cannot be parsed
      */
-    public static long getLong(String key, long def) {
+    public static long getLong(@NonNull String key, long def) {
         if (TRACK_KEY_ACCESS) onKeyAccess(key);
         return native_get_long(key, def);
     }
 
     /**
-     * Get the value for the given key, returned as a boolean.
+     * Get the value for the given {@code key}, returned as a boolean.
      * Values 'n', 'no', '0', 'false' or 'off' are considered false.
      * Values 'y', 'yes', '1', 'true' or 'on' are considered true.
      * (case sensitive).
      * If the key does not exist, or has any other value, then the default
      * result is returned.
+     *
      * @param key the key to lookup
      * @param def a default value to return
      * @return the key parsed as a boolean, or def if the key isn't found or is
      *         not able to be parsed as a boolean.
      */
-    public static boolean getBoolean(String key, boolean def) {
+    public static boolean getBoolean(@NonNull String key, boolean def) {
         if (TRACK_KEY_ACCESS) onKeyAccess(key);
         return native_get_boolean(key, def);
     }
 
     /**
-     * Set the value for the given key.
-     * @throws IllegalArgumentException if the value exceeds 92 characters
+     * Set the value for the given {@code key} to {@code val}.
+     *
+     * @throws IllegalArgumentException if the {@code val} exceeds 91 characters
      */
-    public static void set(String key, String val) {
+    public static void set(@NonNull String key, @Nullable String val) {
         if (val != null && val.length() > PROP_VALUE_MAX) {
-            throw newValueTooLargeException(key, val);
+            throw new IllegalArgumentException("value of system property '" + key
+                    + "' is longer than " + PROP_VALUE_MAX + " characters: " + val);
         }
         if (TRACK_KEY_ACCESS) onKeyAccess(key);
         native_set(key, val);
     }
 
-    public static void addChangeCallback(Runnable callback) {
+    /**
+     * Add a callback that will be run whenever any system property changes.
+     *
+     * @param callback The {@link Runnable} that should be executed when a system property
+     * changes.
+     */
+    public static void addChangeCallback(@NonNull Runnable callback) {
         synchronized (sChangeCallbacks) {
             if (sChangeCallbacks.size() == 0) {
                 native_add_change_callback();
@@ -173,7 +186,8 @@ public class SystemProperties {
         }
     }
 
-    static void callChangeCallbacks() {
+    @SuppressWarnings("unused")  // Called from native code.
+    private static void callChangeCallbacks() {
         synchronized (sChangeCallbacks) {
             //Log.i("foo", "Calling " + sChangeCallbacks.size() + " change callbacks!");
             if (sChangeCallbacks.size() == 0) {
@@ -186,11 +200,6 @@ public class SystemProperties {
         }
     }
 
-    private static IllegalArgumentException newValueTooLargeException(String key, String value) {
-        return new IllegalArgumentException("value of system property '" + key + "' is longer than "
-                + PROP_VALUE_MAX + " characters: " + value);
-    }
-
     /*
      * Notifies listeners that a system property has changed
      */
index 8844fb0..a94cac0 100644 (file)
 
 #define LOG_TAG "SysPropJNI"
 
+#include "android-base/logging.h"
+#include "android-base/properties.h"
 #include "cutils/properties.h"
 #include "utils/misc.h"
 #include <utils/Log.h>
 #include "jni.h"
 #include "core_jni_helpers.h"
 #include <nativehelper/JNIHelp.h>
+#include <nativehelper/ScopedPrimitiveArray.h>
+#include <nativehelper/ScopedUtfChars.h>
 
 namespace android
 {
 
-static jstring SystemProperties_getSS(JNIEnv *env, jobject clazz,
-                                      jstring keyJ, jstring defJ)
-{
-    int len;
-    const char* key;
-    char buf[PROPERTY_VALUE_MAX];
-    jstring rvJ = NULL;
-
-    if (keyJ == NULL) {
-        jniThrowNullPointerException(env, "key must not be null.");
-        goto error;
-    }
-
-    key = env->GetStringUTFChars(keyJ, NULL);
-
-    len = property_get(key, buf, "");
-    if ((len <= 0) && (defJ != NULL)) {
-        rvJ = defJ;
-    } else if (len >= 0) {
-        rvJ = env->NewStringUTF(buf);
-    } else {
-        rvJ = env->NewStringUTF("");
+namespace {
+
+template <typename T, typename Handler>
+T ConvertKeyAndForward(JNIEnv *env, jstring keyJ, T defJ, Handler handler) {
+    std::string key;
+    {
+        // Scope the String access. If the handler can throw an exception,
+        // releasing the string characters late would trigger an abort.
+        ScopedUtfChars key_utf(env, keyJ);
+        if (key_utf.c_str() == nullptr) {
+            return defJ;
+        }
+        key = key_utf.c_str();  // This will make a copy, but we can't avoid
+                                // with the existing interface in
+                                // android::base.
     }
-
-    env->ReleaseStringUTFChars(keyJ, key);
-
-error:
-    return rvJ;
+    return handler(key, defJ);
 }
 
-static jstring SystemProperties_getS(JNIEnv *env, jobject clazz,
-                                      jstring keyJ)
+jstring SystemProperties_getSS(JNIEnv *env, jclass clazz, jstring keyJ,
+                               jstring defJ)
 {
-    return SystemProperties_getSS(env, clazz, keyJ, NULL);
+    // Using ConvertKeyAndForward is sub-optimal for copying the key string,
+    // but improves reuse and reasoning over code.
+    auto handler = [&](const std::string& key, jstring defJ) {
+        std::string prop_val = android::base::GetProperty(key, "");
+        if (!prop_val.empty()) {
+            return env->NewStringUTF(prop_val.c_str());
+        };
+        if (defJ != nullptr) {
+            return defJ;
+        }
+        // This function is specified to never return null (or have an
+        // exception pending).
+        return env->NewStringUTF("");
+    };
+    return ConvertKeyAndForward(env, keyJ, defJ, handler);
 }
 
-static jint SystemProperties_get_int(JNIEnv *env, jobject clazz,
-                                      jstring keyJ, jint defJ)
+jstring SystemProperties_getS(JNIEnv *env, jclass clazz, jstring keyJ)
 {
-    int len;
-    const char* key;
-    char buf[PROPERTY_VALUE_MAX];
-    char* end;
-    jint result = defJ;
-
-    if (keyJ == NULL) {
-        jniThrowNullPointerException(env, "key must not be null.");
-        goto error;
-    }
-
-    key = env->GetStringUTFChars(keyJ, NULL);
-
-    len = property_get(key, buf, "");
-    if (len > 0) {
-        result = strtol(buf, &end, 0);
-        if (end == buf) {
-            result = defJ;
-        }
-    }
-
-    env->ReleaseStringUTFChars(keyJ, key);
-
-error:
-    return result;
+    return SystemProperties_getSS(env, clazz, keyJ, nullptr);
 }
 
-static jlong SystemProperties_get_long(JNIEnv *env, jobject clazz,
-                                      jstring keyJ, jlong defJ)
+template <typename T>
+T SystemProperties_get_integral(JNIEnv *env, jclass, jstring keyJ,
+                                       T defJ)
 {
-    int len;
-    const char* key;
-    char buf[PROPERTY_VALUE_MAX];
-    char* end;
-    jlong result = defJ;
-
-    if (keyJ == NULL) {
-        jniThrowNullPointerException(env, "key must not be null.");
-        goto error;
-    }
-
-    key = env->GetStringUTFChars(keyJ, NULL);
-
-    len = property_get(key, buf, "");
-    if (len > 0) {
-        result = strtoll(buf, &end, 0);
-        if (end == buf) {
-            result = defJ;
-        }
-    }
-
-    env->ReleaseStringUTFChars(keyJ, key);
-
-error:
-    return result;
+    auto handler = [](const std::string& key, T defV) {
+        return android::base::GetIntProperty<T>(key, defV);
+    };
+    return ConvertKeyAndForward(env, keyJ, defJ, handler);
 }
 
-static jboolean SystemProperties_get_boolean(JNIEnv *env, jobject clazz,
-                                      jstring keyJ, jboolean defJ)
+jboolean SystemProperties_get_boolean(JNIEnv *env, jclass, jstring keyJ,
+                                      jboolean defJ)
 {
-    int len;
-    const char* key;
-    char buf[PROPERTY_VALUE_MAX];
-    jboolean result = defJ;
-
-    if (keyJ == NULL) {
-        jniThrowNullPointerException(env, "key must not be null.");
-        goto error;
-    }
-
-    key = env->GetStringUTFChars(keyJ, NULL);
-
-    len = property_get(key, buf, "");
-    if (len == 1) {
-        char ch = buf[0];
-        if (ch == '0' || ch == 'n')
-            result = false;
-        else if (ch == '1' || ch == 'y')
-            result = true;
-    } else if (len > 1) {
-         if (!strcmp(buf, "no") || !strcmp(buf, "false") || !strcmp(buf, "off")) {
-            result = false;
-        } else if (!strcmp(buf, "yes") || !strcmp(buf, "true") || !strcmp(buf, "on")) {
-            result = true;
-        }
-    }
-
-    env->ReleaseStringUTFChars(keyJ, key);
-
-error:
-    return result;
+    auto handler = [](const std::string& key, jboolean defV) -> jboolean {
+        bool result = android::base::GetBoolProperty(key, defV);
+        return result ? JNI_TRUE : JNI_FALSE;
+    };
+    return ConvertKeyAndForward(env, keyJ, defJ, handler);
 }
 
-static void SystemProperties_set(JNIEnv *env, jobject clazz,
-                                      jstring keyJ, jstring valJ)
+void SystemProperties_set(JNIEnv *env, jobject clazz, jstring keyJ,
+                          jstring valJ)
 {
-    int err;
-    const char* key;
-    const char* val;
-
-    if (keyJ == NULL) {
-        jniThrowNullPointerException(env, "key must not be null.");
-        return ;
-    }
-    key = env->GetStringUTFChars(keyJ, NULL);
-
-    if (valJ == NULL) {
-        val = "";       /* NULL pointer not allowed here */
-    } else {
-        val = env->GetStringUTFChars(valJ, NULL);
-    }
-
-    err = property_set(key, val);
-
-    env->ReleaseStringUTFChars(keyJ, key);
-
-    if (valJ != NULL) {
-        env->ReleaseStringUTFChars(valJ, val);
-    }
-
-    if (err < 0) {
+    auto handler = [&](const std::string& key, bool) {
+        std::string val;
+        if (valJ != nullptr) {
+            ScopedUtfChars key_utf(env, valJ);
+            val = key_utf.c_str();
+        }
+        return android::base::SetProperty(key, val);
+    };
+    if (!ConvertKeyAndForward(env, keyJ, true, handler)) {
+        // Must have been a failure in SetProperty.
         jniThrowException(env, "java/lang/RuntimeException",
                           "failed to set system property");
     }
 }
 
-static JavaVM* sVM = NULL;
-static jclass sClazz = NULL;
-static jmethodID sCallChangeCallbacks;
+JavaVM* sVM = nullptr;
+jclass sClazz = nullptr;
+jmethodID sCallChangeCallbacks;
 
-static void do_report_sysprop_change() {
+void do_report_sysprop_change() {
     //ALOGI("Java SystemProperties: VM=%p, Clazz=%p", sVM, sClazz);
-    if (sVM != NULL && sClazz != NULL) {
+    if (sVM != nullptr && sClazz != nullptr) {
         JNIEnv* env;
         if (sVM->GetEnv((void **)&env, JNI_VERSION_1_4) >= 0) {
             //ALOGI("Java SystemProperties: calling %p", sCallChangeCallbacks);
@@ -207,47 +128,49 @@ static void do_report_sysprop_change() {
     }
 }
 
-static void SystemProperties_add_change_callback(JNIEnv *env, jobject clazz)
+void SystemProperties_add_change_callback(JNIEnv *env, jobject clazz)
 {
     // This is called with the Java lock held.
-    if (sVM == NULL) {
+    if (sVM == nullptr) {
         env->GetJavaVM(&sVM);
     }
-    if (sClazz == NULL) {
+    if (sClazz == nullptr) {
         sClazz = (jclass) env->NewGlobalRef(clazz);
         sCallChangeCallbacks = env->GetStaticMethodID(sClazz, "callChangeCallbacks", "()V");
         add_sysprop_change_callback(do_report_sysprop_change, -10000);
     }
 }
 
-static void SystemProperties_report_sysprop_change(JNIEnv /**env*/, jobject /*clazz*/)
+void SystemProperties_report_sysprop_change(JNIEnv /**env*/, jobject /*clazz*/)
 {
     report_sysprop_change();
 }
 
-static const JNINativeMethod method_table[] = {
-    { "native_get", "(Ljava/lang/String;)Ljava/lang/String;",
-      (void*) SystemProperties_getS },
-    { "native_get", "(Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;",
-      (void*) SystemProperties_getSS },
-    { "native_get_int", "(Ljava/lang/String;I)I",
-      (void*) SystemProperties_get_int },
-    { "native_get_long", "(Ljava/lang/String;J)J",
-      (void*) SystemProperties_get_long },
-    { "native_get_boolean", "(Ljava/lang/String;Z)Z",
-      (void*) SystemProperties_get_boolean },
-    { "native_set", "(Ljava/lang/String;Ljava/lang/String;)V",
-      (void*) SystemProperties_set },
-    { "native_add_change_callback", "()V",
-      (void*) SystemProperties_add_change_callback },
-    { "native_report_sysprop_change", "()V",
-      (void*) SystemProperties_report_sysprop_change },
-};
+}  // namespace
 
 int register_android_os_SystemProperties(JNIEnv *env)
 {
-    return RegisterMethodsOrDie(env, "android/os/SystemProperties", method_table,
-                                NELEM(method_table));
+    const JNINativeMethod method_table[] = {
+        { "native_get", "(Ljava/lang/String;)Ljava/lang/String;",
+          (void*) SystemProperties_getS },
+        { "native_get",
+          "(Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;",
+          (void*) SystemProperties_getSS },
+        { "native_get_int", "(Ljava/lang/String;I)I",
+          (void*) SystemProperties_get_integral<jint> },
+        { "native_get_long", "(Ljava/lang/String;J)J",
+          (void*) SystemProperties_get_integral<jlong> },
+        { "native_get_boolean", "(Ljava/lang/String;Z)Z",
+          (void*) SystemProperties_get_boolean },
+        { "native_set", "(Ljava/lang/String;Ljava/lang/String;)V",
+          (void*) SystemProperties_set },
+        { "native_add_change_callback", "()V",
+          (void*) SystemProperties_add_change_callback },
+        { "native_report_sysprop_change", "()V",
+          (void*) SystemProperties_report_sysprop_change },
+    };
+    return RegisterMethodsOrDie(env, "android/os/SystemProperties",
+                                method_table, NELEM(method_table));
 }
 
 };
index d39adbb..9b1fe4b 100755 (executable)
@@ -15,7 +15,7 @@ fi
 
 if [[ $rebuild == true ]]; then
   make -j4 FrameworksCoreSystemPropertiesTests
-  TESTAPP=${ANDROID_PRODUCT_OUT}/data/app/FrameworksCoreSystemPropertiesTests.apk
+  TESTAPP=${ANDROID_PRODUCT_OUT}/data/app/FrameworksCoreSystemPropertiesTests/FrameworksCoreSystemPropertiesTests.apk
   COMMAND="adb install -r $TESTAPP"
   echo $COMMAND
   $COMMAND
index 544a967..282b001 100644 (file)
@@ -51,6 +51,11 @@ public class SystemPropertiesTest extends TestCase {
         value = SystemProperties.get(KEY, "default");
         assertEquals("default", value);
 
+        // null default value is the same as "".
+        SystemProperties.set(KEY, null);
+        value = SystemProperties.get(KEY, "default");
+        assertEquals("default", value);
+
         SystemProperties.set(KEY, "SA");
         value = SystemProperties.get(KEY, "default");
         assertEquals("SA", value);
@@ -62,7 +67,78 @@ public class SystemPropertiesTest extends TestCase {
         value = SystemProperties.get(KEY, "default");
         assertEquals("default", value);
 
+        // null value is the same as "".
+        SystemProperties.set(KEY, "SA");
+        SystemProperties.set(KEY, null);
+        value = SystemProperties.get(KEY, "default");
+        assertEquals("default", value);
+
         value = SystemProperties.get(KEY);
         assertEquals("", value);
     }
+
+    private static void testInt(String setVal, int defValue, int expected) {
+      SystemProperties.set(KEY, setVal);
+      int value = SystemProperties.getInt(KEY, defValue);
+      assertEquals(expected, value);
+    }
+
+    private static void testLong(String setVal, long defValue, long expected) {
+      SystemProperties.set(KEY, setVal);
+      long value = SystemProperties.getLong(KEY, defValue);
+      assertEquals(expected, value);
+    }
+
+    @SmallTest
+    public void testIntegralProperties() throws Exception {
+        testInt("", 123, 123);
+        testInt("", 0, 0);
+        testInt("", -123, -123);
+
+        testInt("123", 124, 123);
+        testInt("0", 124, 0);
+        testInt("-123", 124, -123);
+
+        testLong("", 3147483647L, 3147483647L);
+        testLong("", 0, 0);
+        testLong("", -3147483647L, -3147483647L);
+
+        testLong("3147483647", 124, 3147483647L);
+        testLong("0", 124, 0);
+        testLong("-3147483647", 124, -3147483647L);
+    }
+
+    @SmallTest
+    @SuppressWarnings("null")
+    public void testNullKey() throws Exception {
+        try {
+            SystemProperties.get(null);
+            fail("Expected NullPointerException");
+        } catch (NullPointerException npe) {
+        }
+
+        try {
+            SystemProperties.get(null, "default");
+            fail("Expected NullPointerException");
+        } catch (NullPointerException npe) {
+        }
+
+        try {
+            SystemProperties.set(null, "value");
+            fail("Expected NullPointerException");
+        } catch (NullPointerException npe) {
+        }
+
+        try {
+            SystemProperties.getInt(null, 0);
+            fail("Expected NullPointerException");
+        } catch (NullPointerException npe) {
+        }
+
+        try {
+            SystemProperties.getLong(null, 0);
+            fail("Expected NullPointerException");
+        } catch (NullPointerException npe) {
+        }
+    }
 }