util/scfg: Simple config lib on top of sys/config #2182

ccollins476ad · 2020-02-07T22:10:29Z

High Level

The sys/config package lets an app persist and restore data in permanent storage. It is documented here: http://mynewt.apache.org/latest/os/modules/config/config.html.

This package does its job well and its design is sound. It has one minor problem though (in my opinion, in case that needs saying) - it is nearly impossible to use! Here is an example: https://github.com/apache/mynewt-core/blob/master/boot/split/src/split_config.c. This file implements a single 8-bit setting called split/status. This is not an extreme example; it is how all sys/config client code looks. Speaking for myself, it is a major implementation effort whenever I need to add a new setting, or worse, a new setting group. As basic and fundamental as data persistence is to an embedded application, this should be an easier task.

The problem is obvious: the config package is very powerful, and with that power comes a very complex API.

I think most application code would be fine with a less powerful library. Such a library would allow all settings to be loaded, individual settings to be saved on demand, and very little else. It would not be possible to commit several settings at once or to execute custom code when a setting is saved or loaded. With this simplified set of requirements in mind, I came up with a wish list for an API:

It should be easy to create a new setting group. Ideally you could just copy and paste from existing code.
Creating a new setting should be as simple as appending an element to an array.
Persisting a setting should be possible with a single function call.

So I took that and implemented... something. The library is called "scfg" (short for "simple config"). I'll go into more details below, but first, here is how that split/status configuration would be done using the new library: #2183 [1]. I think this is a marked improvement, but please judge for yourselves.

Implementation

I had the idea that we could just build a simple library on top of the existing sys/config library. Unfortunately, sys/config doesn't lend itself well to this kind of a wrapper library. The problem is that the conf_handler callbacks don't accept a void *arg parameter, meaning every config group must define a dedicated set of callback functions. This is an issue because scfg needs to define a generic set of callbacks for all config groups. So I had to modify sys/config so that the callbacks accepted an extra generic argument, and this had to be done in a backwards compatible way. It's not pretty, but this is what I came up with: #2181.

The scfg library required a second change to sys/config: Support for unsigned integer types. Only signed integer types are currently supported. Before my change, when a config setting needed to use an unsigned type, the handler would work around this limitation by specifying a signed integer type that is slightly wider than the actual data. Then the conf handler callbacks would manually convert between the unsigned and signed types. Since scfg does not allow for arbitrary code to run during save and restore operations, the library needs to handle unsigned types natively. I made this change here: #2180.

Questions

Do we want to enable extended callbacks in sys/config by default, or should this require a syscfg setting to be enabled? This change adds eight bytes to every config group which is a bummer. I prefer that we enable this change unconditionally. I think a simplified config interface is valuable enough that developers should be able to assume it is available.
Does scfg need some sort of "on-loaded" callback for each setting? My feeling is no, we don't need that. If applications need to know when settings are restored, then we can modify sys/config to allow callbacks to be registered. These callbacks would be executed after conf_load() completes.
Do we need support for binary data? sys/config only allows text values for settings. Binary blobs must be converted to text using something like base64 before they are saved. This is a nuissance for large settings (e.g., nimble host security data). I would love it if we could support raw binary values, but my impression is that this would require massive changes to the sys/config library. I think this change would be quite valuable, but it is a feature in itself and it should be considered separately.

All comments are welcome.

[1] It seems the split image feature is broken again. In light of that, this probably wasn't the best group to use as an example!

apache-mynewt-bot · 2020-02-07T22:13:59Z

RAT Report (2020-02-07 22:13:56)

New files with unknown licenses

Detailed analysis

New files in this PR

License	File
?????	util/scfg/include/scfg/scfg.h
AL	util/scfg/pkg.yml
?????	util/scfg/src/scfg.c

utzig

Left a few doubts, but looks OK.

utzig · 2020-02-18T10:37:16Z

util/scfg/pkg.yml

+#
+
+pkg.name: util/scfg
+pkg.description: XXX


How about a better description?

Oops :). Fixed.

utzig · 2020-02-18T10:51:57Z

util/scfg/src/scfg.c

+    off += setting_len;
+
+    buf[off] = '\0';
+    off++;


There's no need to increment a local variable here I think...

Thanks, fixed.

utzig · 2020-02-18T10:56:23Z

util/scfg/src/scfg.c

+    setting_len = strlen(setting_name);
+
+    /* <group>/<setting> */
+    assert(group_len + 1 + setting_len <= SCFG_MAX_SETTING_ID_LEN);


