use iniparser dependency for config file parsing
For the config file, we do not need the custom handwritten parser.
Thanks to Andrew for this command, we can now search for an alternative
$ git grep -lw INI -- :/:*.bb
meta-openembedded/meta-oe/recipes-support/inih/libinih_58.bb
meta-openembedded/meta-oe/recipes-support/iniparser/iniparser_4.1.bb
meta-openembedded/meta-oe/recipes-support/minini/minini_1.2.b.bb
poky/meta/recipes-devtools/python/python3-iniconfig_2.0.0.bb
poky/meta/recipes-devtools/python/python3-iniparse_0.5.bb
For the ini parser we have following requirements
- small API
- easy to use
- compiles fast
- has tests, examples, docs
- support for sections
- minini [1]
can be dropped from the list since it also supports colon
':' instead of '=' for separating key and value, creating 2 ways of
doing something. This makes it harder to swap out the ini parser in
the future.
- libinih [2]
uses SAX-style parsing of .ini files and thus does not provide
a DOM of the .ini. This is a break from the previous parser which
stored everything in struct config. To use this library would require
to create a struct to store all the possible configuration, then fill
that struct in one pass. Essentially wrapping that library to have
DOM capability. That would be possible, but not ideal. libinih is also
highly configurable with lots of config options.
- iniparser [3]
has all the required features and stores the results of its
parsing for later use. It is a seamless upgrade from the previous
parser. The call sites do not have to be modified and we can read the
config as before. A downside is that we have to provide our own wrap
file.
For our purposes, iniparser is a good choice.
Using this dependency allows us to drop the custom parser and all the
tests that go along with it.
If sections are required in future config files, iniparser can also
support that.
References:
[1] https://github.com/compuphase/minIni
[2] https://github.com/benhoyt/inih
[3] https://gitlab.com/iniparser/iniparser
Change-Id: Ie2b57171ec1f8cb6b1b80ca1d9e6c112bedc1195
Signed-off-by: Alexander Hansen <alexander.hansen@9elements.com>
diff --git a/.clang-tidy b/.clang-tidy
index af46506..dfdb4b4 100644
--- a/.clang-tidy
+++ b/.clang-tidy
@@ -207,7 +207,7 @@
'
WarningsAsErrors: '*'
-HeaderFilterRegex: '.*'
+HeaderFilterRegex: '^(test\/)?[a-zA-Z0-9]*\.h'
CheckOptions:
- { key: readability-identifier-naming.VariableCase, value: lower_case }
- { key: readability-identifier-naming.FunctionCase, value: lower_case }
diff --git a/.gitignore b/.gitignore
index cf026c7..bb926e0 100644
--- a/.gitignore
+++ b/.gitignore
@@ -45,7 +45,6 @@
# Repo specific items
*.o
-/config.h
/config.h.in~
/config.log
/config.status
diff --git a/config-internal.h b/config-internal.h
new file mode 100644
index 0000000..0bcb911
--- /dev/null
+++ b/config-internal.h
@@ -0,0 +1,25 @@
+/**
+ * Copyright © 2024 obmc-console authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <iniparser/dictionary.h>
+
+#define CONFIG_MAX_KEY_LENGTH 512
+
+struct config {
+ dictionary *dict;
+};
diff --git a/config.c b/config.c
index 5f5edaf..121425f 100644
--- a/config.c
+++ b/config.c
@@ -29,127 +29,37 @@
#include <sys/mman.h>
#include <sys/stat.h>
-#include "console-server.h"
+
+#include <iniparser/iniparser.h>
+
+#include "config.h"
+#include "config-internal.h"
+
+#define ARRAY_SIZE(a) (sizeof(a) / sizeof((a)[0]))
static const char *config_default_filename = SYSCONFDIR "/obmc-console.conf";
-struct config_item {
- char *name;
- char *value;
- struct config_item *next;
-};
-
-struct config {
- struct config_item *items;
-};
-
const char *config_get_value(struct config *config, const char *name)
{
- struct config_item *item;
+ int rc;
+ char buf[CONFIG_MAX_KEY_LENGTH];
+ rc = snprintf(buf, CONFIG_MAX_KEY_LENGTH, ":%s", name);
- if (!config) {
+ if (rc < 0) {
return NULL;
}
- for (item = config->items; item; item = item->next) {
- if (!strcasecmp(item->name, name)) {
- return item->value;
- }
+ if ((size_t)rc >= sizeof(buf)) {
+ return NULL;
}
- return NULL;
-}
+ const char *value = iniparser_getstring(config->dict, buf, NULL);
-static void config_parse(struct config *config, char *buf)
-{
- struct config_item *item;
- char *name;
- char *value;
- char *p;
- char *line;
-
- for (p = NULL, line = strtok_r(buf, "\n", &p); line;
- line = strtok_r(NULL, "\n", &p)) {
- char *end;
- int rc;
-
- /* trim leading space */
- while (isspace(*line)) {
- line++;
- }
-
- /* skip comments */
- if (*line == '#') {
- continue;
- }
-
- name = malloc(strlen(line));
- value = malloc(strlen(line));
- if (name && value) {
- rc = sscanf(line, "%[^ =] = %[^#]s", name, value);
- } else {
- rc = -ENOMEM;
- }
-
- if (rc != 2) {
- free(name);
- free(value);
- continue;
- }
-
- /* trim trailing space */
- end = value + strlen(value) - 1;
- while (isspace(*end)) {
- *end-- = '\0';
- }
-
- /* create a new item and add to our list */
- item = malloc(sizeof(*item));
- item->name = name;
- item->value = value;
- item->next = config->items;
- config->items = item;
+ if (value && strlen(value) == 0) {
+ return NULL;
}
-}
-static struct config *config_init_fd(int fd, const char *filename)
-{
- struct config *config;
- size_t size;
- size_t len;
- ssize_t rc;
- char *buf;
-
- size = 4096;
- len = 0;
- buf = malloc(size + 1);
- config = NULL;
-
- for (;;) {
- rc = read(fd, buf + len, size - len);
- if (rc < 0) {
- warn("Can't read from configuration file %s", filename);
- goto out_free;
-
- } else if (!rc) {
- break;
- }
- len += rc;
- if (len == size) {
- size <<= 1;
- buf = realloc(buf, size + 1);
- }
- }
- buf[len] = '\0';
-
- config = malloc(sizeof(*config));
- config->items = NULL;
-
- config_parse(config, buf);
-
-out_free:
- free(buf);
- return config;
+ return value;
}
struct config *config_init(const char *filename)
@@ -166,28 +76,30 @@
warn("Can't open configuration file %s", filename);
return NULL;
}
-
- config = config_init_fd(fd, filename);
-
close(fd);
+ dictionary *dict = iniparser_load(filename);
+
+ if (dict == NULL) {
+ return NULL;
+ }
+
+ config = malloc(sizeof(*config));
+ if (config) {
+ config->dict = dict;
+ }
+
return config;
}
void config_fini(struct config *config)
{
- struct config_item *item;
- struct config_item *next;
-
if (!config) {
return;
}
- for (item = config->items; item; item = next) {
- next = item->next;
- free(item->name);
- free(item->value);
- free(item);
+ if (config->dict) {
+ iniparser_freedict(config->dict);
}
free(config);
@@ -357,22 +269,3 @@
return DEFAULT_CONSOLE_ID;
}
-
-#ifdef CONFIG_TEST
-int main(void)
-{
- struct config_item *item;
- struct config *config;
-
- config = config_init_fd(STDIN_FILENO, "<stdin>");
- if (!config)
- return EXIT_FAILURE;
-
- for (item = config->items; item; item = item->next)
- printf("%s: %s\n", item->name, item->value);
-
- config_fini(config);
-
- return EXIT_SUCCESS;
-}
-#endif
diff --git a/config.h b/config.h
new file mode 100644
index 0000000..e8f6f8d
--- /dev/null
+++ b/config.h
@@ -0,0 +1,34 @@
+/**
+ * Copyright © 2016 IBM
+ * Copyright © 2024 9elements
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+#include <stdint.h>
+#include <termios.h>
+
+struct config;
+
+const char *config_get_value(struct config *config, const char *name);
+struct config *config_init(const char *filename);
+const char *config_resolve_console_id(struct config *config,
+ const char *id_arg);
+void config_fini(struct config *config);
+
+int config_parse_baud(speed_t *speed, const char *baud_string);
+uint32_t parse_baud_to_int(speed_t speed);
+speed_t parse_int_to_baud(uint32_t baud);
+int config_parse_bytesize(const char *size_str, size_t *size);
diff --git a/console-client.c b/console-client.c
index 3dc3190..67d163a 100644
--- a/console-client.c
+++ b/console-client.c
@@ -29,6 +29,7 @@
#include <sys/un.h>
#include "console-server.h"
+#include "config.h"
#define EXIT_ESCAPE 2
diff --git a/console-dbus.c b/console-dbus.c
index 71dd0de..d9b8689 100644
--- a/console-dbus.c
+++ b/console-dbus.c
@@ -21,6 +21,7 @@
#include <sys/socket.h>
#include "console-server.h"
+#include "config.h"
/* size of the dbus object path length */
const size_t dbus_obj_path_len = 1024;
diff --git a/console-server.c b/console-server.c
index 596e227..7bddc4c 100644
--- a/console-server.c
+++ b/console-server.c
@@ -39,6 +39,7 @@
#include <poll.h>
#include "console-server.h"
+#include "config.h"
#define DEV_PTS_PATH "/dev/pts"
diff --git a/console-server.h b/console-server.h
index 3dc445e..cc92101 100644
--- a/console-server.h
+++ b/console-server.h
@@ -217,19 +217,6 @@
/* Console server API */
void tty_init_termios(struct console *console);
-/* config API */
-struct config;
-const char *config_get_value(struct config *config, const char *name);
-struct config *config_init(const char *filename);
-const char *config_resolve_console_id(struct config *config,
- const char *id_arg);
-void config_fini(struct config *config);
-
-int config_parse_baud(speed_t *speed, const char *baud_string);
-uint32_t parse_baud_to_int(speed_t speed);
-speed_t parse_int_to_baud(uint32_t baud);
-int config_parse_bytesize(const char *size_str, size_t *size);
-
/* socket paths */
ssize_t console_socket_path(socket_path_t path, const char *id);
ssize_t console_socket_path_readable(const struct sockaddr_un *addr,
diff --git a/log-handler.c b/log-handler.c
index b77dd82..732fe2e 100644
--- a/log-handler.c
+++ b/log-handler.c
@@ -28,6 +28,7 @@
#include <linux/types.h>
#include "console-server.h"
+#include "config.h"
struct log_handler {
struct handler handler;
diff --git a/meson.build b/meson.build
index 06a7200..8ebf8af 100644
--- a/meson.build
+++ b/meson.build
@@ -52,6 +52,8 @@
install_dir: udev.get_variable('udevdir') / 'rules.d')
endif
+iniparser_dep = dependency('iniparser')
+
executable('obmc-console-server',
'config.c',
'console-dbus.c',
@@ -68,6 +70,7 @@
],
dependencies: [
dependency('libsystemd'),
+ iniparser_dep,
meson.get_compiler('c').find_library('rt')
],
install_dir: get_option('sbindir'),
@@ -81,6 +84,9 @@
c_args: [
'-DSYSCONFDIR="@0@"'.format(get_option('sysconfdir'))
],
+ dependencies: [
+ iniparser_dep,
+ ],
install: true)
subdir('test')
diff --git a/subprojects/.clang-tidy b/subprojects/.clang-tidy
new file mode 100644
index 0000000..612bd0e
--- /dev/null
+++ b/subprojects/.clang-tidy
@@ -0,0 +1 @@
+Checks: '-*'
diff --git a/subprojects/iniparser.wrap b/subprojects/iniparser.wrap
new file mode 100644
index 0000000..b104e76
--- /dev/null
+++ b/subprojects/iniparser.wrap
@@ -0,0 +1,10 @@
+[wrap-file]
+directory = iniparser-4.2.1
+source_url = https://github.com/ndevilla/iniparser/archive/refs/tags/v4.2.1.zip
+source_filename = iniparser-4.2.1.zip
+source_hash = 3a77d70e0489a3ef5a00c559134190740f20fd828ba3ff596d408c5438c8a896
+
+patch_directory = iniparser
+
+[provide]
+iniparser = iniparser_dep
diff --git a/subprojects/packagefiles/iniparser/meson.build b/subprojects/packagefiles/iniparser/meson.build
new file mode 100644
index 0000000..c1d89a7
--- /dev/null
+++ b/subprojects/packagefiles/iniparser/meson.build
@@ -0,0 +1,19 @@
+project('iniparser', 'c')
+
+# We create a directory for the include files to be able to include
+# iniparser/iniparser.h and not just iniparser.h
+# Because in case of iniparser being already installed, it is iniparser/iniparser.h
+
+inc = 'iniparser-include'
+
+run_command('sh', '-c', 'mkdir -p iniparser-include/iniparser')
+run_command('sh', '-c', 'cp -r src/* iniparser-include/iniparser/')
+
+iniparser = shared_library('iniparser',
+ 'src/dictionary.c',
+ 'src/iniparser.c',
+ include_directories : inc,
+ install : true)
+
+iniparser_dep = declare_dependency(include_directories : inc,
+ link_with : iniparser)
diff --git a/test/meson.build b/test/meson.build
index 39c9784..a204b12 100644
--- a/test/meson.build
+++ b/test/meson.build
@@ -1,8 +1,4 @@
tests = [
- 'test-client-escape',
- 'test-config-parse',
- 'test-config-parse-bytesize',
- 'test-config-resolve-console-id',
'test-ringbuffer-boundary-poll',
'test-ringbuffer-boundary-read',
'test-ringbuffer-contained-offset-read',
@@ -16,3 +12,23 @@
test(t, executable(t, f'@t@.c', c_args: [ '-DSYSCONFDIR=""' ],
include_directories: '..'))
endforeach
+
+tests_depend_iniparser = [
+ 'test-client-escape',
+ 'test-config-parse',
+ 'test-config-parse-bytesize',
+ 'test-config-resolve-console-id'
+]
+
+foreach ct : tests_depend_iniparser
+ test(
+ ct,
+ executable(
+ ct,
+ f'@ct@.c',
+ c_args: [ '-DSYSCONFDIR=""' ],
+ dependencies: [ iniparser_dep ],
+ include_directories: '..'
+ )
+ )
+endforeach
diff --git a/test/test-config-parse.c b/test/test-config-parse.c
index df90e91..8409f24 100644
--- a/test/test-config-parse.c
+++ b/test/test-config-parse.c
@@ -11,25 +11,66 @@
#include "config.c"
+static struct config *mock_config_from_buffer(const char *input)
+{
+ struct config *ctx;
+ ssize_t rc;
+
+ int fd = memfd_create("test-parse-ini", 0);
+ assert(fd != -1);
+
+ const size_t len = strlen(input);
+ rc = write(fd, input, len);
+
+ assert(rc >= 0);
+ assert((size_t)rc == len);
+
+ rc = lseek(fd, 0, SEEK_SET);
+ assert(rc == 0);
+
+ FILE *f = fdopen(fd, "r");
+ assert(f != NULL);
+
+ dictionary *dict = iniparser_load_file(f, "");
+
+ fclose(f);
+
+ if (dict == NULL) {
+ return NULL;
+ }
+
+ ctx = calloc(1, sizeof(*ctx));
+
+ if (ctx) {
+ ctx->dict = dict;
+ }
+
+ return ctx;
+}
+
static void execute_test(const char *input, const char *key,
const char *expected)
{
- struct config *ctx;
+ struct config *ctx = mock_config_from_buffer(input);
const char *found;
- char *buf;
- ctx = calloc(1, sizeof(*ctx));
- buf = strdup(input);
- config_parse(ctx, buf);
- free(buf);
- found = config_get_value(ctx, key);
if (!expected) {
+ if (ctx == NULL) {
+ return;
+ }
+
+ found = config_get_value(ctx, key);
assert(!found);
+
+ goto cleanup;
}
- if (expected) {
- assert(found);
- assert(!strcmp(expected, found));
- }
+
+ assert(ctx->dict != NULL);
+ found = config_get_value(ctx, key);
+
+ assert(found);
+ assert(!strcmp(expected, found));
+cleanup:
config_fini(ctx);
}
diff --git a/test/test-config-resolve-console-id.c b/test/test-config-resolve-console-id.c
index 84bc5b3..d84ebd1 100644
--- a/test/test-config-resolve-console-id.c
+++ b/test/test-config-resolve-console-id.c
@@ -4,6 +4,26 @@
#include "config.c"
+static struct config *config_mock(char *key, char *value)
+{
+ char buf[CONFIG_MAX_KEY_LENGTH];
+ struct config *config;
+ int rc;
+
+ config = malloc(sizeof(struct config));
+ assert(config != NULL);
+
+ config->dict = dictionary_new(1);
+ assert(config->dict != NULL);
+
+ rc = snprintf(buf, CONFIG_MAX_KEY_LENGTH, ":%s", key);
+ assert(rc >= 0 && (size_t)rc < sizeof(buf));
+
+ dictionary_set(config->dict, buf, value);
+
+ return config;
+}
+
static void test_independence_cmdline_optarg(void)
{
const char *console_id;
@@ -21,12 +41,8 @@
{
const char *console_id;
struct config *ctx;
- char *buf;
- ctx = calloc(1, sizeof(*ctx));
- buf = strdup("console-id = " TEST_CONSOLE_ID);
- config_parse(ctx, buf);
- free(buf);
+ ctx = config_mock("console-id", TEST_CONSOLE_ID);
console_id = config_resolve_console_id(ctx, NULL);
assert(!strcmp(console_id, TEST_CONSOLE_ID));
@@ -38,12 +54,8 @@
{
const char *console_id;
struct config *ctx;
- char *buf;
- ctx = calloc(1, sizeof(*ctx));
- buf = strdup("socket-id = " TEST_CONSOLE_ID);
- config_parse(ctx, buf);
- free(buf);
+ ctx = config_mock("socket-id", TEST_CONSOLE_ID);
console_id = config_resolve_console_id(ctx, NULL);
/*
@@ -70,15 +82,10 @@
static void test_precedence_cmdline_optarg(void)
{
- static const char *const config = "console-id = console\n";
const char *console_id;
struct config *ctx;
- char *buf;
- ctx = calloc(1, sizeof(*ctx));
- buf = strdup(config);
- config_parse(ctx, buf);
- free(buf);
+ ctx = config_mock("console-id", "console");
console_id = config_resolve_console_id(ctx, TEST_CONSOLE_ID);
assert(config_get_value(ctx, "console-id"));
@@ -89,15 +96,10 @@
static void test_precedence_config_console_id(void)
{
- static const char *const config = "console-id = console\n";
const char *console_id;
struct config *ctx;
- char *buf;
- ctx = calloc(1, sizeof(*ctx));
- buf = strdup(config);
- config_parse(ctx, buf);
- free(buf);
+ ctx = config_mock("console-id", "console");
console_id = config_resolve_console_id(ctx, NULL);
assert(config_get_value(ctx, "console-id"));
diff --git a/tty-handler.c b/tty-handler.c
index 734f81e..a722d24 100644
--- a/tty-handler.c
+++ b/tty-handler.c
@@ -25,6 +25,7 @@
#include <termios.h>
#include "console-server.h"
+#include "config.h"
struct tty_handler {
struct handler handler;