Merge changes from topic "property-handle"

* changes:
  Add new SystemProperties.Handle interface
  Reimplement android.os.SystemProperties in terms of libc facilities

core/java/android/os/SystemProperties.java

@@ -26,6 +26,9 @@ import android.util.MutableInt;
 
 import com.android.internal.annotations.GuardedBy;
 
+import dalvik.annotation.optimization.CriticalNative;
+import dalvik.annotation.optimization.FastNative;
+
 import libcore.util.HexEncoding;
 
 import java.nio.charset.StandardCharsets;
@@ -93,18 +96,43 @@ public class SystemProperties {
         }
     }
 
+    // The one-argument version of native_get used to be a regular native function. Nowadays,
+    // we use the two-argument form of native_get all the time, but we can't just delete the
+    // one-argument overload: apps use it via reflection, as the UnsupportedAppUsage annotation
+    // indicates. Let's just live with having a Java function with a very unusual name.
     @UnsupportedAppUsage
-    private static native String native_get(String key);
+    private static String native_get(String key) {
+        return native_get(key, "");
+    }
+
+    @FastNative
     @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.P)
     private static native String native_get(String key, String def);
+    @FastNative
     @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.P)
     private static native int native_get_int(String key, int def);
+    @FastNative
     @UnsupportedAppUsage
     private static native long native_get_long(String key, long def);
+    @FastNative
     @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.P)
     private static native boolean native_get_boolean(String key, boolean def);
+
+    @FastNative
+    private static native long native_find(String name);
+    @FastNative
+    private static native String native_get(long handle);
+    @CriticalNative
+    private static native int native_get_int(long handle, int def);
+    @CriticalNative
+    private static native long native_get_long(long handle, long def);
+    @CriticalNative
+    private static native boolean native_get_boolean(long handle, boolean def);
+
+    // _NOT_ FastNative: native_set performs IPC and can block
     @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.P)
     private static native void native_set(String key, String def);
+
     @UnsupportedAppUsage(maxTargetSdk = Build.VERSION_CODES.P)
     private static native void native_add_change_callback();
     private static native void native_report_sysprop_change();
