sys: Add RIOT Registry implementation by leandrolanzieri · Pull Request #10799 · RIOT-OS/RIOT

leandrolanzieri · 2019-01-17T11:16:31Z

Contribution description

This adds the implementation of the RIOT Registry as discussed in #10622. This module is used for interacting with persistent key-value configurations, and is part of the ongoing effort to implement a Runtime Configuration System for RIOT nodes.

The RIOT Registry allows to register configuration groups via handlers, and non-volatile devices via storage facilities, and access the configuration parameters via its API.

Currently there are 3 storage facilities implemented, and more can be added as needed:

Dummy storage (just for debugging and testing purposes)
EEPROM storage (implemented using the eepreg module) (sys/registry: Add EEPROM storage #10910)
File storage (uses FAT file system) (sys/registry: Add File storage #10911)

Testing procedure

There are unit tests for testing the RIOT Registry API (tests/unittests/tests-registry), and a test application that also serves as example or starting point for usage (tests/registry).

Issues/PRs references

EEPROM storage: #10910
File storage: #10911
Fixes #5773

jia200x · 2019-01-17T14:07:17Z

@@ -0,0 +1,24 @@
+#ifndef REGISTRY_STORE_DUMMY_H


I would put these dummy storage only as a test, not under sys

danpetry

Here's a review of the high level aspects.

The testing philosophy needs some attention IMO. Specifically:

The application that is currently in the tests/ directory combines several modules that are individually testable and also requires changes to the makefile to run each test, which doesn’t make it super easy to use or CI-ready. It could make a good example/ application instead, what do you think? To have this would also be a good “advertisement” for the registry
Please explicitly and individually write tests for all API calls. That is for the registry, registry conversion module, and SFs. (You could have all tests be unit tests and mocking the interfaces using stubs like the dummy storage facility)
edge cases and things that could potentially break things (handing over null pointers etc) aren’t tested
The SFs aren’t tested extensibly – e.g. how do people implement tests for their SF when they add one? I’d suggest a separate test for each.

Please make sure the tests, once you’ve got thorough coverage, compile and run successfully, with all tests passing, on all boards that aren’t blacklisted or that throw warnings due to unavailable features. Use FEATURES_REQUIRED += as appropriate as well as blacklisting boards that don’t build the test applications in Murdock
folder structure/placement of stuff

Suggestion: registry_store stuff could go into registry.c to keep it all together? Or the file into the base folder maybe?
would all SFs and RHs be implemented in sys/registry? Or would they be in with their module? IMO whatever we do for SFs should be done for RHs to keep it consistent.

Should we stipulate naming conventions for SF and RH APIs, or just set a precedent through these initial submissions? (Open question.)
Would you consider separating out the storage facility implementations into separate PRs? This would make reviewing the whole thing more manageable.
Helpers haven’t been implemented in the configuration manager (shell) in the test application to modify access control settings and enable and disable communication interfaces? The architecture doc says this MUST be done… which is wrong, the RDM or the tests/ application?
Please bring the submission up to a final quality (i.e. not WIP) before further review of aspects 2-5
Please rebase following the merge of PRs 10796 and 10802

leandrolanzieri · 2019-01-31T12:28:24Z

Hi @danpetry, thanks for the high level review, here are some comments:

Here's a review of the high level aspects.

The testing philosophy needs some attention IMO. Specifically:

The application that is currently in the tests/ directory combines several modules that are individually testable and also requires changes to the makefile to run each test, which doesn’t make it super easy to use or CI-ready. It could make a good example/ application instead, what do you think? To have this would also be a good “advertisement” for the registry

Are you suggesting to add a test app per storage facility? I think that may be overkill, the only thing to change on the test app is the Makefile to select the wanted storage (the file storage adds some other parameters for configuring the file system, but that is independent of the registry). I think an example would be worth when other modules of the Runtime Configuration System are merged (e.g. the configuration managers).

Please explicitly and individually write tests for all API calls. That is for the registry, registry conversion module, and SFs. (You could have all tests be unit tests and mocking the interfaces using stubs like the dummy storage facility)

Added unit tests for all the registry API (also removed some that I figured should not be part of the public API)

edge cases and things that could potentially break things (handing over null pointers etc) aren’t tested

Check the updated code and please point out the edge cases so I can add them to the unit tests.

The SFs aren’t tested extensibly – e.g. how do people implement tests for their SF when they add one? I’d suggest a separate test for each.

They can be added to the test app. If it is really needed separate test can be implemented for that particular storage.

Please make sure the tests, once you’ve got thorough coverage, compile and run successfully, with all tests passing, on all boards that aren’t blacklisted or that throw warnings due to unavailable features. Use FEATURES_REQUIRED += as appropriate as well as blacklisting boards that don’t build the test applications in Murdock

Fixed the problems with the file storage (#10911).

folder structure/placement of stuff

Suggestion: registry_store stuff could go into registry.c to keep it all together? Or the file into the base folder maybe?

I found a bit cleaner to separate the conversions and the store functions from the main functionalities. Now all of them are in the base registry folder.

would all SFs and RHs be implemented in sys/registry? Or would they be in with their module? IMO whatever we do for SFs should be done for RHs to keep it consistent.

SFs are placed under sys/registry/store. The handlers are implemented in apps or other modules, so they should not be placed in the registry folder.

Would you consider separating out the storage facility implementations into separate PRs? This would make reviewing the whole thing more manageable.

Good idea, done.

Helpers haven’t been implemented in the configuration manager (shell) in the test application to modify access control settings and enable and disable communication interfaces? The architecture doc says this MUST be done… which is wrong, the RDM or the tests/ application?

The shell in this test application is not the suggested CLI (though pretty similar) from the RDM. The RDM has been updated to indicate that the configuration managers MAY implement this access control among other functionalities. Thanks for pointing that out.

Please bring the submission up to a final quality (i.e. not WIP) before further review of aspects 2-5

Cleaned up and removed the WIP state now.

Please rebase following the merge of PRs 10796 and 10802

Done!

danpetry · 2019-02-04T14:49:13Z

Hi Leandro, sorry for the delay

Are you suggesting to add a test app per storage facility? I think that may be overkill, the only thing to change on the test app is the Makefile to select the wanted storage

Are there any other tests where the makefile needs to be changed or there are #defines to change tests? If so, then I'm ok with this. (I saw the unittests btw - looks good at first glance, thank you, and all the tests build correctly for the platforms I tried)

an example would be worth when other modules of the Runtime Configuration System are merged

sounds sensible

Check the updated code and please point out the edge cases

I'll have a closer look while doing the next round of review, maybe write some extra tests as a PR

SFs are placed under sys/registry/store. The handlers are implemented in apps or other modules, so they should not be placed in the registry folder.

ok, seems sensible, doesn't seem like it's feasible to make it symmetric

All other points on your post I would say agreed/fine/etc for.

danpetry · 2019-02-04T14:49:38Z

I will do a review of the other aspects asap.

leandrolanzieri · 2019-02-20T10:21:15Z

@kaspar030 any chance you could also take a look?

danpetry

As discussed offline, these are the comments for the code review so far. More to come

danpetry · 2019-02-11T13:42:37Z

+
+
+/**
+ * @brief Prototype of a callback function for the load action of a store


for clarity: s/store/storage facility

danpetry · 2019-02-11T13:43:55Z

+typedef void (*load_cb_t)(char *name, char *val, void *cb_arg);
+
+/**
+ * @brief Descriptor used to check duplications in store facilities


s/store facility/storage facility, here and below, i.e. keep the naming consistent across all documentation

danpetry · 2019-02-11T13:57:43Z

+ */
+typedef struct {
+    clist_node_t node;                    /**< linked list node */
+    const struct registry_store_itf *itf; /**< interface for the facility */


Could you explain, why didn't you put the linked list node inside the interface struct like with the registry handlers? (also why is a *name not required?)

danpetry · 2019-02-11T14:38:53Z

+     * @param[in] cb_arg Argument passed to @p cb function
+     * @return 0 on success, non-zero on failure
+     */
+    int (*load)(registry_store_t *store, load_cb_t cb, void *cb_arg);


Just wondering, why is there a registry_store_t* as an argument? Isn't all the code in the functions specific to a certain storage facility anyway, so you wouldn't have to pass a storage facility through the interface? Is this to add another potential abstraction layer, e.g. in the case of file storage where the underlying physical storage might be different but the "storage facility", i.e. the interface is the same?

danpetry · 2019-02-19T15:30:05Z

+void registry_register(registry_handler_t *handler);
+
+/**
+ * @brief Registers a new storage as a source of configurations. Multiple


s/storage/storage facility would be clearer I think (same below)
(same idea about standardizing the language so "storage facility" is always used for a storage facility)

danpetry · 2019-02-19T15:33:56Z

+                        int argc, char **argv, void *context);
+
+    void *context; /**< Optional context used by the handlers */
+} registry_handler_t;


After reading the code a bit I found that the name "registry handler" wasn't that intuitive. "Storage facility" clearly indicates what it means, but since "registry" is the whole thing being implemented, "registry handler" is a bit too generic. Something like just "config group" would be clearer imo. What do you think?

I think this should be discussed in the corresponding RDM: #10622

moved to there

I've unresolved tho if that's ok just to remind us that it needs to be decided before merging this implementation

danpetry · 2019-02-19T15:54:52Z

+ * @brief Storage facility interface.
+ * All store facilities should, at least, implement the load and save actions.
+ */
+typedef struct registry_store_itf {


In the doxygen, it shows two typedefs (this and typedef void(* | load_cb_t) (char *name, char *val, void *cb_arg)) when there are actually a bunch of typedefs.the rest are in the data structures section. do you know why this is and whether it's necessary/possible to categorise them a bit more cleanly?

danpetry · 2019-02-19T15:56:51Z

+                                          0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA,
+                                          0xAA, 0xAA, 0xAA, 0xAA};
+
+char *get_handler(int argc, char **argv, char *val, int val_len_max, void *ctx)


could move the handlers to another file called handlers.c so that people can use it as a reference to implement their own handler?

danpetry

Here are some more. Let me know if posting the review bits at a time is a problem

danpetry · 2019-02-20T14:12:06Z

+
+/**
+ * @brief Convenience function to parse a configuration parameter value of
+ * `bytes` type from a string.


Please point out that the encoding is base64, here and in the str_from_bytes function

danpetry · 2019-02-20T14:19:22Z

+     *
+     * @param[in] argc Number of elements in @p argv
+     * @param[in] argv Parsed string representing the configuration parameter.
+     * @param[out] val Buffer containing the string of the new value


wouldn't the set function need to know the size of the output buffer though, so it doesn't overflow?

danpetry · 2019-02-20T14:20:09Z

+                        int argc, char **argv, void *context);
+
+    void *context; /**< Optional context used by the handlers */
+} registry_handler_t;


moved to there

danpetry · 2019-02-20T14:20:59Z

+                        int argc, char **argv, void *context);
+
+    void *context; /**< Optional context used by the handlers */
+} registry_handler_t;


