From e592dd89f1d6a6613b2ca37434ad568da0998b19 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 22 Apr 2024 08:51:58 +0200 Subject: [PATCH 1/4] build: always define NL_DEBUG Checking conditional defines with #ifdef is error prone because we don't get a compiler warning when the define wrongly is missing. Instead, always define it to either 0 or 1. The benefit is also that now we can use NL_DEBUG in C (not only in the preprocessor). --- configure.ac | 9 ++++++--- include/nl-aux-core/nl-core.h | 2 +- lib/cache_mngr.c | 4 ++-- lib/route/neigh.c | 2 +- lib/route/route_obj.c | 4 ++-- lib/utils.c | 2 +- 6 files changed, 13 insertions(+), 10 deletions(-) diff --git a/configure.ac b/configure.ac index bc8b3908..cce0ad0b 100644 --- a/configure.ac +++ b/configure.ac @@ -110,10 +110,13 @@ fi AM_CONDITIONAL([ENABLE_STATIC], [test "$enable_static" != "no"]) AC_ARG_ENABLE([debug], - AS_HELP_STRING([--disable-debug], [Do not include debugging statements]), - [enable_debug="$enableval"], [enable_debug="yes"]) -if test "x$enable_debug" = "xyes"; then + AS_HELP_STRING([--disable-debug], [Do not include debugging statements]), + [enable_debug="$enableval"], [enable_debug="yes"]) +if test "$enable_debug" = "yes"; then AC_DEFINE([NL_DEBUG], [1], [Define to 1 to enable debugging]) +else + enable_debug=no + AC_DEFINE([NL_DEBUG], [0], [Define to 1 to enable debugging]) fi AC_CONFIG_SUBDIRS([doc]) diff --git a/include/nl-aux-core/nl-core.h b/include/nl-aux-core/nl-core.h index 5b34bcb0..c6bdbebb 100644 --- a/include/nl-aux-core/nl-core.h +++ b/include/nl-aux-core/nl-core.h @@ -5,7 +5,7 @@ #include "base/nl-base-utils.h" -#ifdef NL_DEBUG +#if NL_DEBUG #define NL_DBG(LVL, FMT, ARG...) \ do { \ if (LVL <= nl_debug) { \ diff --git a/lib/cache_mngr.c b/lib/cache_mngr.c index f16882f7..0caf10d1 100644 --- a/lib/cache_mngr.c +++ b/lib/cache_mngr.c @@ -58,7 +58,7 @@ static int include_cb(struct nl_object *obj, struct nl_parser_param *p) struct nl_cache_ops *ops = ca->ca_cache->c_ops; NL_DBG(2, "Including object %p into cache %p\n", obj, ca->ca_cache); -#ifdef NL_DEBUG +#if NL_DEBUG if (nl_debug >= 4) nl_object_dump(obj, &nl_debug_dp); #endif @@ -93,7 +93,7 @@ static int event_input(struct nl_msg *msg, void *arg) NL_DBG(2, "Cache manager %p, handling new message %p as event\n", mngr, msg); -#ifdef NL_DEBUG +#if NL_DEBUG if (nl_debug >= 4) nl_msg_dump(msg, stderr); #endif diff --git a/lib/route/neigh.c b/lib/route/neigh.c index 91500244..1f19fed0 100644 --- a/lib/route/neigh.c +++ b/lib/route/neigh.c @@ -242,7 +242,7 @@ static void neigh_keygen(struct nl_object *obj, uint32_t *hashkey, uint16_t n_vlan; char n_addr[0]; } _nl_packed *nkey; -#ifdef NL_DEBUG +#if NL_DEBUG char buf[INET6_ADDRSTRLEN+5]; #endif diff --git a/lib/route/route_obj.c b/lib/route/route_obj.c index ce68259c..488cff5b 100644 --- a/lib/route/route_obj.c +++ b/lib/route/route_obj.c @@ -345,7 +345,7 @@ static void route_keygen(struct nl_object *obj, uint32_t *hashkey, uint32_t rt_prio; char rt_addr[0]; } _nl_packed *rkey = NULL; -#ifdef NL_DEBUG +#if NL_DEBUG char buf[INET6_ADDRSTRLEN+5]; #endif @@ -502,7 +502,7 @@ static int route_update(struct nl_object *old_obj, struct nl_object *new_obj) struct rtnl_route *old_route = (struct rtnl_route *) old_obj; struct rtnl_nexthop *new_nh; int action = new_obj->ce_msgtype; -#ifdef NL_DEBUG +#if NL_DEBUG char buf[INET6_ADDRSTRLEN+5]; #endif diff --git a/lib/utils.c b/lib/utils.c index 4f5fd1aa..2363c027 100644 --- a/lib/utils.c +++ b/lib/utils.c @@ -52,7 +52,7 @@ int nl_debug = 0; /** @cond SKIP */ -#ifdef NL_DEBUG +#if NL_DEBUG struct nl_dump_params nl_debug_dp = { .dp_type = NL_DUMP_DETAILS, }; From dbe21b8d64cdfd38476cddee0bb766fa7d4a7c3c Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 22 Apr 2024 09:13:24 +0200 Subject: [PATCH 2/4] core: always define statements for NL_DBG() Conditionally defining to nothing, means that the compiler doesn't see the print statement without NL_DEBUG. In turn, we lack checking of the statement by the compiler. Instead, add an "if (NL_DEBUG)" around it. Since NL_DEBUG is a constant, the compiler will optimize out all the code of the statement, while still checking it. --- include/nl-aux-core/nl-core.h | 11 +++-------- lib/route/neigh.c | 2 -- lib/route/route_obj.c | 4 ---- 3 files changed, 3 insertions(+), 14 deletions(-) diff --git a/include/nl-aux-core/nl-core.h b/include/nl-aux-core/nl-core.h index c6bdbebb..5198296f 100644 --- a/include/nl-aux-core/nl-core.h +++ b/include/nl-aux-core/nl-core.h @@ -5,21 +5,16 @@ #include "base/nl-base-utils.h" -#if NL_DEBUG #define NL_DBG(LVL, FMT, ARG...) \ do { \ - if (LVL <= nl_debug) { \ - int _errsv = errno; \ + if ((NL_DEBUG) && (LVL) <= nl_debug) { \ + const int _errsv = errno; \ + \ fprintf(stderr, "DBG<" #LVL ">%20s:%-4u %s: " FMT, \ __FILE__, __LINE__, __func__, ##ARG); \ errno = _errsv; \ } \ } while (0) -#else /* NL_DEBUG */ -#define NL_DBG(LVL, FMT, ARG...) \ - do { \ - } while (0) -#endif /* NL_DEBUG */ struct nl_addr; void nl_addr_put(struct nl_addr *); diff --git a/lib/route/neigh.c b/lib/route/neigh.c index 1f19fed0..7e698b49 100644 --- a/lib/route/neigh.c +++ b/lib/route/neigh.c @@ -242,9 +242,7 @@ static void neigh_keygen(struct nl_object *obj, uint32_t *hashkey, uint16_t n_vlan; char n_addr[0]; } _nl_packed *nkey; -#if NL_DEBUG char buf[INET6_ADDRSTRLEN+5]; -#endif if (neigh->n_family == AF_BRIDGE) { if (neigh->n_lladdr) diff --git a/lib/route/route_obj.c b/lib/route/route_obj.c index 488cff5b..50775937 100644 --- a/lib/route/route_obj.c +++ b/lib/route/route_obj.c @@ -345,9 +345,7 @@ static void route_keygen(struct nl_object *obj, uint32_t *hashkey, uint32_t rt_prio; char rt_addr[0]; } _nl_packed *rkey = NULL; -#if NL_DEBUG char buf[INET6_ADDRSTRLEN+5]; -#endif if (route->rt_dst) addr = route->rt_dst; @@ -502,9 +500,7 @@ static int route_update(struct nl_object *old_obj, struct nl_object *new_obj) struct rtnl_route *old_route = (struct rtnl_route *) old_obj; struct rtnl_nexthop *new_nh; int action = new_obj->ce_msgtype; -#if NL_DEBUG char buf[INET6_ADDRSTRLEN+5]; -#endif /* * ipv6 ECMP route notifications from the kernel come as From 264b244e4d0044d6598d247e94f6d39defaede90 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 22 Apr 2024 09:39:40 +0200 Subject: [PATCH 3/4] utils: always define nl_debug_dp Otherwise, whether libnl-3.so exports nl_debug_dp depends on NL_DEBUG. That is ugly. It also breaks the linker checking the symbol versioning file with the "--no-undefined-version" flag. Instead, always define it. It's small anyway. Reported-by: lch361 See-also: https://github.com/thom311/libnl/pull/375 --- lib/cache_mngr.c | 10 ++++------ lib/utils.c | 4 +--- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/lib/cache_mngr.c b/lib/cache_mngr.c index 0caf10d1..7b2a9bd3 100644 --- a/lib/cache_mngr.c +++ b/lib/cache_mngr.c @@ -58,10 +58,9 @@ static int include_cb(struct nl_object *obj, struct nl_parser_param *p) struct nl_cache_ops *ops = ca->ca_cache->c_ops; NL_DBG(2, "Including object %p into cache %p\n", obj, ca->ca_cache); -#if NL_DEBUG - if (nl_debug >= 4) + + if (NL_DEBUG && nl_debug >= 4) nl_object_dump(obj, &nl_debug_dp); -#endif if (ops->co_event_filter) if (ops->co_event_filter(ca->ca_cache, obj) != NL_OK) @@ -93,10 +92,9 @@ static int event_input(struct nl_msg *msg, void *arg) NL_DBG(2, "Cache manager %p, handling new message %p as event\n", mngr, msg); -#if NL_DEBUG - if (nl_debug >= 4) + + if (NL_DEBUG && nl_debug >= 4) nl_msg_dump(msg, stderr); -#endif if (mngr->cm_protocol != protocol) BUG(); diff --git a/lib/utils.c b/lib/utils.c index 2363c027..471cd21a 100644 --- a/lib/utils.c +++ b/lib/utils.c @@ -52,7 +52,6 @@ int nl_debug = 0; /** @cond SKIP */ -#if NL_DEBUG struct nl_dump_params nl_debug_dp = { .dp_type = NL_DUMP_DETAILS, }; @@ -61,7 +60,7 @@ static void _nl_init nl_debug_init(void) { char *nldbg, *end; - if ((nldbg = getenv("NLDBG"))) { + if (NL_DEBUG && (nldbg = getenv("NLDBG"))) { long level = strtol(nldbg, &end, 0); if (nldbg != end) nl_debug = level; @@ -69,7 +68,6 @@ static void _nl_init nl_debug_init(void) nl_debug_dp.dp_fd = stderr; } -#endif int __nl_read_num_str_file(const char *path, int (*cb)(long, const char *)) { From 13ab0122fc38462d16b2aa920167220ca196f15d Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 22 Apr 2024 10:17:16 +0200 Subject: [PATCH 4/4] github: test with --enable-debug=no configure option In this case, only test it with clang. It seems not worth building everything twice toggling only this option. --- .github/workflows/ci.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 708928e9..25bb0b9c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -71,15 +71,17 @@ jobs: export CC="${{ matrix.cc }}" export CFLAGS="-DNL_MORE_ASSERTS=1000 -O2 -Werror -Wall -Wdeclaration-after-statement -Wvla -std=gnu11 -fexceptions" + CONFIGURE_ARGS= if [ "$CC" = "clang" ]; then CFLAGS="$CFLAGS -Wno-error=unused-command-line-argument -Wno-error=unused-function" export LDFLAGS="-Wl,--no-undefined-version,--fatal-warnings" + CONFIGURE_ARGS="--enable-debug=no" else export LDFLAGS="-Wl,--no-undefined-version" fi ./autogen.sh - ./configure + ./configure $CONFIGURE_ARGS make -j 5 shell: bash