@@ -229,25 +257,27 @@ public class SystemProperties {
 
     @SuppressWarnings("unused")  // Called from native code.
     private static void callChangeCallbacks() {
+        ArrayList<Runnable> callbacks = null;
         synchronized (sChangeCallbacks) {
             //Log.i("foo", "Calling " + sChangeCallbacks.size() + " change callbacks!");
             if (sChangeCallbacks.size() == 0) {
                 return;
             }
-            ArrayList<Runnable> callbacks = new ArrayList<Runnable>(sChangeCallbacks);
+            callbacks = new ArrayList<Runnable>(sChangeCallbacks);
-            final long token = Binder.clearCallingIdentity();
+        }
-            try {
+        final long token = Binder.clearCallingIdentity();
-                for (int i = 0; i < callbacks.size(); i++) {
+        try {
-                    try {
+            for (int i = 0; i < callbacks.size(); i++) {
-                        callbacks.get(i).run();
+                try {
-                    } catch (Throwable t) {
+                    callbacks.get(i).run();
-                        Log.wtf(TAG, "Exception in SystemProperties change callback", t);
+                } catch (Throwable t) {
-                        // Ignore and try to go on.
+                    // Ignore and try to go on. Don't use wtf here: that
-                    }
+                    // will cause the process to exit on some builds and break tests.
+                    Log.e(TAG, "Exception in SystemProperties change callback", t);
                 }
-            } finally {
-                Binder.restoreCallingIdentity(token);
             }
+        } finally {
+            Binder.restoreCallingIdentity(token);
         }
     }
 
@@ -284,4 +314,60 @@ public class SystemProperties {
     @UnsupportedAppUsage
     private SystemProperties() {
     }
+
+    /**
+     * Look up a property location by name.
+     * @name name of the property
+     * @return property handle or {@code null} if property isn't set
+     * @hide
+     */
+    @Nullable public static Handle find(@NonNull String name) {
+        long nativeHandle = native_find(name);
+        if (nativeHandle == 0) {
+            return null;
+        }
+        return new Handle(nativeHandle);
+    }
+
+    /**
+     * Handle to a pre-located property. Looking up a property handle in advance allows
+     * for optimal repeated lookup of a single property.
+     * @hide
+     */
+    public static final class Handle {
+
+        private final long mNativeHandle;
+
+        /**
+         * @return Value of the property
+         */
+        @NonNull public String get() {
+            return native_get(mNativeHandle);
+        }
+        /**
+         * @param def default value
+         * @return value or {@code def} on parse error
+         */
+        public int getInt(int def) {
+            return native_get_int(mNativeHandle, def);
+        }
+        /**
+         * @param def default value
+         * @return value or {@code def} on parse error
+         */
+        public long getLong(long def) {
+            return native_get_long(mNativeHandle, def);
+        }
+        /**
+         * @param def default value
+         * @return value or {@code def} on parse error
+         */
+        public boolean getBoolean(boolean def) {
+            return native_get_boolean(mNativeHandle, def);
+        }
+
+        private Handle(long nativeHandle) {
+            mNativeHandle = nativeHandle;
+        }
+    }
 }

core/jni/android_os_SystemProperties.cpp

@@ -17,9 +17,13 @@
 
 #define LOG_TAG "SysPropJNI"
 
+#include <utility>
+#include <optional>
+
 #include "android-base/logging.h"
+#include "android-base/parsebool.h"
+#include "android-base/parseint.h"
 #include "android-base/properties.h"
-#include "cutils/properties.h"
 #include "utils/misc.h"
 #include <utils/Log.h>
 #include "jni.h"
@@ -28,86 +32,171 @@
 #include <nativehelper/ScopedPrimitiveArray.h>
 #include <nativehelper/ScopedUtfChars.h>
 
-namespace android
+#if defined(__BIONIC__)
-{
+# include <sys/system_properties.h>
+#else
+struct prop_info;
+#endif
 
+namespace android {
 namespace {
 
-template <typename T, typename Handler>
+using android::base::ParseBoolResult;
-T ConvertKeyAndForward(JNIEnv *env, jstring keyJ, T defJ, Handler handler) {
+
-    std::string key;
+template<typename Functor>
-    {
+void ReadProperty(const prop_info* prop, Functor&& functor)
-        // Scope the String access. If the handler can throw an exception,
+{
-        // releasing the string characters late would trigger an abort.
+#if defined(__BIONIC__)
-        ScopedUtfChars key_utf(env, keyJ);
+    auto thunk = [](void* cookie,
-        if (key_utf.c_str() == nullptr) {
+                    const char* /*name*/,
-            return defJ;
+                    const char* value,
-        }
+                    uint32_t /*serial*/) {
-        key = key_utf.c_str();  // This will make a copy, but we can't avoid
+        std::forward<Functor>(*static_cast<Functor*>(cookie))(value);
-                                // with the existing interface in
+    };
-                                // android::base.
+    __system_property_read_callback(prop, thunk, &functor);
-    }
+#else
-    return handler(key, defJ);
+    LOG(FATAL) << "fast property access supported only on device";
+#endif
 }
 
-jstring SystemProperties_getSS(JNIEnv *env, jclass clazz, jstring keyJ,
+template<typename Functor>
+void ReadProperty(JNIEnv* env, jstring keyJ, Functor&& functor)
+{
+    ScopedUtfChars key(env, keyJ);
+    if (!key.c_str()) {
+        return;
+    }
+#if defined(__BIONIC__)
+    const prop_info* prop = __system_property_find(key.c_str());
+    if (!prop) {
+        return;
+    }
+    ReadProperty(prop, std::forward<Functor>(functor));
+#else
+    std::forward<Functor>(functor)(
+        android::base::GetProperty(key.c_str(), "").c_str());
+#endif
+}
+
+jstring SystemProperties_getSS(JNIEnv* env, jclass clazz, jstring keyJ,
                                jstring defJ)
 {
-    // Using ConvertKeyAndForward is sub-optimal for copying the key string,
+    jstring ret = defJ;
-    // but improves reuse and reasoning over code.
+    ReadProperty(env, keyJ, [&](const char* value) {
-    auto handler = [&](const std::string& key, jstring defJ) {
+        if (value[0]) {
-        std::string prop_val = android::base::GetProperty(key, "");
+            ret = env->NewStringUTF(value);
-        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).
+    if (ret == nullptr && !env->ExceptionCheck()) {
-        return env->NewStringUTF("");
+      ret = env->NewStringUTF("");  // Legacy behavior
-    };
+    }
-    return ConvertKeyAndForward(env, keyJ, defJ, handler);
+    return ret;
-}
-
-jstring SystemProperties_getS(JNIEnv *env, jclass clazz, jstring keyJ)
-{
-    return SystemProperties_getSS(env, clazz, keyJ, nullptr);
 }
 
 template <typename T>
 T SystemProperties_get_integral(JNIEnv *env, jclass, jstring keyJ,
                                        T defJ)
 {
-    auto handler = [](const std::string& key, T defV) {
+    T ret = defJ;
-        return android::base::GetIntProperty<T>(key, defV);
+    ReadProperty(env, keyJ, [&](const char* value) {
-    };
+        android::base::ParseInt<T>(value, &ret);
-    return ConvertKeyAndForward(env, keyJ, defJ, handler);
+    });
+    return ret;
+}
+
+static jboolean jbooleanFromParseBoolResult(ParseBoolResult parseResult, jboolean defJ) {
+    jboolean ret;
+    switch (parseResult) {
+        case ParseBoolResult::kError:
+            ret = defJ;
+            break;
+        case ParseBoolResult::kFalse:
+            ret = JNI_FALSE;
+            break;
+        case ParseBoolResult::kTrue:
+            ret = JNI_TRUE;
+            break;
+    }
+    return ret;
 }
 
 jboolean SystemProperties_get_boolean(JNIEnv *env, jclass, jstring keyJ,
                                       jboolean defJ)
 {
-    auto handler = [](const std::string& key, jboolean defV) -> jboolean {
+    ParseBoolResult parseResult;
-        bool result = android::base::GetBoolProperty(key, defV);
+    ReadProperty(env, keyJ, [&](const char* value) {
-        return result ? JNI_TRUE : JNI_FALSE;
+        parseResult = android::base::ParseBool(value);
-    };
+    });
-    return ConvertKeyAndForward(env, keyJ, defJ, handler);
+    return jbooleanFromParseBoolResult(parseResult, defJ);
+}
+
+jlong SystemProperties_find(JNIEnv* env, jclass, jstring keyJ)
+{
+#if defined(__BIONIC__)
+    ScopedUtfChars key(env, keyJ);
+    if (!key.c_str()) {
+        return 0;
+    }
+    const prop_info* prop = __system_property_find(key.c_str());
+    return reinterpret_cast<jlong>(prop);
+#else
+    LOG(FATAL) << "fast property access supported only on device";
+    __builtin_unreachable();  // Silence warning
+#endif
+}
+
+jstring SystemProperties_getH(JNIEnv* env, jclass clazz, jlong propJ)
+{
+    jstring ret;
+    auto prop = reinterpret_cast<const prop_info*>(propJ);
+    ReadProperty(prop, [&](const char* value) {
+        ret = env->NewStringUTF(value);
+    });
+    return ret;
+}
+
+template <typename T>
+T SystemProperties_get_integralH(CRITICAL_JNI_PARAMS_COMMA jlong propJ, T defJ)
+{
+    T ret = defJ;
+    auto prop = reinterpret_cast<const prop_info*>(propJ);
+    ReadProperty(prop, [&](const char* value) {
+        android::base::ParseInt<T>(value, &ret);
+    });
+    return ret;
+}
+
+jboolean SystemProperties_get_booleanH(CRITICAL_JNI_PARAMS_COMMA jlong propJ, jboolean defJ)
+{
+    ParseBoolResult parseResult;
+    auto prop = reinterpret_cast<const prop_info*>(propJ);
+    ReadProperty(prop, [&](const char* value) {
+        parseResult = android::base::ParseBool(value);
+    });
+    return jbooleanFromParseBoolResult(parseResult, defJ);
 }
 
 void SystemProperties_set(JNIEnv *env, jobject clazz, jstring keyJ,
                           jstring valJ)
 {
-    auto handler = [&](const std::string& key, bool) {
+    ScopedUtfChars key(env, keyJ);
-        std::string val;
+    if (!key.c_str()) {
-        if (valJ != nullptr) {
+        return;
-            ScopedUtfChars key_utf(env, valJ);
+    }
-            val = key_utf.c_str();
+    std::optional<ScopedUtfChars> value;
+    if (valJ != nullptr) {
+        value.emplace(env, valJ);
+        if (!value->c_str()) {
+            return;
         }
-        return android::base::SetProperty(key, val);
+    }
-    };
+    bool success;
-    if (!ConvertKeyAndForward(env, keyJ, true, handler)) {
+#if defined(__BIONIC__)
-        // Must have been a failure in SetProperty.
+    success = !__system_property_set(key.c_str(), value ? value->c_str() : "");
+#else
+    success = android::base::SetProperty(key.c_str(), value ? value->c_str() : "");
+#endif
+    if (!success) {
         jniThrowException(env, "java/lang/RuntimeException",
                           "failed to set system property (check logcat for reason)");
     }
@@ -157,8 +246,6 @@ void SystemProperties_report_sysprop_change(JNIEnv /**env*/, jobject /*clazz*/)
 int register_android_os_SystemProperties(JNIEnv *env)
 {
     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 },
@@ -168,6 +255,18 @@ int register_android_os_SystemProperties(JNIEnv *env)
           (void*) SystemProperties_get_integral<jlong> },
         { "native_get_boolean", "(Ljava/lang/String;Z)Z",
           (void*) SystemProperties_get_boolean },
+        { "native_find",
+          "(Ljava/lang/String;)J",
+          (void*) SystemProperties_find },
+        { "native_get",
+          "(J)Ljava/lang/String;",
+          (void*) SystemProperties_getH },
+        { "native_get_int", "(JI)I",
+          (void*) SystemProperties_get_integralH<jint> },
+        { "native_get_long", "(JJ)J",
+          (void*) SystemProperties_get_integralH<jlong> },
+        { "native_get_boolean", "(JZ)Z",
+          (void*) SystemProperties_get_booleanH },
         { "native_set", "(Ljava/lang/String;Ljava/lang/String;)V",
           (void*) SystemProperties_set },
         { "native_add_change_callback", "()V",
@@ -179,4 +278,4 @@ int register_android_os_SystemProperties(JNIEnv *env)
                                 method_table, NELEM(method_table));
 }
 
-};
+}  // namespace android

core/tests/systemproperties/src/android/os/SystemPropertiesTest.java

@@ -92,6 +92,27 @@ public class SystemPropertiesTest extends TestCase {
       assertEquals(expected, value);
     }
 
+    @SmallTest
+    private static void testHandle() throws Exception {
+        String value;
+        SystemProperties.Handle handle = SystemProperties.find("doesnotexist_2341431");
+        assertNull(handle);
+        SystemProperties.set(KEY, "abc");
+        handle = SystemProperties.find(KEY);
+        assertNotNull(handle);
+        value = handle.get();
+        assertEquals("abc", value);
+        SystemProperties.set(KEY, "blarg");
+        value = handle.get();
+        assertEquals("blarg", value);
+        SystemProperties.set(KEY, "1");
+        assertEquals(1, handle.getInt(-1));
+        assertEquals(1, handle.getLong(-1));
+        assertEquals(true, handle.getBoolean(false));
+        SystemProperties.set(KEY, "");
+        assertEquals(12345, handle.getInt(12345));
+    }
+
     @SmallTest
     public void testIntegralProperties() throws Exception {
         testInt("", 123, 123);