I've unresolved tho if that's ok just to remind us that it needs to be decided before merging this implementation

danpetry · 2019-02-20T16:21:43Z

+    (void)ctx;
+
+    if (argc) {
+        if (!strcmp("opt1", argv[0])) {


What about an implementation of these config params which is more extensible? i.e., so you don't have to manually add each new param in all handlers. I'm thinking an array of structs, where the structs contain pointers to the encoding/decoding functions, the name, the value, the REGISTRY_TYPE, and anything else which might be needed. Then you could just loop through the array in each of the handlers. maybe with a switch-case statement inside the loop if required
Seems like this test is something like the reference implementation - people will likely go here to see how to implement their own handler

danpetry · 2019-02-22T17:38:09Z

+    registry_handler_t *hndlr = container_of(current, registry_handler_t, node);
+    if (hndlr->hndlr_commit) {
+        _res = hndlr->hndlr_commit(hndlr->context);
+        if (!*(int *)res) {


So does this mean that registry_commit stops if a commit handler returns a non-zero value and returns that error value? Could this be documented in the API documentation?

danpetry · 2019-02-22T17:45:19Z

+            return hndlr->hndlr_commit(hndlr->context);
+        }
+        else {
+            return 0;


If there's no commit handler defined, I think it should return something more informative than "success", IMO

danpetry · 2019-02-22T17:49:50Z

+        }
+        if (hndlr->hndlr_export) {
+            return hndlr->hndlr_export(export_func, name_argc - 1,
+                                       &name_argv[1], hndlr->context);


IMO name_argv + 1 is more readable but I guess this is a matter of style

danpetry · 2019-02-22T17:51:23Z

+                                       &name_argv[1], hndlr->context);
+        }
+        else {
+            return 0;


Same as above, would be good to return something other than "success" if no handler present

danpetry · 2019-02-22T18:05:57Z

+    }
+    else {
+        DEBUG("[registry export] exporting all\n");
+        clist_node_t *node = registry_handlers.next;


I guess you do this because the registry_handlers is a node which doesn't represent any registry handler at all?
If that's the case, the do loop below will call container_of with the registry_handlers node at some point I think, is that right? Would that lead to an error?

danpetry

Some more comments, on registry_store.c specifically

danpetry · 2019-03-04T10:43:26Z

+
+void registry_store_init(void)
+{
+    load_srcs.next = NULL;


save_dst = NULL as well?

danpetry · 2019-03-04T10:44:47Z

+        return -1;
+    }
+
+    if (save_dst->itf->save_start) {


just a check on save_dst != NULL? this would catch both if checks in this function. Also, if save_dst doesn't exist, it'll already have tried to dereference the pointer just to perform this check afaiu

danpetry · 2019-03-04T10:45:29Z

+    int res = 0;
+    int res2;
+
+    if (!node) {


With the null pointer checks, it would be nicer IMO to use a consistent syntax between if checks and assert checks, for code beautification reasons, but this is fairly marginal personal style reasons so nonblocking

danpetry · 2019-03-04T11:05:41Z

+        return -ENOENT;
+    }
+
+    dup.name = (char *)name;


It's my understanding that casting away constness is evil and should be avoided, because you've told the users of your API that an argument is const but in fact you can now modify it internally, which could lead to undefined behaviour from the point of view of the user.
Suggest making *name and *val in registry_dup_check_arg_t both const. Along with both the arguments to this function. If they won't need to be modified in any conceivable use cases that is

danpetry · 2019-03-04T11:20:39Z

+    return 0;
+}
+
+static void _registry_dup_check_cb(char *name, char *val, void *cb_arg)


the logic below AFAIU is as follows:

Names not identical => is_dup = 0

Names identical and neither val or dup_arg->val present => is_dup = 1

Names identical and both val and dup_arg->val present and identical => is_dup = 1

Names identical and only one or other of val and dup_arg->val present => is_dup = 0

So, you can compare either just the name, or the name and the value, according to your choosing
This seems to give too much flexibility for an internal function; you're the only one who's using it. So you could just keep the check on both val and name, to save code size. And reduce some of the other checks too as far as possible and just make sure in the calling function that the arguments you're passing aren't causing any errors.

( I just remembered that elsewhere I suggested removing the duplication check. The above comments would apply only if removing the duplication check and silently copying over is considered a bad idea :) )

danpetry · 2019-03-04T11:26:51Z

+
+int registry_save_one(const char *name, char *value)
+{
+    registry_store_t *dst = save_dst;


As far as I can tell you don't need this, you can just access save_dst

danpetry · 2019-03-04T11:36:43Z

+                res = res2;
+            }
+        }
+    } while (node != registry_handlers.next);


