Bluetooth: Host: Add API to clean BT settings #59821

theob-pro · 2023-06-28T13:01:55Z

See commits message.

include/zephyr/bluetooth/settings.h

tests/bsim/bluetooth/host/settings/cleanup/src/test.c

subsys/bluetooth/host/settings.c

tests/bsim/bluetooth/host/settings/cleanup/src/test.c

jhedberg · 2023-07-05T09:43:46Z

Shouldn't the Bluetooth subsystem do this automatically upon init? Introducing an explicit public API creates the need for applications to figure out when to call it, not to mention that the inconsistency in storage might not be the application's fault at all. It also seems arbitrary to leave mesh of the scope. What's the solution for inconsistent storage for mesh, or for non-Bluetooth related data for that matter?

jori-nordic · 2023-07-05T09:47:13Z

Shouldn't the Bluetooth subsystem do this automatically upon init? Introducing an explicit public API creates the need for applications to figure out when to call it, not to mention that the inconsistency in storage might not be the application's fault at all. It also seems arbitrary to leave mesh of the scope. What's the solution for inconsistent storage for mesh, or for non-Bluetooth related data for that matter?

The problem might be that this will take too much time - writing to flash - at an infortunate time. That's why we thought it'd be better for the app to decide when to do GC.

theob-pro · 2023-07-05T11:26:35Z

It also seems arbitrary to leave mesh of the scope. What's the solution for inconsistent storage for mesh, or for non-Bluetooth related data for that matter?

I discussed it with @PavelVPV, they might do it differently with their own settings layer.

Thalley

Generally looks good. One thing that I request changed is the break in the for loop, and then a few minor comments/suggestions

Thalley · 2023-07-10T13:28:36Z

include/zephyr/bluetooth/settings.h

+ * Example of possible leftover: An identity has been deleted, the corresponding
+ * address as been removed from 'bt/id' but before removing the corresponding
+ * key in 'bt/keys', the device has been powered off. Calling
+ * `bt_settings_cleanup` will ensure that the key is removed.


Would it make sense to have an additional function that verifies the settings to indicate whether calling this function will change anything?

Something along the lines of bt_settings_valid so that the use of this function will be

if (!bt_settings_valid()) { bt_settings_cleanup(); }

or some other way of doing a dry run? I could imagine that for some it may be interesting to see what is incorrect / will be cleaned up before this is called.

I am not sure of the use case. Maybe we could simply add LOG_WRN for each deleted setting?

I am not sure of the use case.

If I am a user and I have my settings stored in flash, I'd perhaps rather be aware of an issue in my settings before deleting them, rather than just silently deleting them, e.g. if I store my own settings and that I am foolish enough to store something under bt/id myself, then I'd rather become aware of that, rather than it just silently getting deleted by the stack

Thalley · 2023-07-10T13:29:30Z

include/zephyr/bluetooth/settings.h

+ * @warning User must not call this function if they use the 'bt/' namespace to
+ * store settings. All settings stored under 'bt/' namespace that are not
+ * defined by the Bluetooth stack will be deleted.


This seems like something that should be placed other places as well.

We should perhaps generally recommend (if not already) that applications do not use the bt/ namespace for their BT settings?

Actually, I think that sharing a settings namespace between an application and any Zephyr subsystem is a bad idea (applications need to use their own namespaces). Maybe we could assume that user should not do it?

subsys/bluetooth/host/settings.c

tests/bsim/bluetooth/host/settings/cleanup/src/test.c

subsys/bluetooth/host/settings.c

alwa-nordic · 2023-07-11T09:02:07Z

subsys/bluetooth/host/settings.c

+		LOG_ERR("Failed to read data (err %d)", len);
+	}
+
+	id_count = read_len / sizeof(id_addr[0]);


it could become alive :p
I suggest a BT_DBG("Read back %u identity addresses", id_count);.

alwa-nordic · 2023-07-11T09:14:32Z

