Big clean-up PR!

.clang-tidy

@@ -4,30 +4,67 @@
 # check-by-check basis.
 #
 # Disabled checks:
+# - bugprone-assignment-in-if-condition
+#   Very carefully used, but useful when reading a bf_marsh.
+# - bugprone-easily-swappable-parameters
+#   Too many false positives, especially when swapable arguments are of different
+#   types (which will be flagged by the compiler).
+# - cert-dcl37-c
+#   Handled by bugprone-reserved-identifier.AllowedIdentifiers.
+# - cert-dcl51-cpp
+#   Handled by bugprone-reserved-identifier.AllowedIdentifiers.
 # - clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling
 #   Avoid usage of Annex K functions for portability reasons.
+# - clang-analyzer-unix.Malloc
+#   Generates false positives.
+# - modernize-macro-to-enum
+#   No benefit.
+# - readability-function-cognitive-complexity
+#   Functions generating BPF bytecode will trigger this rule anytime, but they're
+#   not that complex due to heavy use of macros.
+# - readability-isolate-declaration
+#   Rely on manual check: it's uncommon in bpfilter for multiple variable to be
+#   defined on a single line, but it's sometimes for the better.
 Checks: >
   -*,
   bugprone-*,
+    -bugprone-assignment-in-if-condition,
+    -bugprone-easily-swappable-parameters,
   cert-*,
+    -cert-dcl37-c,
+    -cert-dcl51-cpp,
   clang-analyzer-*,
     -clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling,
-  concurrency-*,
+    -clang-analyzer-unix.Malloc,
   misc-*,
   modernize-*,
+    -modernize-macro-to-enum,
   performance-*,
   portability-*,
-  readability-*
+  readability-*,
+    -readability-function-cognitive-complexity,
+    -readability-isolate-declaration
 
 WarningsAsErrors: ''
-HeaderFilterRegex: '^(?!external\/filter.h).*'
 FormatStyle: none
 UseColor: yes
 
 CheckOptions:
-  # Allowed short variable names
-  - key: readability-identifier-length.IgnoredVariableNames
-    value: 'fd|r'
+  # Allow use of reserved identifiers as long as they start with "_bf_"
+  - key: bugprone-reserved-identifier.AllowedIdentifiers
+    value: _(bf|BF)_[a-zA-Z0-9_]+
   # Unless a *statement* takes 1 line, it should be in braces
   - key: readability-braces-around-statements.ShortStatementLines
-    value: 2
+    value: 6
+  # Allowed short variable names
+  - key: readability-identifier-length.IgnoredVariableNames
+    value: '_|i|fd|r|j[0-9]|op'
+  # Allowed short parameter names
+  - key: readability-identifier-length.IgnoredParameterNames
+    value: 'ip|fd|op'
+  # Allow for magic constants that are power of 2.
+  - key: readability-magic-numbers.IgnorePowersOf2IntegerValues
+    value: true
+  # Allow specific masks
+  - key: readability-magic-numbers.IgnoredIntegerValues
+    value: 255;65535

.github/workflows/ci.yaml

@@ -37,6 +37,7 @@ jobs:
             libcmocka-devel \
             doxygen \
             git \
+            jq \
             lcov \
             libasan \
             libbpf-devel \
@@ -62,7 +63,7 @@ jobs:
         env:
           CODECOV_TOKEN: ${{ secrets.CODECOV_TOKEN }}
       - name: Check style
-        run: make -C $GITHUB_WORKSPACE/build checkstyle
+        run: make -C $GITHUB_WORKSPACE/build check
       - name: Generate documentation
         run: make -C $GITHUB_WORKSPACE/build doc
 
@@ -90,6 +91,7 @@ jobs:
             doxygen \
             flex \
             git \
+            jq \
             lcov \
             libasan \
             libbpf-devel \
@@ -113,8 +115,9 @@ jobs:
             cmake \
             doxygen \
             flex \
-            git \
             furo \
+            git \
+            jq \
             lcov \
             libbpf-dev \
             libcmocka-dev \

.github/workflows/fork.yaml

@@ -35,6 +35,7 @@ jobs:
             doxygen \
             flex \
             git \
+            jq \
             lcov \
             libasan \
             libbpf-devel \
@@ -53,6 +54,6 @@ jobs:
       # Skip unit tests, the GitHub-hosted runners' kernel is too old.
       # Skip coverage as unit tests are not run.
       - name: Check style
-        run: make -C $GITHUB_WORKSPACE/build checkstyle
+        run: make -C $GITHUB_WORKSPACE/build check
       - name: Generate documentation
         run: make -C $GITHUB_WORKSPACE/build doc

.github/workflows/pages.yaml

@@ -38,6 +38,7 @@ jobs:
             doxygen \
             flex \
             git \
+            jq \
             lcov \
             libasan \
             libbpf-devel \

CMakeLists.txt