So this will return the error code of the first failing hndlr_export function but continue exporting from the other handlers. This should be in the API documentation I would say

jcarrano · 2019-05-27T15:08:08Z

@leandrolanzieri Can we split the registry itself into its own PR. This way we can comment on the registry only (and it will be more reviewable) and also we can dump the conversations we had in person.

leandrolanzieri · 2019-05-27T19:09:56Z

@leandrolanzieri Can we split the registry itself into its own PR. This way we can comment on the registry only (and it will be more reviewable) and also we can dump the conversations we had in person.

Yes, I was planning on opening a new PR, only with the registry, with the changes we discussed on the configuration values and keys. I will keep it to the minimum so it's easier to review and discuss.

stale · 2020-02-11T09:15:15Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

leandrolanzieri requested a review from jia200x January 17, 2019 11:16

jia200x changed the title ~~sys: Add RIOT Registry implementation~~ [WIP] sys: Add RIOT Registry implementation Jan 17, 2019

jia200x requested changes Jan 17, 2019

View reviewed changes

danpetry mentioned this pull request Jan 24, 2019

boards/sensebox_samd21: Add MTD definition #10802

Merged

danpetry removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jan 24, 2019

leandrolanzieri force-pushed the riot_registry branch from 1d03ba7 to 5ed8d58 Compare January 25, 2019 09:22

