[riot-notifications] [RIOT-OS/RIOT] syslog: add a simple syslog module (#15739)

Marian Buschsieweke notifications at github.com
Tue Jan 12 12:10:33 CET 2021


@maribu commented on this pull request.

Some comments for the API. I didn't look at the implementation yet.

> +
+config SYSLOG_BACKEND_ROT_SIZE
+    int "Rotation size"
+    default 262144
+    help
+        Max size of a file before a file rotation is triggered
+
+config SYSLOG_BACKEND_NB_FILES
+    int "Max number of log files"
+    default 32
+    help
+        Maximum number of log files that are kept
+
+endif # KCONFIG_SYSLOG_BACKEND_FILE
+
+endif # KCONFIG_SYSLOG

Missing `\n` to terminate the last line

> @@ -0,0 +1,7 @@
+# syslog files
+SRC := syslog.c backend.c
+
+# enable submodules
+SUBMODULES := 1
+
+include $(RIOTBASE)/Makefile.base

Missing `\n` to terminate the last line

> +
+void syslog_backend_file_suspend_rotation(void)
+{
+    mutex_lock(&rotation_lock);
+    suspend_rotation++;
+    mutex_unlock(&rotation_lock);
+}
+
+void syslog_backend_file_resume_rotation(void)
+{
+    assert(suspend_rotation);
+
+    mutex_lock(&rotation_lock);
+    suspend_rotation--;
+    mutex_unlock(&rotation_lock);
+}

Missing `\n` to terminate the last line

> +
+int syslog_getmask(const syslog_entry_t *log)
+{
+    if (!log) {
+        return 0;
+    }
+    return log->mask;
+}
+
+const char *syslog_getident(const syslog_entry_t *log)
+{
+    if (!log) {
+        return NULL;
+    }
+    return log->ident;
+}

Missing `\n` to terminate the last line

> +
+int _syslog_handler(int argc, char **argv)
+{
+    if (argc < 2) {
+        _syslog_usage(argv);
+        return 1;
+    }
+    if (strcmp(argv[1], "list") == 0) {
+        return _list_handler(argc - 1, argv + 1);
+    }
+    else {
+        printf("%s: unsupported sub-command: %s\n", argv[0], argv[1]);
+        _syslog_usage(argv);
+        return 1;
+    }
+}

Missing `\n` to terminate the last line

> +struct syslog_msg {
+    /* metadata */
+    uint8_t pri;
+    struct tm time;
+    bool time_set;
+    const char *app_name;
+    kernel_pid_t proc_id;
+    /* msg */
+    char msg[CONFIG_SYSLOG_MAX_STRING_LEN];
+    size_t len;
+    /* internal */
+    atomic_int cnt;
+};

While a personally agree that doing `typedef`s on structs is bad style, for consistency with the rest of the code base I think this should be a typedef. Also, this should be documented.

> +#ifndef CONFIG_SYSLOG_MAX_STRING_LEN
+#define CONFIG_SYSLOG_MAX_STRING_LEN    256
+#endif

```suggestion
#if !defined(CONFIG_SYSLOG_MAX_STRING_LEN) || defined(DOXYGEN)
#define CONFIG_SYSLOG_MAX_STRING_LEN    256 /**< Maximum message length in bytes */
#endif
```

> +ifneq (,$(filter syslog, $(USEMODULE)))
+  FEATURES_REQUIRED += periph_rtc
+endif

IMO peripheral interfaces are low level interfaces that in most cases shouldn't be directly used. E.g. this would result in syslog requiring an RTC. This could limit its use significantly, e.g. boards without an RTC cannot be used. Some MCUs that do have an RTC can provide either an RTT or an RTC, as they share the same hardware. However, using the RTT can yield significant power savings when using `ztimer`.

IMO it would be better to put this on top of ztimer. But I agree that `ztimer` currently misses features (or more specifically a real time clock) and utility functions for to convert from `ztimer_now_t` to a broken up (year, month, day, hour, minute, second) timestamp.

@kaspar030: Maybe we can add a `ZTIMER_SEC_REALTIME` clock (and higher frequency friends?). IMO there should also be a way to atomically correct the realtime clock to allow for network synchronization. (This would btw. very naturally map to the corresponding PTP low level function, one we have a `ztimer_periph_ptp` backend. And by storing an internal offset to the underlying clock this would also be easily implemented in software.)

> +    if (IS_USED(MODULE_SYSLOG)) {
+        extern void syslog_init(void);
+        syslog_init();
+    }

Can't we initialize syslog backends here as well? That would safe a function pointer in the backend struct.

> +/**
+ * @brief   Enable/disable stdio output
+ *
+ * @param[in]   print   true to enable stdio output, false to disable
+ */
+void syslog_backend_stdio_print(bool print);
+
+/* File */
+
+/**
+ * @brief   Start file backend
+ *
+ * Start syslog file backend. This should be called when the filesystem is
+ * ready. It will initialize the file backend by looking into the log directory
+ * and counting the current number of files.
+ */
+int syslog_backend_file_start(void);
+
+/**
+ * @brief   Suspend log rotation
+ *
+ * If called multiple times, syslog_backend_file_resume_rotation() must be
+ * called the same number of times so the rotation is resumed.
+ */
+void syslog_backend_file_suspend_rotation(void);
+
+/**
+ * @brief   Resume log rotation
+ */
+void syslog_backend_file_resume_rotation(void);

Are there any use cases for these? IMO it is enough to just start logging once the backends are initialized. Being able to adjust the logging level at runtime already increases complexity (and resource requirements) quite a bit and likely already addresses all use cases.

> +    uint8_t pri;
+    struct tm time;
+    bool time_set;
+    const char *app_name;
+    kernel_pid_t proc_id;

```suggestion
    kernel_pid_t proc_id;
    uint8_t pri;
    bool time_set;
    struct tm time;
    const char *app_name;
```

Reordering the members safe quite a bit of RAM. `kernel_pid_t` as a 16 bit alignment requirements, `uint8_t` and `bool` have an 8 bit alignment requirement. Placing them in that order results in the subsequent member being on 32 bit boundary, so that a 32 bit alignment requirement can be fullilled without padding.

> +
+#include "thread.h"
+#include "time.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#ifndef CONFIG_SYSLOG_MAX_STRING_LEN
+#define CONFIG_SYSLOG_MAX_STRING_LEN    256
+#endif
+
+struct syslog_msg {
+    /* metadata */
+    uint8_t pri;
+    struct tm time;

As above, IMO it would be better to use ztimer. A `ztimer_now_t` is also only 8 bytes in size (with `ztimer_now64` used), while `struct tm` is in between 36 and 44 Bytes on ARM. The backend could use a helper function to convert this to year, month, day, hour, minute and seconds.

It might be reasonable to add a `tm_t` type that contains only the reasonable fields using reasonable data times. IMO we should also consider updating the `periph_rtc` API to use that, so that driver authors don't need to worry about the day of the week, daylight saving settings, etc. in addition to saving same bytes for RAM. Maybe such a `tm_t` is better than `ztimer_now_t` for this use case.

> +    /**
+     * @brief   syslog backend init
+     */
+    int (*init)(void);

Can't we get rid of this function pointer and just initialize the backend within `auto_init` one by one?

> @@ -0,0 +1,211 @@
+
+#include <fcntl.h>
+#include <errno.h>
+#include <string.h>
+
+#include "syslog.h"
+#include "backend.h"
+#include "vfs.h"
+#include "mutex.h"
+
+#define ENABLE_DEBUG    (1)

```suggestion
#define ENABLE_DEBUG    0
```

Please don't use braces here, as this prevents using it with the `IS_ACTIVE()` helper macro.

-- 
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/15739#pullrequestreview-566103308
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210112/e181836a/attachment-0001.htm>


More information about the notifications mailing list