Add __system_property_wait and return the serial in __system_property_read_callback.
In order to implement android::base::WaitForProperty well, we need a way to
wait not for *any* property to change (__system_property_wait_any), but to
specifically wait for the property represented by a given `prop_info` to
change.
The android::base::WaitForProperty implementation, like attempts to cache
system properties in the past, also needs a way to keep serials and values
in sync, but the existing functions don't provide a cheap way to get a
consistent snapshot. Change the __system_property_read_callback callback's
type to include the serial corresponding to the given value.
Add a test, slightly clean up some of the existing tests (and name them to
include the names of the functions they're testing, in our usual style).
Bug: http://b/35201172
Test: ran tests
Change-Id: Ibc8ebe2e88eef1e333a1bd3dd7f68135f1ba7fb5
diff --git a/libc/bionic/system_properties.cpp b/libc/bionic/system_properties.cpp
index a62ce14..32d1e31 100644
--- a/libc/bionic/system_properties.cpp
+++ b/libc/bionic/system_properties.cpp
@@ -1102,7 +1102,7 @@
return fsetxattr_failed ? -2 : 0;
}
-unsigned int __system_property_area_serial() {
+uint32_t __system_property_area_serial() {
prop_area* pa = __system_property_area__;
if (!pa) {
return -1;
@@ -1163,8 +1163,10 @@
}
void __system_property_read_callback(const prop_info* pi,
- void (*callback)(void* cookie, const char* name,
- const char* value),
+ void (*callback)(void* cookie,
+ const char* name,
+ const char* value,
+ uint32_t serial),
void* cookie) {
while (true) {
uint32_t serial = __system_property_serial(pi); // acquire semantics
@@ -1177,7 +1179,7 @@
// TODO: see todo in __system_property_read function
atomic_thread_fence(memory_order_acquire);
if (serial == load_const_atomic(&(pi->serial), memory_order_relaxed)) {
- callback(cookie, pi->name, value_buf);
+ callback(cookie, pi->name, value_buf, serial);
return;
}
}
@@ -1329,30 +1331,34 @@
}
// Wait for non-locked serial, and retrieve it with acquire semantics.
-unsigned int __system_property_serial(const prop_info* pi) {
+uint32_t __system_property_serial(const prop_info* pi) {
uint32_t serial = load_const_atomic(&pi->serial, memory_order_acquire);
while (SERIAL_DIRTY(serial)) {
- __futex_wait(const_cast<volatile void*>(reinterpret_cast<const void*>(&pi->serial)), serial,
- nullptr);
+ __futex_wait(const_cast<_Atomic(uint_least32_t)*>(&pi->serial), serial, nullptr);
serial = load_const_atomic(&pi->serial, memory_order_acquire);
}
return serial;
}
-unsigned int __system_property_wait_any(unsigned int serial) {
+uint32_t __system_property_wait_any(uint32_t old_serial) {
prop_area* pa = __system_property_area__;
- uint32_t my_serial;
+ if (!pa) return 0;
- if (!pa) {
- return 0;
- }
-
+ uint32_t new_serial;
do {
- __futex_wait(pa->serial(), serial, nullptr);
- my_serial = atomic_load_explicit(pa->serial(), memory_order_acquire);
- } while (my_serial == serial);
+ __futex_wait(pa->serial(), old_serial, nullptr);
+ new_serial = atomic_load_explicit(pa->serial(), memory_order_acquire);
+ } while (new_serial == old_serial);
+ return new_serial;
+}
- return my_serial;
+uint32_t __system_property_wait(const prop_info* pi, uint32_t old_serial) {
+ uint32_t new_serial;
+ do {
+ __futex_wait(const_cast<_Atomic(uint_least32_t)*>(&pi->serial), old_serial, nullptr);
+ new_serial = load_const_atomic(&pi->serial, memory_order_acquire);
+ } while (new_serial == old_serial);
+ return new_serial;
}
const prop_info* __system_property_find_nth(unsigned n) {
diff --git a/libc/include/sys/_system_properties.h b/libc/include/sys/_system_properties.h
index ffa6d2e..fa98d11 100644
--- a/libc/include/sys/_system_properties.h
+++ b/libc/include/sys/_system_properties.h
@@ -30,6 +30,7 @@
#define _INCLUDE_SYS__SYSTEM_PROPERTIES_H
#include <sys/cdefs.h>
+#include <stdint.h>
#ifndef _REALLY_INCLUDE_SYS__SYSTEM_PROPERTIES_H_
#error you should #include <sys/system_properties.h> instead
@@ -91,7 +92,7 @@
**
** Returns the serial number on success, -1 on error.
*/
-unsigned int __system_property_area_serial();
+uint32_t __system_property_area_serial();
/* Add a new system property. Can only be done by a single
** process that has write access to the property area, and
@@ -118,12 +119,22 @@
**
** Returns the serial number on success, -1 on error.
*/
-unsigned int __system_property_serial(const prop_info *pi);
+uint32_t __system_property_serial(const prop_info* pi);
-/* Wait for any system property to be updated. Caller must pass
-** in 0 the first time, and the previous return value on each
-** successive call. */
-unsigned int __system_property_wait_any(unsigned int serial);
+/*
+ * Waits for any system property to be updated past `old_serial`.
+ * If you don't know the current global serial number, use 0.
+ * Returns the new global serial number.
+ */
+uint32_t __system_property_wait_any(uint32_t old_serial);
+
+/*
+ * Waits for the specific system property identified by `pi` to be updated past `old_serial`.
+ * If you don't know the current serial, use 0.
+ * Returns the serial number for `pi` that caused the wake.
+ */
+uint32_t __system_property_wait(const prop_info* pi, uint32_t old_serial)
+ __INTRODUCED_IN_FUTURE;
/* Initialize the system properties area in read only mode.
* Should be done by all processes that need to read system
diff --git a/libc/include/sys/system_properties.h b/libc/include/sys/system_properties.h
index d80b2fe..fb90251 100644
--- a/libc/include/sys/system_properties.h
+++ b/libc/include/sys/system_properties.h
@@ -31,77 +31,52 @@
#include <sys/cdefs.h>
#include <stddef.h>
+#include <stdint.h>
__BEGIN_DECLS
typedef struct prop_info prop_info;
-#define PROP_NAME_MAX 32
#define PROP_VALUE_MAX 92
-/* Look up a system property by name, copying its value and a
-** \0 terminator to the provided pointer. The total bytes
-** copied will be no greater than PROP_VALUE_MAX. Returns
-** the string length of the value. A property that is not
-** defined is identical to a property with a length 0 value.
-*/
-int __system_property_get(const char *name, char *value);
-
-/* Set a system property by name.
-**/
+/*
+ * Sets system property `key` to `value`, creating the system property if it doesn't already exist.
+ */
int __system_property_set(const char* key, const char* value) __INTRODUCED_IN(12);
-/* Return a pointer to the system property named name, if it
-** exists, or NULL if there is no such property. Use
-** __system_property_read() to obtain the string value from
-** the returned prop_info pointer.
-**
-** It is safe to cache the prop_info pointer to avoid future
-** lookups. These returned pointers will remain valid for
-** the lifetime of the system.
-*/
-const prop_info *__system_property_find(const char *name);
+/*
+ * Returns a `prop_info` corresponding system property `name`, or nullptr if it doesn't exist.
+ * Use __system_property_read_callback to query the current value.
+ *
+ * Property lookup is expensive, so it can be useful to cache the result of this function.
+ */
+const prop_info* __system_property_find(const char* name);
-/* Read the value of a system property. Returns the length
-** of the value. Copies the value and \0 terminator into
-** the provided value pointer. Total length (including
-** terminator) will be no greater that PROP_VALUE_MAX for
-** __system_property_read.
-**
-** If name is nonzero, up to PROP_NAME_MAX bytes will be
-** copied into the provided name pointer. The name will
-** be \0 terminated.
-*/
-int __system_property_read(const prop_info *pi, char *name, char *value);
+/*
+ * Calls `callback` with a consistent trio of name, value, and serial number for property `pi`.
+ */
void __system_property_read_callback(const prop_info *pi,
- void (*)(void* cookie, const char *name, const char *value),
- void* cookie) __INTRODUCED_IN_FUTURE;
+ void (*callback)(void* cookie, const char *name, const char *value, uint32_t serial),
+ void* cookie) __INTRODUCED_IN_FUTURE;
-/* Return a prop_info for the nth system property, or NULL if
-** there is no nth property. Use __system_property_read() to
-** read the value of this property.
-**
-** Please do not call this method. It only exists to provide
-** backwards compatibility to NDK apps. Its implementation
-** is inefficient and order of results may change from call
-** to call.
-*/
-const prop_info *__system_property_find_nth(unsigned n)
- __REMOVED_IN(26);
-
-/* Pass a prop_info for each system property to the provided
-** callback. Use __system_property_read() to read the value
-** of this property.
-**
-** This method is for inspecting and debugging the property
-** system. Please use __system_property_find() instead.
-**
-** Order of results may change from call to call. This is
-** not a bug.
-*/
+/*
+ * Passes a `prop_info` for each system property to the provided
+ * callback. Use __system_property_read_callback() to read the value.
+ *
+ * This method is for inspecting and debugging the property system, and not generally useful.
+ */
int __system_property_foreach(void (*propfn)(const prop_info* pi, void* cookie), void* cookie)
__INTRODUCED_IN(19);
+/* Deprecated. In Android O and above, there's no limit on property name length. */
+#define PROP_NAME_MAX 32
+/* Deprecated. Use __system_property_read_callback instead. */
+int __system_property_read(const prop_info *pi, char *name, char *value);
+/* Deprecated. Use __system_property_read_callback instead. */
+int __system_property_get(const char *name, char *value);
+/* Deprecated. Use __system_property_foreach instead. Aborts in Android O and above. */
+const prop_info *__system_property_find_nth(unsigned n) __REMOVED_IN(26);
+
__END_DECLS
#endif
diff --git a/libc/libc.arm.map b/libc/libc.arm.map
index 4319429..33aedb2 100644
--- a/libc/libc.arm.map
+++ b/libc/libc.arm.map
@@ -1266,6 +1266,7 @@
LIBC_O {
global:
__system_property_read_callback; # future
+ __system_property_wait; # future
bsd_signal; # arm x86 mips versioned=26
catclose; # future
catgets; # future
diff --git a/libc/libc.arm64.map b/libc/libc.arm64.map
index f88c284..0d4fc2d 100644
--- a/libc/libc.arm64.map
+++ b/libc/libc.arm64.map
@@ -1189,6 +1189,7 @@
LIBC_O {
global:
__system_property_read_callback; # future
+ __system_property_wait; # future
catclose; # future
catgets; # future
catopen; # future
diff --git a/libc/libc.map.txt b/libc/libc.map.txt
index 46087e6..4d9ac57 100644
--- a/libc/libc.map.txt
+++ b/libc/libc.map.txt
@@ -1291,6 +1291,7 @@
LIBC_O {
global:
__system_property_read_callback; # future
+ __system_property_wait; # future
bsd_signal; # arm x86 mips versioned=26
catclose; # future
catgets; # future
diff --git a/libc/libc.mips.map b/libc/libc.mips.map
index 075746c..c526226 100644
--- a/libc/libc.mips.map
+++ b/libc/libc.mips.map
@@ -1250,6 +1250,7 @@
LIBC_O {
global:
__system_property_read_callback; # future
+ __system_property_wait; # future
bsd_signal; # arm x86 mips versioned=26
catclose; # future
catgets; # future
diff --git a/libc/libc.mips64.map b/libc/libc.mips64.map
index f88c284..0d4fc2d 100644
--- a/libc/libc.mips64.map
+++ b/libc/libc.mips64.map
@@ -1189,6 +1189,7 @@
LIBC_O {
global:
__system_property_read_callback; # future
+ __system_property_wait; # future
catclose; # future
catgets; # future
catopen; # future
diff --git a/libc/libc.x86.map b/libc/libc.x86.map
index 75c7757..130bbed 100644
--- a/libc/libc.x86.map
+++ b/libc/libc.x86.map
@@ -1248,6 +1248,7 @@
LIBC_O {
global:
__system_property_read_callback; # future
+ __system_property_wait; # future
bsd_signal; # arm x86 mips versioned=26
catclose; # future
catgets; # future
diff --git a/libc/libc.x86_64.map b/libc/libc.x86_64.map
index f88c284..0d4fc2d 100644
--- a/libc/libc.x86_64.map
+++ b/libc/libc.x86_64.map
@@ -1189,6 +1189,7 @@
LIBC_O {
global:
__system_property_read_callback; # future
+ __system_property_wait; # future
catclose; # future
catgets; # future
catopen; # future
diff --git a/tests/system_properties_test.cpp b/tests/system_properties_test.cpp
index 6b037d8..39734d7 100644
--- a/tests/system_properties_test.cpp
+++ b/tests/system_properties_test.cpp
@@ -20,7 +20,9 @@
#include <errno.h>
#include <sys/wait.h>
#include <unistd.h>
+
#include <string>
+#include <thread>
#if defined(__BIONIC__)
@@ -111,13 +113,11 @@
#endif // __BIONIC__
-TEST(properties, add) {
+TEST(properties, __system_property_add) {
#if defined(__BIONIC__)
LocalPropertyTestState pa;
ASSERT_TRUE(pa.valid);
- char propvalue[PROP_VALUE_MAX];
-
ASSERT_EQ(0, __system_property_add("property", 8, "value1", 6));
ASSERT_EQ(0, __system_property_add("other_property", 14, "value2", 6));
ASSERT_EQ(0, __system_property_add("property_other", 14, "value3", 6));
@@ -132,6 +132,7 @@
name[sizeof(name)-1] = '\0';
ASSERT_EQ(0, __system_property_add(name, strlen(name), "value", 5));
+ char propvalue[PROP_VALUE_MAX];
ASSERT_EQ(6, __system_property_get("property", propvalue));
ASSERT_STREQ(propvalue, "value1");
@@ -148,30 +149,28 @@
#endif // __BIONIC__
}
-TEST(properties, update) {
+TEST(properties, __system_property_update) {
#if defined(__BIONIC__)
LocalPropertyTestState pa;
ASSERT_TRUE(pa.valid);
- char propvalue[PROP_VALUE_MAX];
- prop_info *pi;
-
ASSERT_EQ(0, __system_property_add("property", 8, "oldvalue1", 9));
ASSERT_EQ(0, __system_property_add("other_property", 14, "value2", 6));
ASSERT_EQ(0, __system_property_add("property_other", 14, "value3", 6));
- pi = (prop_info *)__system_property_find("property");
- ASSERT_NE((prop_info *)NULL, pi);
- __system_property_update(pi, "value4", 6);
+ const prop_info* pi = __system_property_find("property");
+ ASSERT_TRUE(pi != nullptr);
+ __system_property_update(const_cast<prop_info*>(pi), "value4", 6);
- pi = (prop_info *)__system_property_find("other_property");
- ASSERT_NE((prop_info *)NULL, pi);
- __system_property_update(pi, "newvalue5", 9);
+ pi = __system_property_find("other_property");
+ ASSERT_TRUE(pi != nullptr);
+ __system_property_update(const_cast<prop_info*>(pi), "newvalue5", 9);
- pi = (prop_info *)__system_property_find("property_other");
- ASSERT_NE((prop_info *)NULL, pi);
- __system_property_update(pi, "value6", 6);
+ pi = __system_property_find("property_other");
+ ASSERT_TRUE(pi != nullptr);
+ __system_property_update(const_cast<prop_info*>(pi), "value6", 6);
+ char propvalue[PROP_VALUE_MAX];
ASSERT_EQ(6, __system_property_get("property", propvalue));
ASSERT_STREQ(propvalue, "value4");
@@ -230,16 +229,16 @@
#endif // __BIONIC__
}
-TEST(properties, foreach) {
+TEST(properties, __system_property_foreach) {
#if defined(__BIONIC__)
LocalPropertyTestState pa;
ASSERT_TRUE(pa.valid);
- size_t count = 0;
ASSERT_EQ(0, __system_property_add("property", 8, "value1", 6));
ASSERT_EQ(0, __system_property_add("other_property", 14, "value2", 6));
ASSERT_EQ(0, __system_property_add("property_other", 14, "value3", 6));
+ size_t count = 0;
ASSERT_EQ(0, __system_property_foreach(foreach_test_callback, &count));
ASSERT_EQ(3U, count);
#else // __BIONIC__
@@ -247,7 +246,7 @@
#endif // __BIONIC__
}
-TEST(properties, find_nth) {
+TEST(properties, __system_property_find_nth) {
#if defined(__BIONIC__)
LocalPropertyTestState pa;
ASSERT_TRUE(pa.valid);
@@ -342,47 +341,74 @@
#endif // __BIONIC__
}
-TEST(properties, serial) {
+TEST(properties, __system_property_serial) {
#if defined(__BIONIC__)
LocalPropertyTestState pa;
ASSERT_TRUE(pa.valid);
- const prop_info *pi;
- unsigned int serial;
ASSERT_EQ(0, __system_property_add("property", 8, "value1", 6));
- ASSERT_NE((const prop_info *)NULL, pi = __system_property_find("property"));
- serial = __system_property_serial(pi);
- ASSERT_EQ(0, __system_property_update((prop_info *)pi, "value2", 6));
+ const prop_info* pi = __system_property_find("property");
+ ASSERT_TRUE(pi != nullptr);
+ unsigned serial = __system_property_serial(pi);
+ ASSERT_EQ(0, __system_property_update(const_cast<prop_info*>(pi), "value2", 6));
ASSERT_NE(serial, __system_property_serial(pi));
#else // __BIONIC__
GTEST_LOG_(INFO) << "This test does nothing.\n";
#endif // __BIONIC__
}
-TEST(properties, wait) {
+TEST(properties, __system_property_wait_any) {
#if defined(__BIONIC__)
LocalPropertyTestState pa;
ASSERT_TRUE(pa.valid);
- unsigned int serial;
- prop_info *pi;
- pthread_t t;
- int flag = 0;
ASSERT_EQ(0, __system_property_add("property", 8, "value1", 6));
- serial = __system_property_wait_any(0);
+ unsigned serial = __system_property_wait_any(0);
- pi = const_cast<prop_info*>(__system_property_find("property"));
+ prop_info* pi = const_cast<prop_info*>(__system_property_find("property"));
ASSERT_TRUE(pi != nullptr);
__system_property_update(pi, "value2", 6);
serial = __system_property_wait_any(serial);
+ int flag = 0;
+ pthread_t t;
ASSERT_EQ(0, pthread_create(&t, nullptr, PropertyWaitHelperFn, &flag));
ASSERT_EQ(flag, 0);
serial = __system_property_wait_any(serial);
ASSERT_EQ(flag, 1);
- void* result;
- ASSERT_EQ(0, pthread_join(t, &result));
+ ASSERT_EQ(0, pthread_join(t, nullptr));
+#else // __BIONIC__
+ GTEST_LOG_(INFO) << "This test does nothing.\n";
+#endif // __BIONIC__
+}
+
+TEST(properties, __system_property_wait) {
+#if defined(__BIONIC__)
+ LocalPropertyTestState pa;
+ ASSERT_TRUE(pa.valid);
+
+ ASSERT_EQ(0, __system_property_add("property", 8, "value1", 6));
+
+ prop_info* pi = const_cast<prop_info*>(__system_property_find("property"));
+ ASSERT_TRUE(pi != nullptr);
+
+ unsigned serial = __system_property_serial(pi);
+
+ std::thread thread([]() {
+ prop_info* pi = const_cast<prop_info*>(__system_property_find("property"));
+ ASSERT_TRUE(pi != nullptr);
+
+ __system_property_update(pi, "value2", 6);
+ });
+
+ __system_property_wait(pi, serial);
+
+ char value[PROP_VALUE_MAX];
+ ASSERT_EQ(6, __system_property_get("property", value));
+ ASSERT_STREQ("value2", value);
+
+ thread.join();
#else // __BIONIC__
GTEST_LOG_(INFO) << "This test does nothing.\n";
#endif // __BIONIC__
diff --git a/tests/system_properties_test2.cpp b/tests/system_properties_test2.cpp
index 0560960..e6e7ef2 100644
--- a/tests/system_properties_test2.cpp
+++ b/tests/system_properties_test2.cpp
@@ -90,20 +90,22 @@
ASSERT_TRUE(pi != nullptr);
std::string expected_name = property_name;
- __system_property_read_callback(pi, [](void* cookie, const char *name, const char *value) {
- const std::string* expected_name = static_cast<const std::string*>(cookie);
- ASSERT_EQ(*expected_name, name);
- ASSERT_STREQ("value1-1", value);
+ __system_property_read_callback(pi,
+ [](void* cookie, const char* name, const char* value, unsigned /*serial*/) {
+ const std::string* expected_name = static_cast<const std::string*>(cookie);
+ ASSERT_EQ(*expected_name, name);
+ ASSERT_STREQ("value1-1", value);
}, &expected_name);
pi = __system_property_find(long_property_name.c_str());
ASSERT_TRUE(pi != nullptr);
expected_name = long_property_name;
- __system_property_read_callback(pi, [](void* cookie, const char *name, const char *value) {
- const std::string* expected_name = static_cast<const std::string*>(cookie);
- ASSERT_EQ(*expected_name, name);
- ASSERT_STREQ("value2", value);
+ __system_property_read_callback(pi,
+ [](void* cookie, const char* name, const char* value, unsigned /*serial*/) {
+ const std::string* expected_name = static_cast<const std::string*>(cookie);
+ ASSERT_EQ(*expected_name, name);
+ ASSERT_STREQ("value2", value);
}, &expected_name);
// Check that read() for long names still works but returns truncated version of the name