danpetry suggested changes Jan 28, 2019

View reviewed changes

leandrolanzieri force-pushed the riot_registry branch 3 times, most recently from 4189131 to ce6f602 Compare January 31, 2019 09:34

sys: Add RIOT Registry

ea60cb1

leandrolanzieri force-pushed the riot_registry branch from ce6f602 to ea60cb1 Compare January 31, 2019 10:19

This was referenced Jan 31, 2019

sys/registry: Add EEPROM storage #10910

Closed

sys/registry: Add File storage #10911

Closed

leandrolanzieri changed the title ~~[WIP] sys: Add RIOT Registry implementation~~ sys: Add RIOT Registry implementation Jan 31, 2019

leandrolanzieri removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Jan 31, 2019

leandrolanzieri requested a review from kaspar030 January 31, 2019 11:29

danpetry added the Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines label Feb 4, 2019

danpetry suggested changes Feb 20, 2019

View reviewed changes

danpetry suggested changes Feb 22, 2019

View reviewed changes

danpetry suggested changes Mar 4, 2019

View reviewed changes

stale Bot added the State: stale State: The issue / PR has no activity for >185 days label Feb 11, 2020

stale Bot closed this Mar 13, 2020



		/**
		* @brief Prototype of a callback function for the load action of a store

Conversation

leandrolanzieri commented Jan 17, 2019 • edited by miri64 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danpetry left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri commented Jan 31, 2019

Uh oh!

danpetry commented Feb 4, 2019

Uh oh!

danpetry commented Feb 4, 2019

Uh oh!

leandrolanzieri commented Feb 20, 2019

Uh oh!

danpetry left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danpetry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danpetry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leandrolanzieri commented Jan 17, 2019 •

edited by miri64

Loading

danpetry left a comment •

edited

Loading