@@ -37,6 +37,7 @@ find_program(CLANG_TIDY_BIN clang-tidy REQUIRED)
 find_program(CLANG_FORMAT_BIN clang-format REQUIRED)
 find_program(CLANG_BIN clang REQUIRED)
 find_program(BPFTOOL_BIN bpftool REQUIRED)
+find_program(JQ_BIN jq REQUIRED)
 
 set(CMAKE_EXPORT_COMPILE_COMMANDS ON)
 

README.md

@@ -30,6 +30,7 @@ Detailed information can be found in the [documentation](https://bpfilter.io).
         clang-tools-extra \
         cmake \
         flex \
+        jq \
         libcmocka-devel \
         doxygen \
         git \

doc/Doxyfile.in

@@ -53,4 +53,9 @@ GENERATE_PERLMOD        = NO
 #---------------------------------------------------------------------------
 # Configuration options related to the preprocessor
 #---------------------------------------------------------------------------
-INCLUDE_PATH            = "@CMAKE_SOURCE_DIR@/src"
+INCLUDE_PATH            = "@CMAKE_SOURCE_DIR@/src"
+
+#---------------------------------------------------------------------------
+# Configuration options related to diagram generator tools
+#---------------------------------------------------------------------------
+HAVE_DOT                = NO

doc/developers/build.rst

@@ -8,11 +8,10 @@ Required dependencies on Fedora and Ubuntu:
 .. code-block:: shell
 
     # Fedora
-    sudo dnf install -y bison bpftool clang clang-tools-extra cmake doxygen flex git lcov libasan libbpf-devel libcmocka-devel libnl3-devel libubsan python3-breathe python3-furo python3-linuxdoc python3-sphinx pkgconf
+    sudo dnf install -y bison bpftool clang clang-tools-extra cmake doxygen flex git jq lcov libasan libbpf-devel libcmocka-devel libnl3-devel libubsan python3-breathe python3-furo python3-linuxdoc python3-sphinx pkgconf
 
     # Ubuntu
-    sudo apt-get install -y bison clang clang-format clang-tidy cmake doxygen flex git furo lcov libpf-dev libcmocka-dev libnl-3-dev linux-tools-common python3-breathe python3-pip python3-sphinx pkgconf
-    pip3 install linuxdoc
+    sudo apt-get install -y bison clang clang-format clang-tidy cmake doxygen flex furo git jq lcov libpf-dev libcmocka-dev libnl-3-dev linux-tools-common python3-breathe python3-pip python3-sphinx pkgconf pip3 install linuxdoc
 
 You can then use CMake to generate the build system:
 

doc/developers/style.rst

@@ -5,7 +5,7 @@ Coding style
 
     This document is not yet complete, it will evolve gradually over time. If you are unsure about a specific rule: check ClangFormat's configuration (``.clang-format``), check this document, and check the existing code. If none of those can answer your question, do as you want.
 
-``bpfilter`` coding style is enforced by ClangFormat, as defined in its configuration file ``.clang-format`` at the root of the repository. The ``checkstyle`` build target can be used to validate all the source files under ``src``, but the issues won't be resolved automatically: changes performed by ClangFormat should be controlled by a developer.
+``bpfilter`` coding style is enforced by ClangFormat, as defined in its configuration file ``.clang-format`` at the root of the repository. The ``check`` build target can be used to validate all the source files under ``src``, but the issues won't be resolved automatically: changes performed by ClangFormat should be controlled by a developer.
 
 To format a source file using ClangFormat (from the root of the repository):
 

src/bfcli/main.c

@@ -4,15 +4,20 @@
  */
 
 #include <argp.h>
+#include <errno.h>
 #include <stdarg.h>
+#include <stdint.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>
 
 #include "bfcli/lexer.h"
 #include "bfcli/parser.h"
 #include "core/chain.h"
-#include "core/dump.h"
+#include "core/front.h"
+#include "core/helper.h"
 #include "core/list.h"
+#include "core/logger.h"
 #include "core/marsh.h"
 #include "core/request.h"
 #include "core/response.h"
@@ -23,7 +28,7 @@ int bf_send(const struct bf_request *request, struct bf_response **response);
 static struct bf_options
 {
     const char *input_file;
-} _opts = {
+} _bf_opts = {
     .input_file = NULL,
 };
 
@@ -41,16 +46,12 @@ static error_t _bf_opts_parser(int key, char *arg, struct argp_state *state)
     switch (key) {
     case 'f':
         opts->input_file = strdup(arg);
-        if (!opts->input_file) {
-            fprintf(stderr, "failed to allocate memory for '%s'\n", arg);
-            return -ENOMEM;
-        }
+        if (!opts->input_file)
+            return bf_err_r(-ENOMEM, "failed to copy input file path");
         break;
     case ARGP_KEY_END:
-        if (!opts->input_file) {
-            fprintf(stderr, "--file is required\n");
-            return -EINVAL;
-        }
+        if (!opts->input_file)
+            return bf_err_r(-EINVAL, "--file argument is required");
         break;
     default:
         return ARGP_ERR_UNKNOWN;
@@ -62,37 +63,44 @@ static error_t _bf_opts_parser(int key, char *arg, struct argp_state *state)
 int main(int argc, char *argv[])
 {
     struct argp argp = {options, _bf_opts_parser, NULL, NULL, 0, NULL, NULL};
-    bf_list chains = bf_list_default({.free = (bf_list_ops_free)bf_chain_free});
-    bf_list sets = bf_list_default({.free = (bf_list_ops_free)bf_set_free});
+    struct bf_ruleset ruleset = {
+        .chains = bf_list_default({.free = (bf_list_ops_free)bf_chain_free}),
+        .sets = bf_list_default({.free = (bf_list_ops_free)bf_set_free}),
+    };
     int r;
 
-    r = argp_parse(&argp, argc, argv, 0, 0, &_opts);
+    bf_logger_setup();
+
+    r = argp_parse(&argp, argc, argv, 0, 0, &_bf_opts);
     if (r) {
-        fprintf(stderr, "failed to parse arguments\n");
-        return EXIT_FAILURE;
+        bf_err_r(r, "failed to parse arguments");
+        goto end_clean;
     }
 
-    printf("Using source file: %s\n", _opts.input_file);
+    bf_info("using source file: %s", _bf_opts.input_file);
 
-    FILE *rules = fopen(_opts.input_file, "r");
+    FILE *rules = fopen(_bf_opts.input_file, "r");
     if (!rules) {
-        fprintf(stderr, "Failed to read rules from 'rule.bpfilter'\n");
-        return -1;
+        r = errno;
+        bf_err_r(r, "failed to read rules from %s:", _bf_opts.input_file);
+        goto end_clean;
     }
 
     yyin = rules;
 
-    r = yyparse(&chains, &sets);
+    r = yyparse(&ruleset);
     if (r == 1) {
-        fprintf(stderr, "failed to parse rules, syntax invalid\n");
-        return EXIT_FAILURE;
+        bf_err("failed to parse rules, invalid syntax");
+        r = -EINVAL;
+        goto end_close;
     } else if (r == 2) {
-        fprintf(stderr, "failed to parse rules, not enough memory\n");
-        return EXIT_FAILURE;
+        bf_err("failed to parse rules, not enough memory");
+        r = -EINVAL;
+        goto end_close;
     }
 
     // Set rules indexes
-    bf_list_foreach (&chains, chain_node) {
+    bf_list_foreach (&ruleset.chains, chain_node) {
         struct bf_chain *chain = bf_list_node_get_data(chain_node);
         uint32_t index = 0;
 
@@ -102,21 +110,21 @@ int main(int argc, char *argv[])
         }
     }
 
-    bf_list_foreach (&chains, chain_node) {
+    bf_list_foreach (&ruleset.chains, chain_node) {
         struct bf_chain *chain = bf_list_node_get_data(chain_node);
         _cleanup_bf_request_ struct bf_request *request = NULL;
         _cleanup_bf_response_ struct bf_response *response = NULL;
         _cleanup_bf_marsh_ struct bf_marsh *marsh = NULL;
 
         r = bf_chain_marsh(chain, &marsh);
         if (r) {
-            fprintf(stderr, "failed to marsh chain, skipping\n");
+            bf_err_r(r, "failed to marsh chain, skipping");
             continue;
         }
 
         r = bf_request_new(&request, marsh, bf_marsh_size(marsh));
         if (r) {
-            fprintf(stderr, "failed to create request for chain, skipping\n");
+            bf_err_r(r, "failed to create request for chain, skipping");
             continue;
         }
 
@@ -125,34 +133,32 @@ int main(int argc, char *argv[])
 
         r = bf_send(request, &response);
         if (r) {
-            fprintf(stderr,
-                    "failed to send chain creation request, skipping\n");
+            bf_err_r(r, "failed to send chain creation request, skipping");
             continue;
         }
 
         if (response->type == BF_RES_FAILURE) {
-            fprintf(stderr, "chain creation request failed, %d received\n",
-                    response->error);
+            bf_err_r(response->error, "chain creation request failed");
             continue;
         }
     }
 
-    bf_list_clean(&chains);
-    fclose(rules);
+end_close:
+    (void)fclose(rules);
+end_clean:
+    bf_list_clean(&ruleset.chains);
+    bf_list_clean(&ruleset.sets);
 
-    return 0;
+    return r;
 }
 
-void yyerror(bf_list *chains, bf_list *sets, const char *fmt, ...)
+void yyerror(struct bf_ruleset *ruleset, const char *fmt, ...)
 {
-    UNUSED(chains);
-    UNUSED(sets);
+    UNUSED(ruleset);
 
     va_list args;
 
     va_start(args, fmt);
-    vfprintf(stderr, fmt, args);
+    bf_err_v(fmt, args);
     va_end(args);
-
-    exit(-1);
 }