Settings filter duplicates

doc/reference/storage/settings/settings.rst

@@ -94,13 +94,16 @@ Loading data from persisted storage
 
 A call to ``settings_load()`` uses an ``h_set`` implementation
 to load settings data from storage to volatile memory.
-For both FCB and filesystem back-end the most
-recent key values are guaranteed by traversing all stored content
-and (potentially) overwriting older key values with newer ones.
 After all data is loaded, the ``h_commit`` handler is issued,
 signalling the application that the settings were successfully
 retrieved.
 
+Technically FCB and filesystem backends may store some history of the entities.
+This means that the newest data entity is stored after any
+older existing data entities.
+Starting with Zephyr 2.1, the back-end must filter out all old entities and
+call the callback with only the newest entity.
+
 Storing data to persistent storage
 **********************************
 

include/settings/settings.h

@@ -390,6 +390,12 @@ struct settings_store_itf {
 	 * Parameters:
 	 *  - cs - Corresponding backend handler node,
 	 *  - arg - Structure that holds additional data for data loading.
+	 *
+	 * @note
+	 * Backend is expected not to provide duplicates of the entities.
+	 * It means that if the backend does not contain any functionality to
+	 * really delete old keys, it has to filter out old entities and call
+	 * load callback only on the final entity.
 	 */
 
 	int (*csi_save_start)(struct settings_store *cs);

subsys/settings/src/settings_fcb.c

@@ -6,8 +6,10 @@
  */
 
 #include <errno.h>
+#include <stdbool.h>
 #include <fs/fcb.h>
 #include <string.h>
+#include <assert.h>
 
 #include "settings/settings.h"
 #include "settings/settings_fcb.h"
@@ -18,11 +20,6 @@ LOG_MODULE_DECLARE(settings, CONFIG_SETTINGS_LOG_LEVEL);
 
 #define SETTINGS_FCB_VERS		1
 
-struct settings_fcb_load_cb_arg {
-	line_load_cb cb;
-	void *cb_arg;
-};
-
 static int settings_fcb_load(struct settings_store *cs,
 			     const struct settings_load_arg *arg);
 static int settings_fcb_save(struct settings_store *cs, const char *name,
@@ -79,52 +76,102 @@ int settings_fcb_dst(struct settings_fcb *cf)
 	return 0;
 }
 
-static int settings_fcb_load_cb(struct fcb_entry_ctx *entry_ctx, void *arg)
+/**
+ * @brief Check if there is any duplicate of the current setting
+ *
+ * This function checks if there is any duplicated data further in the buffer.
+ *
+ * @param cf        FCB handler
+ * @param entry_ctx Current entry context
+ * @param name      The name of the current entry
+ *
+ * @retval false No duplicates found
+ * @retval true  Duplicate found
+ */
+static bool settings_fcb_check_duplicate(struct settings_fcb *cf,
+					const struct fcb_entry_ctx *entry_ctx,
+					const char * const name)
 {
-	struct settings_fcb_load_cb_arg *argp;
-	char buf[SETTINGS_MAX_NAME_LEN + SETTINGS_EXTRA_LEN + 1];
-	int rc;
+	struct fcb_entry_ctx entry2_ctx = *entry_ctx;
 
-	argp = (struct settings_fcb_load_cb_arg *)arg;
+	while (fcb_getnext(&cf->cf_fcb, &entry2_ctx.loc) == 0) {
+		char name2[SETTINGS_MAX_NAME_LEN + SETTINGS_EXTRA_LEN + 1];
+		size_t name2_len;
 
-	size_t len_read;
+		if (settings_line_name_read(name2, sizeof(name2), &name2_len,
+					    &entry2_ctx)) {
+			LOG_ERR("failed to load line");
+			continue;
+		}
+		name2[name2_len] = '\0';
+		if (!strcmp(name, name2)) {
+			return true;
+		}
+	}
+	return false;
+}
 
-	rc = settings_line_name_read(buf, sizeof(buf), &len_read,
-				     (void *)&entry_ctx->loc);
-	if (rc) {
+static int read_entry_len(const struct fcb_entry_ctx *entry_ctx, off_t off)
+{
+	if (off >= entry_ctx->loc.fe_data_len) {
 		return 0;
 	}
-	buf[len_read] = '\0';
-
-	/*name, val-read_cb-ctx, val-off*/
-	/* take into account '=' separator after the name */
-	argp->cb(buf, (void *)&entry_ctx->loc, len_read + 1, argp->cb_arg);
-	return 0;
+	return entry_ctx->loc.fe_data_len - off;
 }
 
-static int settings_fcb_load_priv(struct settings_store *cs, line_load_cb cb,
-				  void *cb_arg)
+static int settings_fcb_load_priv(struct settings_store *cs,
+				  line_load_cb cb,
+				  void *cb_arg,
+				  bool filter_duplicates)
 {
 	struct settings_fcb *cf = (struct settings_fcb *)cs;
-	struct settings_fcb_load_cb_arg arg;
+	struct fcb_entry_ctx entry_ctx = {
+		{.fe_sector = NULL, .fe_elem_off = 0},
+		.fap = cf->cf_fcb.fap
+	};
 	int rc;
 
-	arg.cb = cb;
-	arg.cb_arg = cb_arg;
-	rc = fcb_walk(&cf->cf_fcb, 0, settings_fcb_load_cb, &arg);
-	if (rc) {
-		return -EINVAL;
+	while ((rc = fcb_getnext(&cf->cf_fcb, &entry_ctx.loc)) == 0) {
+		char name[SETTINGS_MAX_NAME_LEN + SETTINGS_EXTRA_LEN + 1];
+		size_t name_len;
+		int rc;
+		bool pass_entry = true;
+
+		rc = settings_line_name_read(name, sizeof(name), &name_len,
+					     (void *)&entry_ctx);
+		if (rc) {
+			LOG_ERR("Failed to load line name: %d", rc);
+			continue;
+		}
+		name[name_len] = '\0';
+
+		if (filter_duplicates &&
+		    (!read_entry_len(&entry_ctx, name_len+1) ||
+		     settings_fcb_check_duplicate(cf, &entry_ctx, name))) {
+			pass_entry = false;
+		}
+		/*name, val-read_cb-ctx, val-off*/
+		/* take into account '=' separator after the name */
+		if (pass_entry) {
+			cb(name, &entry_ctx, name_len + 1, cb_arg);
+		}
+	}
+	if (rc == -ENOTSUP) {
+		rc = 0;
 	}
 	return 0;
 }
 
 static int settings_fcb_load(struct settings_store *cs,
 			     const struct settings_load_arg *arg)
 {
-	return settings_fcb_load_priv(cs, settings_line_load_cb, (void *)arg);
+	return settings_fcb_load_priv(
+		cs,
+		settings_line_load_cb,
+		(void *)arg,
+		true);
 }
 
-
 static int read_handler(void *ctx, off_t off, char *buf, size_t *len)
 {
 	struct fcb_entry_ctx *entry_ctx = ctx;
@@ -307,7 +354,7 @@ static int settings_fcb_save(struct settings_store *cs, const char *name,
 	cdca.val = (char *)value;
 	cdca.is_dup = 0;
 	cdca.val_len = val_len;
-	settings_fcb_load_priv(cs, settings_line_dup_check_cb, &cdca);
+	settings_fcb_load_priv(cs, settings_line_dup_check_cb, &cdca, false);
 	if (cdca.is_dup == 1) {
 		return 0;
 	}

subsys/settings/src/settings_file.c

@@ -6,6 +6,7 @@
  */
 
 #include <string.h>
+#include <stdbool.h>
 #include <zephyr.h>
 
 #include <fs/fs.h>
@@ -14,6 +15,10 @@
 #include "settings/settings_file.h"
 #include "settings_priv.h"
 
+#include <logging/log.h>
+LOG_MODULE_DECLARE(settings, CONFIG_SETTINGS_LOG_LEVEL);
+
+
 static int settings_file_load(struct settings_store *cs,
 			      const struct settings_load_arg *arg);
 static int settings_file_save(struct settings_store *cs, const char *name,
@@ -49,19 +54,63 @@ int settings_file_dst(struct settings_file *cf)
 	return 0;
 }
 
+/**
+ * @brief Check if there is any duplicate of the current setting
+ *
+ * This function checks if there is any duplicated data further in the buffer.
+ *
+ * @param cf        FCB handler
+ * @param entry_ctx Current entry context
+ * @param name      The name of the current entry
+ *
+ * @retval false No duplicates found
+ * @retval true  Duplicate found
+ */
+bool settings_file_check_duplicate(struct settings_file *cf,
+				  const struct line_entry_ctx *entry_ctx,
+				  const char * const name)
+{
+	struct line_entry_ctx entry2_ctx = *entry_ctx;
+
+	/* Searching the duplicates */
+	while (settings_next_line_ctx(&entry2_ctx) == 0) {
+		char name2[SETTINGS_MAX_NAME_LEN + SETTINGS_EXTRA_LEN + 1];
+		size_t name2_len;
+
+		if (entry2_ctx.len == 0) {
+			break;
+		}
+
+		if (settings_line_name_read(name2, sizeof(name2), &name2_len,
+					    &entry2_ctx)) {
+			continue;
+		}
+		name2[name2_len] = '\0';
+
+		if (!strcmp(name, name2)) {
+			return true;
+		}
+	}
+	return false;
+}
+
+static int read_entry_len(const struct line_entry_ctx *entry_ctx, off_t off)
+{
+	if (off >= entry_ctx->len) {
+		return 0;
+	}
+	return entry_ctx->len - off;
+}
 
 static int settings_file_load_priv(struct settings_store *cs, line_load_cb cb,
-				   void *cb_arg)
+				   void *cb_arg, bool filter_duplicates)
 {
 	struct settings_file *cf = (struct settings_file *)cs;
-	char buf[SETTINGS_MAX_NAME_LEN + SETTINGS_EXTRA_LEN + 1];
 	struct fs_dirent file_info;
-	struct fs_file_t  file;
-	size_t len_read;
+	struct fs_file_t file;
 	int lines;
 	int rc;
 
-
 	struct line_entry_ctx entry_ctx = {
 		.stor_ctx = (void *)&file,
 		.seek = 0,
@@ -81,23 +130,33 @@ static int settings_file_load_priv(struct settings_store *cs, line_load_cb cb,
 	}
 
 	while (1) {
-		rc = settings_next_line_ctx(&entry_ctx);
+		char name[SETTINGS_MAX_NAME_LEN + SETTINGS_EXTRA_LEN + 1];
+		size_t name_len;
+		bool pass_entry = true;
 
+		rc = settings_next_line_ctx(&entry_ctx);
 		if (rc || entry_ctx.len == 0) {
 			break;
 		}
 
-		rc = settings_line_name_read(buf, sizeof(buf), &len_read,
-					     (void *)&entry_ctx);
+		rc = settings_line_name_read(name, sizeof(name), &name_len,
+					     &entry_ctx);
 
-		if (rc || len_read == 0) {
+		if (rc || name_len == 0) {
 			break;
 		}
-		buf[len_read] = '\0';
+		name[name_len] = '\0';
 
+		if (filter_duplicates &&
+		    (!read_entry_len(&entry_ctx, name_len+1) ||
+		     settings_file_check_duplicate(cf, &entry_ctx, name))) {
+			pass_entry = false;
+		}
 		/*name, val-read_cb-ctx, val-off*/
 		/* take into account '=' separator after the name */
-		cb(buf, (void *)&entry_ctx, len_read + 1, cb_arg);
+		if (pass_entry) {
+			cb(name, (void *)&entry_ctx, name_len + 1, cb_arg);
+		}
 		lines++;
 	}
 
@@ -113,7 +172,10 @@ static int settings_file_load_priv(struct settings_store *cs, line_load_cb cb,
 static int settings_file_load(struct settings_store *cs,
 			      const struct settings_load_arg *arg)
 {
-	return settings_file_load_priv(cs, settings_line_load_cb, (void *)arg);
+	return settings_file_load_priv(cs,
+				       settings_line_load_cb,
+				       (void *)arg,
+				       true);
 }
 
 static void settings_tmpfile(char *dst, const char *src, char *pfx)
@@ -359,7 +421,7 @@ static int settings_file_save(struct settings_store *cs, const char *name,
 	cdca.val = (char *)value;
 	cdca.is_dup = 0;
 	cdca.val_len = val_len;
-	settings_file_load_priv(cs, settings_line_dup_check_cb, &cdca);
+	settings_file_load_priv(cs, settings_line_dup_check_cb, &cdca, false);
 	if (cdca.is_dup == 1) {
 		return 0;
 	}

tests/subsys/settings/functional/file/CMakeLists.txt

@@ -0,0 +1,19 @@
+# SPDX-License-Identifier: Apache-2.0
+
+cmake_minimum_required(VERSION 3.13.1)
+include($ENV{ZEPHYR_BASE}/cmake/app/boilerplate.cmake NO_POLICY_SCOPE)
+project(NONE)
+
+FILE(GLOB app_sources ../src/*.c)
+target_sources(app PRIVATE ${app_sources})
+zephyr_include_directories(
+	$ENV{ZEPHYR_BASE}/subsys/settings/include
+	$ENV{ZEPHYR_BASE}/subsys/settings/src
+	$ENV{ZEPHYR_BASE}/tests/subsys/settings/nffs/src
+	)
+
+if(TEST)
+	target_compile_definitions(app PRIVATE
+		-DTEST_${TEST}
+		)
+endif()

tests/subsys/settings/functional/file/native_posix.overlay

@@ -0,0 +1,25 @@
+/*
+ * Copyright (c) 2019 Jan Van Winkel <[email protected]>
+ *
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+/delete-node/ &storage_partition;
+/delete-node/ &scratch_partition;
+
+&flash0 {
+	/*
+	 * For more information, see:
+	 * http://docs.zephyrproject.org/latest/guides/dts/index.html#flash-partitions
+	 */
+	partitions {
+		compatible = "fixed-partitions";
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		storage_partition: partition@70000 {
+			label = "storage";
+			reg = <0x00070000 0x10000>;
+		};
+	};
+};