The test fails if I add these:

	settings_save_one("bt/irk", &dummy_value, sizeof(dummy_value));
	settings_save_one("bt/irk/", &dummy_value, sizeof(dummy_value));
	settings_save_one("bt/irk/foo", &dummy_value, sizeof(dummy_value));

Should this be handled?

MarekPieta · 2023-07-13T14:09:48Z

subsys/bluetooth/host/settings.c

+	"mesh",
+};
+
+static bool mem_eq(const uint8_t *a1, size_t n1, const uint8_t *a2, size_t n2)


Maybe we could use create the is_supported_bt_key function instead (mem_eq is quite ambiguous name)?
That would also allow us to also limit scope of bt_settings_keys.

Was actually thinking the same during my review

subsys/bluetooth/host/settings.c

MarekPieta · 2023-07-13T14:24:02Z

subsys/bluetooth/host/settings.c

+	snprintk(full_key, sizeof(full_key), "bt/%s", key);
+	LOG_DBG("Key to delete: %s", full_key);
+
+	err = settings_delete(full_key);


Is deleting settings in the middle of load (exactly direct load) operation guaranteed to be safe?

It seems there are no guarantees, and it is the backend (ie nvs, file, etc) that does the iteration and callback invocation.
So yeah, to be on the safe side, we could store the full names somewhere (maybe a list of malloc'd strings) and do the deleting (and str freeing) after settings_load_subtree_direct("bt", bt_settings_load_direct_cleanup, &id_addr);.

MarekPieta · 2023-07-13T14:41:47Z

subsys/bluetooth/host/settings.c

+	int id_count;
+	bt_addr_le_t *id_addr = (bt_addr_le_t *)param;
+
+	read_len = read_cb(cb_arg, id_addr, CONFIG_BT_ID_MAX * sizeof(id_addr[0]));


Should we check for number of stored identities? The settings content may not be aligned with this expectations (e.g. after a DFU)

Yeah I'm also concerned about that. A test could check the num of IDs increasing and decreasing. idk how settings work but with the current logic i'd think increasing would read back garbage and decreasing would/could not read back the right identities.

Although I think the stack could break down in other places if we tried to decrease the number of bonds. At the very least, test increasing the number.

Maybe we could make use of len parameter of the direct load callback here? That could let you know the expected data length in advance.
btw. IIRC reading more bytes than available in the storage would simply lower the returned value of read_cb to indicate that not all bytes were actually read.

For details, see: https://docs.zephyrproject.org/apidoc/latest/group__settings.html#ga51cdac276c1fb8cd27fc3eec42749a04

We may also think about how to handle cases where the number of identities from settings readout does not match value defined in Kconfig.

subsys/bluetooth/host/settings.c

MarekPieta · 2023-07-13T14:58:10Z

subsys/bluetooth/host/settings.c

+			return 0;
+		}
+
+		next_len = settings_name_next(next, &id_str);


I wonder if we should somehow check if given record is expected to have associated Bluetooth address and identity string (e.g. hash should not have it)

maybe it should. That's way out of the scope of this PR, but storing the last hash seen by a given bond would simplify SC / caching handling quite a bit.

You are right. The main drawback is the fact that storing hash per bond would require quite significant changes in the implementation

MarekPieta · 2023-07-13T15:00:23Z

subsys/bluetooth/host/settings.c

+	int err;
+	bt_addr_le_t id_addr[CONFIG_BT_ID_MAX] = {0};
+
+	err = settings_load_subtree_direct("bt/id", bt_settings_load_direct_id, (void *)id_addr);


btw. Keep in mind that there may be another dependencies similar to this one. E.g. one may delete keys and leave CCC/CF/SC as a leftover data in storage. We may need to think about some more advanced way of handling settings dependencies at some point if we want to ensure fully proper implementation.

include/zephyr/bluetooth/settings.h

subsys/bluetooth/host/settings.c

include/zephyr/bluetooth/settings.h

jori-nordic · 2023-07-17T07:09:13Z

subsys/bluetooth/host/settings.c

