btif: Don't persist remote devices to the config

We don't need to persist the unpaired devices to NVRAM
so skip saving them.

This fixes a regression in a previous patch where the most recent
instead of the least recent devices would be removed, making some
devices unpairable in extremely busy environments.

Bug: 26071376

Change-Id: If7ee9d960f70c836bf08b78da5f3fc852ba60a85

diff --git a/btif/include/btif_storage.h b/btif/include/btif_storage.h
index 32df1e7..7b5a957 100644 (file)
--- a/btif/include/btif_storage.h
+++ b/btif/include/btif_storage.h
@@ -96,11 +96,11 @@ bt_status_t btif_storage_set_remote_device_property(bt_bdaddr_t *remote_bd_addr,
 **
 ** Function         btif_storage_add_remote_device
 **
-** Description      BTIF storage API - Adds a newly discovered device to NVRAM
-**                  along with the timestamp. Also, stores the various
+** Description      BTIF storage API - Adds a newly discovered device to
+**                  track along with the timestamp. Also, stores the various
 **                  properties - RSSI, BDADDR, NAME (if found in EIR)
 **
-** Returns          BT_STATUS_SUCCESS if the store was successful,
+** Returns          BT_STATUS_SUCCESS if successful,
 **                  BT_STATUS_FAIL otherwise
 **
 *******************************************************************************/

diff --git a/btif/src/btif_config.c b/btif/src/btif_config.c
index ef90f89..ea1b8b7 100644 (file)
--- a/btif/src/btif_config.c
+++ b/btif/src/btif_config.c
@@ -64,9 +64,9 @@ static const period_ms_t CONFIG_SETTLE_PERIOD_MS = 3000;
 
 static void timer_config_save_cb(void *data);
 static void btif_config_write(UINT16 event, char *p_param);
-static void btif_config_devcache_cleanup(void);
 static bool is_factory_reset(void);
 static void delete_config_files(void);
+static void btif_config_remove_unpaired(config_t *config);
 
 static enum ConfigSource {
   NOT_LOADED,
@@ -151,7 +151,7 @@ static future_t *init(void) {
     goto error;
   }
 
-  btif_config_devcache_cleanup();
+  btif_config_remove_unpaired(config);
 
   // TODO(sharvil): use a non-wake alarm for this once we have
   // API support for it. There's no need to wake the system to
@@ -388,8 +388,8 @@ bool btif_config_remove(const char *section, const char *key) {
 }
 
 void btif_config_save(void) {
-  assert(config_timer != NULL);
   assert(config != NULL);
+  assert(config_timer != NULL);
 
   alarm_set(config_timer, CONFIG_SETTLE_PERIOD_MS, timer_config_save_cb, NULL);
 }
@@ -400,10 +400,6 @@ void btif_config_flush(void) {
 
   alarm_cancel(config_timer);
   btif_config_write(0, NULL);
-
-  pthread_mutex_lock(&lock);
-  config_save(config, CONFIG_FILE_PATH);
-  pthread_mutex_unlock(&lock);
 }
 
 bool btif_config_clear(void) {
@@ -438,47 +434,38 @@ static void btif_config_write(UNUSED_ATTR UINT16 event, UNUSED_ATTR char *p_para
   assert(config != NULL);
   assert(config_timer != NULL);
 
-  btif_config_devcache_cleanup();
-
   pthread_mutex_lock(&lock);
   rename(CONFIG_FILE_PATH, CONFIG_BACKUP_PATH);
   sync();
-  config_save(config, CONFIG_FILE_PATH);
+  config_t *config_paired = config_clone(config);
+  btif_config_remove_unpaired(config_paired);
+  config_save(config_paired, CONFIG_FILE_PATH);
   pthread_mutex_unlock(&lock);
 }
 
-static void btif_config_devcache_cleanup(void) {
-  assert(config != NULL);
-
-  // The config accumulates cached information about remote
-  // devices during regular inquiry scans. We remove some of these
-  // so the cache doesn't grow indefinitely.
-  // We don't remove information about bonded devices (which have link keys).
-  static const size_t ADDRS_MAX = 512;
-  size_t total_addrs = 0;
+static void btif_config_remove_unpaired(config_t *conf) {
+  assert(conf != NULL);
 
-  pthread_mutex_lock(&lock);
-  const config_section_node_t *snode = config_section_begin(config);
-  while (snode != config_section_end(config)) {
+  // The paired config used to carry information about
+  // discovered devices during regular inquiry scans.
+  // We remove these now and cache them in memory instead.
+  const config_section_node_t *snode = config_section_begin(conf);
+  while (snode != config_section_end(conf)) {
     const char *section = config_section_name(snode);
     if (string_is_bdaddr(section)) {
-      ++total_addrs;
-
-      if ((total_addrs > ADDRS_MAX) &&
-          !config_has_key(config, section, "LinkKey") &&
-          !config_has_key(config, section, "LE_KEY_PENC") &&
-          !config_has_key(config, section, "LE_KEY_PID") &&
-          !config_has_key(config, section, "LE_KEY_PCSRK") &&
-          !config_has_key(config, section, "LE_KEY_LENC") &&
-          !config_has_key(config, section, "LE_KEY_LCSRK")) {
+      if (!config_has_key(conf, section, "LinkKey") &&
+          !config_has_key(conf, section, "LE_KEY_PENC") &&
+          !config_has_key(conf, section, "LE_KEY_PID") &&
+          !config_has_key(conf, section, "LE_KEY_PCSRK") &&
+          !config_has_key(conf, section, "LE_KEY_LENC") &&
+          !config_has_key(conf, section, "LE_KEY_LCSRK")) {
         snode = config_section_next(snode);
-        config_remove_section(config, section);
+        config_remove_section(conf, section);
         continue;
       }
     }
     snode = config_section_next(snode);
   }
-  pthread_mutex_unlock(&lock);
 }
 
 void btif_debug_config_dump(int fd) {

diff --git a/osi/include/config.h b/osi/include/config.h
index ab3096d..3d7630f 100644 (file)
--- a/osi/include/config.h
+++ b/osi/include/config.h
@@ -37,6 +37,14 @@ config_t *config_new_empty(void);
 // file on the filesystem.
 config_t *config_new(const char *filename);
 
+// Clones |src|, including all of it's sections, keys, and values.
+// Returns a new config which is a copy and separated from the original;
+// changes to the new config are not reflected in any way in the original.
+//
+// |src| must not be NULL
+// This function will not return NULL.
+config_t *config_clone(config_t *src);
+
 // Frees resources associated with the config file. No further operations may
 // be performed on the |config| object after calling this function. |config|
 // may be NULL.

diff --git a/osi/src/config.c b/osi/src/config.c
index 740677a..8157bd5 100644 (file)
--- a/osi/src/config.c
+++ b/osi/src/config.c
@@ -94,6 +94,30 @@ config_t *config_new(const char *filename) {
   return config;
 }
 
+config_t *config_clone(config_t *src) {
+  assert(src != NULL);
+
+  config_t *ret = config_new_empty();
+
+  assert(ret != NULL);
+
+  for (const list_node_t *node = list_begin(src->sections);
+       node != list_end(src->sections);
+       node = list_next(node)) {
+    section_t *sec = list_node(node);
+
+    for (const list_node_t *node_entry = list_begin(sec->entries);
+         node_entry != list_end(sec->entries);
+         node_entry = list_next(node_entry)) {
+      entry_t *entry = list_node(node_entry);
+
+      config_set_string(ret, sec->name, entry->key, entry->value);
+    }
+  }
+
+  return ret;
+}
+
 void config_free(config_t *config) {
   if (!config)
     return;

diff --git a/osi/test/config_test.cpp b/osi/test/config_test.cpp
index 4a0c4f8..2b61db1 100644 (file)
--- a/osi/test/config_test.cpp
+++ b/osi/test/config_test.cpp
@@ -80,6 +80,19 @@ TEST_F(ConfigTest, config_free_null) {
   config_free(NULL);
 }
 
+TEST_F(ConfigTest, config_clone) {
+  config_t *config = config_new(CONFIG_FILE);
+  config_t *clone = config_clone(config);
+
+  config_set_string(clone, CONFIG_DEFAULT_SECTION, "first_key", "not_value");
+
+  EXPECT_STRNE(config_get_string(config, CONFIG_DEFAULT_SECTION, "first_key", "one"),
+               config_get_string(clone, CONFIG_DEFAULT_SECTION, "first_key", "one"));
+
+  config_free(config);
+  config_free(clone);
+}
+
 TEST_F(ConfigTest, config_has_section) {
   config_t *config = config_new(CONFIG_FILE);
   EXPECT_TRUE(config_has_section(config, "DID"));