From 158910b4aaf2a72babdb4bf4176377c3de50b017 Mon Sep 17 00:00:00 2001 From: Marie Janssen Date: Fri, 25 Mar 2016 13:37:13 -0700 Subject: 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. This is a backport of http://r.android.com/210955 and http://r.android.com/212838 together. Bug: 26071376 Change-Id: If7ee9d960f70c836bf08b78da5f3fc852ba60a85 --- btif/include/btif_storage.h | 6 +++--- btif/src/btif_config.c | 49 ++++++++++++++++++--------------------------- osi/include/config.h | 9 +++++++++ osi/src/config.c | 24 ++++++++++++++++++++++ osi/test/config_test.cpp | 13 ++++++++++++ 5 files changed, 69 insertions(+), 32 deletions(-) diff --git a/btif/include/btif_storage.h b/btif/include/btif_storage.h index 54dc5d6b0..7763d3a3d 100644 --- a/btif/include/btif_storage.h +++ b/btif/include/btif_storage.h @@ -93,11 +93,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 dfd308e90..ad5b60728 100644 --- a/btif/src/btif_config.c +++ b/btif/src/btif_config.c @@ -44,7 +44,7 @@ static const period_ms_t CONFIG_SETTLE_PERIOD_MS = 3000; static void timer_config_save_cb(void *data); static void btif_config_write(void); -static void btif_config_devcache_cleanup(void); +static void btif_config_remove_unpaired(config_t *config); // TODO(zachoverflow): Move these two functions out, because they are too specific for this file // {grumpy-cat/no, monty-python/you-make-me-sad} @@ -109,7 +109,7 @@ static future_t *init(void) { unlink(LEGACY_CONFIG_FILE_PATH); } - 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 @@ -358,7 +358,6 @@ void btif_config_flush(void) { assert(alarm_timer != NULL); alarm_cancel(alarm_timer); - btif_config_write(); } @@ -390,43 +389,35 @@ static void btif_config_write(void) { assert(config != NULL); assert(alarm_timer != NULL); - btif_config_devcache_cleanup(); - pthread_mutex_lock(&lock); - config_save(config, CONFIG_FILE_PATH); + config_t *config_paired = config_new_clone(config); + btif_config_remove_unpaired(config_paired); + config_save(config_paired, CONFIG_FILE_PATH); + config_free(config_paired); 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); } diff --git a/osi/include/config.h b/osi/include/config.h index ab3096d4e..d7c8ff9a2 100644 --- a/osi/include/config.h +++ b/osi/include/config.h @@ -37,6 +37,15 @@ 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. +// Clients must call config_free on the returned object. +config_t *config_new_clone(const 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 11a5baf72..c87f5310a 100644 --- a/osi/src/config.c +++ b/osi/src/config.c @@ -96,6 +96,30 @@ config_t *config_new(const char *filename) { return config; } +config_t *config_new_clone(const 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 70e5a2494..0419ddc1c 100644 --- 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_new_clone) { + config_t *config = config_new(CONFIG_FILE); + config_t *clone = config_new_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")); -- cgit v1.2.3