[riot-notifications] [RIOT-OS/RIOT] sys: Add RIOT Registry implementation (#10799)

danpetry notifications at github.com
Fri Feb 22 19:12:05 CET 2019


danpetry requested changes on this pull request.

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

> + * To parse the string to bytes @ref registry_bytes_from_str() function must be
+ * used.
+ *
+ * @param[in] val_str Pointer of the string containing the value
+ * @param[in] type Type of the parameter to be parsed
+ * @param[out] vp Pointer to store the parsed value
+ * @param[in] maxlen Maximum length of the output buffer when the type of the
+ * parameter is string.
+ * @return 0 on success, non-zero on failure
+ */
+int registry_value_from_str(char *val_str, registry_type_t type, void *vp,
+                            int maxlen);
+
+/**
+ * @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

> +     * failure
+     *
+     * @note The strings passed in @p argv do not contain the string of the name
+     * of the configuration group. E.g. If a parameter name is 'group/foo/var'
+     * and the name of the group is 'group', argv will contain 'foo' and 'var'.
+     */
+    char *(*hndlr_get)(int argc, char **argv, char *val, int val_len_max,
+                       void *context);
+
+    /**
+     * @brief Handler to set a the value of a configuration parameter.
+     * The value will be passed as a string in @p val.
+     *
+     * @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?

> +     * @param[in] argc Number of elements in @p argv
+     * @param[in] argv Parsed string representing the configuration parameter.
+     * @param[in] context Context of the handler
+     * @return 0 on success, non-zero on failure (if @p export_func fails,
+     *         i.e. returns non-zero, this error code should be returned by the
+     *         handler as it is considered a failure).
+     *
+     * @note The strings passed in @p argv do not contain the string of the name
+     * of the configuration group. E.g. If a parameter name is 'group/foo/var'
+     * and the name of the group is 'group', argv will contain 'foo' and 'var'.
+     */
+    int (*hndlr_export)(int (*export_func)(const char *name, char *val),
+                        int argc, char **argv, void *context);
+
+    void *context; /**< Optional context used by the handlers */
+} registry_handler_t;

moved to there