I think the test should be < here, because if the setting name length is == SCFG_MAX_SETTING_ID_LEN the code would write the ending \0 one past the end of the buffer....

Good catch, thanks.

I am making two changes:

Change that constant into a syscfg setting.

Allocate max_len + 1 bytes to hold the setting ID (i.e., max_len refers to maximum string length, not buffer size).

Re: 2- I think that is better because it makes the system easier to configure. The user can set the value to exactly the number of ascii characters required (and not have to think about null terminators).

utzig · 2020-02-18T10:57:28Z

util/scfg/include/scfg/scfg.h

+     * Whether this setting contains private data.  If 1, the value is hidden
+     * in config dump output.
+     */
+    uint8_t f_private : 1;


Is there something to be gained by using a bit field here? This looks like it would be better by being a bool.

Thanks, I have fixed it. Initially I had a second flag here and I wanted to save space. Since we only have one, it is better to use bool.

utzig · 2020-02-18T11:06:25Z

util/scfg/src/scfg.c

+
+    SCFG_FOREACH_SETTING(group, setting) {
+        if (strcmp(setting->name, setting_name) == 0) {
+            return (struct scfg_setting *)setting;


Does this cast need to be here?

Yes, I believe it does. The cast "casts away" const. This is the same approach used by strstr for example,

char *strstr(const char *haystack, const char *needle);

Since C doesn't let you overload functions based on the "constness" of their parameters (not complaining!), we have to handle both const and non-const with the same function. The approach that the standard library takes is to always accept const arguments and always return non-const. It's not perfect, but I think it is reasonable.

The cast "casts away" const.

Ah, sorry, I totally missed the const keyword here, my bad!

utzig · 2020-02-18T11:06:39Z

util/scfg/src/scfg.c

+
+    SCFG_FOREACH_SETTING(group, setting) {
+        if (setting->val == val) {
+            return (struct scfg_setting *)setting;


Same cast doubt...

apache-mynewt-bot · 2020-02-19T18:19:30Z

Style check summary

No suggestions at this time!

utzig

Looks good, thanks!

ccollins476ad force-pushed the scfg branch 2 times, most recently from a743069 to e6c65da Compare February 7, 2020 22:20

ccollins476ad mentioned this pull request Feb 8, 2020

sys/config: Extended callbacks #2181

Merged

ccollins476ad force-pushed the scfg branch from e6c65da to b8db31a Compare February 12, 2020 21:40

utzig reviewed Feb 18, 2020

View reviewed changes

utzig approved these changes Feb 19, 2020

View reviewed changes

util/scfg: Simple config lib on top of sys/config

3f0d41b

ccollins476ad force-pushed the scfg branch from 4a8c946 to 3f0d41b Compare February 20, 2020 17:00

ccollins476ad changed the title ~~[RFC] util/scfg: Simple config lib on top of sys/config~~ util/scfg: Simple config lib on top of sys/config Feb 20, 2020

ccollins476ad merged commit 6f459fc into apache:master Feb 20, 2020

ccollins476ad deleted the scfg branch February 20, 2020 17:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util/scfg: Simple config lib on top of sys/config #2182

util/scfg: Simple config lib on top of sys/config #2182

ccollins476ad commented Feb 7, 2020 •

edited

apache-mynewt-bot commented Feb 7, 2020

New files in this PR

utzig left a comment

utzig Feb 18, 2020

ccollins476ad Feb 19, 2020

utzig Feb 18, 2020

ccollins476ad Feb 19, 2020

utzig Feb 18, 2020

ccollins476ad Feb 19, 2020

utzig Feb 18, 2020

ccollins476ad Feb 19, 2020

utzig Feb 18, 2020

ccollins476ad Feb 19, 2020

utzig Feb 19, 2020

utzig Feb 18, 2020

apache-mynewt-bot commented Feb 19, 2020

utzig left a comment

util/scfg: Simple config lib on top of sys/config #2182

util/scfg: Simple config lib on top of sys/config #2182

Conversation

ccollins476ad commented Feb 7, 2020 • edited

High Level

Implementation

Questions

apache-mynewt-bot commented Feb 7, 2020

RAT Report (2020-02-07 22:13:56)

New files with unknown licenses

New files in this PR

utzig 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

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

apache-mynewt-bot commented Feb 19, 2020

Style check summary

No suggestions at this time!

utzig left a comment

Choose a reason for hiding this comment

ccollins476ad commented Feb 7, 2020 •

edited