subsys: settings: Invalidate entry with the same key in FCB

[Olivier-ProGlove]

Without this PR, entries with the same key name are present in FCB setting partition as many time as they have been written.
All these duplicated keys are loaded at the initialisation of the settings subsystem. The consumer of these settings will be notified for each of these key.

It could lead to issues in the caller logic.

See my comment to see how it affects Zephyr bluetooth stack: #11409 (comment)

With this change, support for deleting settings should be easier to implement properly. Today, to delete a key, users use settings_save_one("mykey", NULL). Rather than deleting the key it creates a new key with the NULL value.

Signed-off-by: Olivier Martin [email protected]

[zephyrbot]

Found the following issues, please fix and resubmit:

Checkpatch issues

-:60: WARNING:LONG_LINE: line over 80 characters
#60: FILE: subsys/settings/src/settings_fcb.c:119:
+	rc = settings_line_parse(buf + FCB_SETTINGS_EXTRA_LEN, &name_str, &val_str);

-:100: ERROR:POINTER_LOCATION: "foo* bar" should be "foo *bar"
#100: FILE: subsys/settings/src/settings_fcb.c:242:
+	const char* entry_name = arg;

-:126: WARNING:LONG_LINE: line over 80 characters
#126: FILE: subsys/settings/src/settings_fcb.c:268:
+	rc = settings_line_parse(buf + FCB_SETTINGS_EXTRA_LEN, &name_str, &val_str);

-:186: ERROR:POINTER_LOCATION: "(foo*)" should be "(foo *)"
#186: FILE: subsys/settings/src/settings_fcb.c:330:
+	rc = fcb_walk(&cf->cf_fcb, 0, settings_fcb_invalid_entry, (char*)name);

-:213: WARNING:LONG_LINE_STRING: line over 80 characters
#213: FILE: tests/subsys/settings/fcb/src/settings_test_fcb.c:99:
+		zassert_false(val8_is_set, "SETTINGS_VALUE_SET callback - val8 loaded twice");

-:222: WARNING:LONG_LINE_STRING: line over 80 characters
#222: FILE: tests/subsys/settings/fcb/src/settings_test_fcb.c:108:
+		zassert_false(val64_is_set, "SETTINGS_VALUE_SET callback - val64 loaded twice");

-:243: WARNING:LONG_LINE_STRING: line over 80 characters
#243: FILE: tests/subsys/settings/fcb/src/settings_test_fcb.c:305:
+		zassert_false(val32_is_set, "SETTINGS_VALUE_SET callback - val32 loaded twice");

- total: 2 errors, 5 warnings, 241 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

[nvlsianpu]

I will add this patch to my PR #9521.
So coding style improvement not required here.
Please DNM this before #9521 (will try to finish mine PR today).

[nvlsianpu]

Without this PR, entries with the same key name are present in FCB setting partition as many time as they have been written.
All these duplicated keys are loaded at the initialisation of the settings subsystem. The consumer of these settings will be notified for each of these key.

It could lead to issues in the caller logic.

This is the reason for implementing keep h_commit handler. When loading of settings is done this handler is called for informing the application that most recently configuration was loaded.

I reviewed the algorithm. As now I see it is using read-modify-write for making any name-value pair invalid.
Such operation is forbidden for many of Flash devices -some of them have ECC feutre, others has appropriate obstacle described in their manuals.

So if you want to introduce something like this you should align size of the new field you introduced to the smallest write block for the flash device (this can be fetched form the flash driver API).

I also recommend to put changes onto #9521 because it changes a lot entire implementation of the settings subsystem.

Feature you introduced should has appropriate kconfig key, at last to inform application at compile time that such feature is enabled (probably shouldn't be enabled for FS). I can stand something like SETTINGS_AVOID_MULTIPLE _LOADS or so.

[nvlsianpu]

see my comment #11574 (comment)

[jhedberg]

Is this one still valid? Should it be rebased or just closed without merging?

[nvlsianpu]

As wrote before:

I reviewed the algorithm. As now I see it is using read-modify-write for making any name-value pair invalid.
Such operation is forbidden for many of Flash devices -some of them have ECC feutre, others has appropriate obstacle described in their manuals.

So if you want to introduce something like this you should align size of the new field you introduced to the smallest write block for the flash device (this can be fetched form the flash driver API).

So rebase is not enough - this is technical issue.

[nvlsianpu]

@Olivier-ProGlove Do you still want to work on this PR? I believe the objective behind this PR was fulfilled by #19541.

[nashif]

no response, closing