> +     * @param[in] argc Number of elements in @p argv
+     * @param[in] argv Parsed string representing the configuration parameter.
+     * @param[in] context Context of the handler
+     * @return 0 on success, non-zero on failure (if @p export_func fails,
+     *         i.e. returns non-zero, this error code should be returned by the
+     *         handler as it is considered a failure).
+     *
+     * @note The strings passed in @p argv do not contain the string of the name
+     * of the configuration group. E.g. If a parameter name is 'group/foo/var'
+     * and the name of the group is 'group', argv will contain 'foo' and 'var'.
+     */
+    int (*hndlr_export)(int (*export_func)(const char *name, char *val),
+                        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

> +#define BYTES_LENGTH    16
+#endif
+
+/* These are the 'configuration parameters' */
+int64_t test_opt1 = 0;
+int8_t test_opt2 = 1;
+unsigned char test_bytes[BYTES_LENGTH] = {0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA,
+                                          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)
+{
+    (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

> +                                        val, val_len_max);
+        }
+        else if (!strcmp("bytes", argv[0])) {
+            return registry_str_from_bytes((void *)test_bytes, BYTES_LENGTH,
+                                           val, val_len_max);
+
+        }
+    }
+    return NULL;
+}
+
+int set_handler(int argc, char **argv, char *val, void *ctx)
+{
+    (void)ctx;
+    const char buf[BYTES_LENGTH];
+    int len = sizeof(test_bytes);

If `len` is an argument, we don't have to take the buffer length from global variables, we can have it through the interface. i.e. better encapsulation

> +#ifndef REGISTRY_REGISTRY_H
+#define REGISTRY_REGISTRY_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <stdint.h>
+#include "clist.h"
+
+/**
+ * @brief Separator character to define hierarchy in configurations names.
+ */
+#define REGISTRY_NAME_SEPARATOR    '/'
+
+/**

These default buffer sizes are quite generous. Also, the max length of a 64 bit value would be 19 chars I think. Could reduce them?

> @@ -0,0 +1,198 @@
+#include <stdio.h>
+#include <string.h>
+#include <errno.h>
+#include "clist.h"
+#include "registry/registry.h"
+#include "kernel_defines.h"
+#include "assert.h"
+#define ENABLE_DEBUG (0)
+#include "debug.h"
+
+static int _registry_cmp_name(clist_node_t *current, void *name);
+static int _registry_call_commit(clist_node_t *current, void *name);
+static void _parse_name(char *name, int *name_argc, char *name_argv[]);
+static registry_handler_t *_handler_parse_and_lookup(char *name, int *name_argc,
+                                                     char *name_argv[]);

Could it be consistent across the contribution tho in that case? This is the only place they appear

> +
+    if (node != NULL) {
+        hndlr = container_of(node, registry_handler_t, node);
+    }
+
+    return hndlr;
+}
+
+static void _parse_name(char *name, int *name_argc, char *name_argv[])
+{
+    int i = 0;
+    char *_name_p = &name[0];
+
+    while (_name_p) {
+        name_argv[i++] = _name_p;
+        while(1) {

Should be `while (1)` according to our code beautification standards. There are a few more scattered about, could you run uncrustify on all the files?

> +    }
+
+    return hndlr->hndlr_set(name_argc - 1, &name_argv[1], val_str,
+                            hndlr->context);
+}
+
+char *registry_get_value(char *name, char *buf, int buf_len)
+{
+    int name_argc;
+    char *name_argv[REGISTRY_MAX_DIR_DEPTH];
+    registry_handler_t *hndlr;
+
+    hndlr = _handler_parse_and_lookup(name, &name_argc, name_argv);
+
+    if (!hndlr) {
+        return NULL;

This seems inconsistent from the `set` function above, which returns `-EINVAL` if the handler doesn't exist

> +    if (!hndlr->hndlr_get) {
+        return NULL;
+    }
+
+    return hndlr->hndlr_get(name_argc - 1, &name_argv[1], buf, buf_len,
+                            hndlr->context);
+}
+
+static int _registry_call_commit(clist_node_t *current, void *res)
+{
+    assert(current != NULL);
+    int _res = *(int *)res;
+    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?

> +    }
+
+    return hndlr->hndlr_set(name_argc - 1, &name_argv[1], val_str,
+                            hndlr->context);
+}
+
+char *registry_get_value(char *name, char *buf, int buf_len)
+{
+    int name_argc;
+    char *name_argv[REGISTRY_MAX_DIR_DEPTH];
+    registry_handler_t *hndlr;
+
+    hndlr = _handler_parse_and_lookup(name, &name_argc, name_argv);
+
+    if (!hndlr) {
+        return NULL;

Also the functions below

> +    int name_argc;
+    int rc = 0;
+
+    if (name) {
+        registry_handler_t *hndlr;
+        char *name_argv[REGISTRY_MAX_DIR_DEPTH];
+
+        hndlr = _handler_parse_and_lookup(name, &name_argc, name_argv);
+        if (!hndlr) {
+            return -EINVAL;
+        }
+        if (hndlr->hndlr_commit) {
+            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

> +int registry_export(int (*export_func)(const char *name, char *val), char *name)
+{
+    assert(export_func != NULL);
+    int name_argc;
+    char *name_argv[REGISTRY_MAX_DIR_DEPTH];
+    registry_handler_t *hndlr;
+
+    if (name) {
+        DEBUG("[registry export] exporting %s\n", name);
+        hndlr = _handler_parse_and_lookup(name, &name_argc, name_argv);
+        if (!hndlr) {
+            return -EINVAL;
+        }
+        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

> +    int name_argc;
+    char *name_argv[REGISTRY_MAX_DIR_DEPTH];
+    registry_handler_t *hndlr;
+
+    if (name) {
+        DEBUG("[registry export] exporting %s\n", name);
+        hndlr = _handler_parse_and_lookup(name, &name_argc, name_argv);
+        if (!hndlr) {
+            return -EINVAL;
+        }
+        if (hndlr->hndlr_export) {
+            return hndlr->hndlr_export(export_func, name_argc - 1,
+                                       &name_argv[1], hndlr->context);
+        }
+        else {
+            return 0;

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

> +        DEBUG("[registry export] exporting %s\n", name);
+        hndlr = _handler_parse_and_lookup(name, &name_argc, name_argv);
+        if (!hndlr) {
+            return -EINVAL;
+        }
+        if (hndlr->hndlr_export) {
+            return hndlr->hndlr_export(export_func, name_argc - 1,
+                                       &name_argv[1], hndlr->context);
+        }
+        else {
+            return 0;
+        }
+    }
+    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?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/10799#pullrequestreview-205777887
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190222/a20915d4/attachment-0001.html>


More information about the notifications mailing list