+	int id_count;
+	bt_addr_le_t *id_addr = (bt_addr_le_t *)param;
+
+	read_len = read_cb(cb_arg, id_addr, CONFIG_BT_ID_MAX * sizeof(id_addr[0]));


Yeah I'm also concerned about that. A test could check the num of IDs increasing and decreasing. idk how settings work but with the current logic i'd think increasing would read back garbage and decreasing would/could not read back the right identities.

Although I think the stack could break down in other places if we tried to decrease the number of bonds. At the very least, test increasing the number.

jori-nordic · 2023-07-17T07:14:05Z

subsys/bluetooth/host/settings.c

+{
+	ssize_t read_len;
+	int id_count;
+	bt_addr_le_t *id_addr = (bt_addr_le_t *)param;


s/id_addr/id_addrs. I suggest the same for bt_settings_cleanup, this way it's unambiguous that this is an array of identity addresses and not a ptr to a single identity address.

jori-nordic · 2023-07-17T07:32:36Z

subsys/bluetooth/host/settings.c

+			id = strtoul(id_str, NULL, 10);
+
+			if (id >= CONFIG_BT_ID_MAX) {
+				LOG_ERR("Invalid local identity (%lu)", id);


Yeah i'd suggest a LOG_WRN("Invalid local identity: %lu > %lu", id, CONFIG_BT_ID_MAX);
I believe them's the guidelines: ERR for unrecoverable, WRN for recoverable/ignored.

subsys/bluetooth/host/settings.c

jori-nordic · 2023-07-17T07:35:26Z

subsys/bluetooth/host/settings.c

+
+	/* At this point, we know that the key need to be cleared */
+	snprintk(full_key, sizeof(full_key), "bt/%s", key);
+	LOG_DBG("Key to delete: %s", full_key);


suggest a "Deleting key: %s"

jori-nordic · 2023-07-17T07:42:19Z

subsys/bluetooth/host/settings.c

+	snprintk(full_key, sizeof(full_key), "bt/%s", key);
+	LOG_DBG("Key to delete: %s", full_key);
+
+	err = settings_delete(full_key);


It seems there are no guarantees, and it is the backend (ie nvs, file, etc) that does the iteration and callback invocation.
So yeah, to be on the safe side, we could store the full names somewhere (maybe a list of malloc'd strings) and do the deleting (and str freeing) after settings_load_subtree_direct("bt", bt_settings_load_direct_cleanup, &id_addr);.

jori-nordic · 2023-07-17T07:53:10Z

tests/bsim/bluetooth/host/settings/cleanup/src/test.c

+	bt_settings_store_cf(2, &dummy_addr, &dummy_value, sizeof(dummy_value));
+	bt_settings_store_ccc(2, &dummy_addr, &dummy_value, sizeof(dummy_value));
+
+	settings_save_one("bt/mesh/dummy/key", &dummy_value, sizeof(dummy_value));


re: @alwa-nordic 's comment: you could add a "bt/mes/somekey" setting for that.

Add a new function `bt_settings_cleanup` that allow users to clean potential leftover present in the Bluetooth settings. The function will look for every settings key under the 'bt/' namespace and remove the one not listed in the `bt_settings_keys` list. The settings that are linked to an ID that no longer exist will also be deleted. Signed-off-by: Théo Battrel <[email protected]>

Add a test for the new `bt_settings_cleanup` function. The test artificially write settings then call `bt_settings_cleanup` and check that the settings that should not have been deleted have not been. Signed-off-by: Théo Battrel <[email protected]>

theob-pro · 2023-07-25T08:24:14Z

This seems to be non-trivial to implement without a way to delete settings keys while loading them and trying to respect MISRA (ie no malloc).
This issue does not seem to be an actual problem for devices in the field (for now).

We are not happy with the current solution (w/ @alwa-nordic and @jori-nordic) so will close the PR.

theob-pro requested review from alwa-nordic, jhedberg, Vudentz, wopu-ot and sjanc as code owners June 28, 2023 13:01

zephyrbot added area: Bluetooth Host area: Bluetooth labels Jun 28, 2023

zephyrbot requested review from asbjornsabo, hermabe, jori-nordic, kruithofa and Thalley June 28, 2023 13:02

zephyrbot assigned jhedberg Jun 28, 2023

theob-pro force-pushed the settings-cleanup branch from 916c929 to 03e3c73 Compare June 28, 2023 13:09

MarekPieta self-requested a review June 30, 2023 10:18

alwa-nordic requested changes Jul 3, 2023

View reviewed changes

include/zephyr/bluetooth/settings.h Outdated Show resolved Hide resolved

tests/bsim/bluetooth/host/settings/cleanup/src/test.c Show resolved Hide resolved

subsys/bluetooth/host/settings.c Show resolved Hide resolved

theob-pro force-pushed the settings-cleanup branch from 03e3c73 to 9a3d27c Compare July 4, 2023 06:08

alwa-nordic requested changes Jul 4, 2023

View reviewed changes

tests/bsim/bluetooth/host/settings/cleanup/src/test.c Show resolved Hide resolved

theob-pro force-pushed the settings-cleanup branch 2 times, most recently from 14025a8 to 1af09fd Compare July 5, 2023 08:35

alwa-nordic approved these changes Jul 5, 2023

View reviewed changes

Thalley requested changes Jul 10, 2023

View reviewed changes

alwa-nordic reviewed Jul 11, 2023

View reviewed changes

subsys/bluetooth/host/settings.c Outdated Show resolved Hide resolved

alwa-nordic requested changes Jul 11, 2023

View reviewed changes

MarekPieta suggested changes Jul 13, 2023

View reviewed changes

jori-nordic reviewed Jul 17, 2023

View reviewed changes

theob-pro added 2 commits July 20, 2023 09:35

theob-pro force-pushed the settings-cleanup branch from 1af09fd to 33ec6ed Compare July 20, 2023 07:36

MarekPieta self-requested a review July 21, 2023 07:20

theob-pro closed this Jul 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bluetooth: Host: Add API to clean BT settings #59821

Bluetooth: Host: Add API to clean BT settings #59821

theob-pro commented Jun 28, 2023

jhedberg commented Jul 5, 2023

jori-nordic commented Jul 5, 2023

theob-pro commented Jul 5, 2023

Thalley left a comment

Thalley Jul 10, 2023

theob-pro Jul 12, 2023

Thalley Jul 12, 2023

Thalley Jul 10, 2023

MarekPieta Jul 13, 2023 •

edited

Loading

alwa-nordic Jul 11, 2023

jori-nordic Jul 17, 2023

alwa-nordic commented Jul 11, 2023

MarekPieta Jul 13, 2023

Thalley Jul 13, 2023

MarekPieta Jul 13, 2023

jori-nordic Jul 17, 2023

MarekPieta Jul 13, 2023

jori-nordic Jul 17, 2023

MarekPieta Jul 21, 2023

MarekPieta Jul 21, 2023

MarekPieta Jul 13, 2023

jori-nordic Jul 17, 2023

MarekPieta Jul 21, 2023

MarekPieta Jul 13, 2023

jori-nordic Jul 17, 2023

jori-nordic Jul 17, 2023

jori-nordic Jul 17, 2023

jori-nordic Jul 17, 2023

jori-nordic Jul 17, 2023

jori-nordic Jul 17, 2023

theob-pro commented Jul 25, 2023

Bluetooth: Host: Add API to clean BT settings #59821

Bluetooth: Host: Add API to clean BT settings #59821

Conversation

theob-pro commented Jun 28, 2023

jhedberg commented Jul 5, 2023

jori-nordic commented Jul 5, 2023

theob-pro commented Jul 5, 2023

Thalley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarekPieta Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alwa-nordic commented Jul 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theob-pro commented Jul 25, 2023

MarekPieta Jul 13, 2023 •

edited

Loading