Add bridge exclusion#1479
Conversation
Signed-off-by: Niklas Ciecior <niklas.ciecior@tricera.energy>
📝 WalkthroughWalkthroughThe changes introduce support for topic exclusions in bridge node configurations. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
include/nng/supplemental/nanolib/conf.h (1)
227-227: Type inconsistency:topic_lenusessize_twhile similar fields useuint32_t.The
topicsstruct (lines 230-247) usesuint32_tforremote_topic_lenandlocal_topic_len, but the newexclusionsstruct usessize_tfortopic_len. This inconsistency could lead to issues on platforms wheresize_tdiffers fromuint32_t, and it makes the codebase less uniform.♻️ Suggested fix for type consistency
typedef struct { char *topic; - size_t topic_len; + uint32_t topic_len; } exclusions;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/nng/supplemental/nanolib/conf.h` at line 227, The new exclusions struct introduces topic_len as size_t which is inconsistent with the topics struct fields remote_topic_len and local_topic_len that use uint32_t; change topic_len to uint32_t to match existing types and avoid platform-dependent size differences—update the declaration of topic_len in the exclusions struct and verify any code that reads/writes exclusions->topic_len uses uint32_t (referencing the exclusions struct and topics' remote_topic_len/local_topic_len symbols to locate all related usages).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/nng/supplemental/nanolib/conf.h`:
- Around line 225-228: The exclusions parsed into the exclusions struct
(exclusions { char *topic; size_t topic_len; }) and stored in exclusions_list
are never consulted during bridge forwarding; update the bridge
message-forwarding path (where messages are forwarded across the bridge) to
iterate the exclusions_list and skip forwarding when a message's topic matches
any exclusions[i].topic using topic_len-aware comparison (memcmp or strncmp) or
exact string compare as appropriate; ensure you use the same lifetime/ownership
semantics as the parser (don't free entries when skipping) and add unit/inline
checks so that the forwarding function returns early when a match is found.
In `@src/supplemental/nanolib/conf_ver2.c`:
- Around line 1267-1282: The loop over exclusions_list currently breaks on an
invalid empty/null topic which aborts processing remaining exclusions; change
the behavior in the cJSON_ArrayForEach(exclusion, exclusions_list) loop so that
after hocon_read_str(exc, topic, exclusion) you validate topic_len and on
invalid topic call log_error, free the allocated struct (NNI_FREE_STRUCT(exc))
and continue (not break) to skip only that entry; additionally add the same
maximum-topic-length check used in forwards/subscription parsing (compare
topic_len against the established MAX_TOPIC_LEN constant or the same validation
routine), free exc and continue if it exceeds the max, otherwise set
exc->topic_len and cvector_push_back(node->exclusions_list, exc); finally ensure
node->exclusions_count = cvector_size(node->exclusions_list) remains after the
loop.
In `@src/supplemental/nanolib/conf.c`:
- Around line 3770-3776: The format string is unsafe: %.*s expects an int
precision and %ld expects a long, but topic_len is size_t and j is size_t;
update the logging call in the loop that uses node->exclusions_count,
node->exclusions_list and node->exclusions_list[j]->topic_len so the types match
the format specifiers—cast topic_len to int for the precision argument (e.g.,
(int)node->exclusions_list[j]->topic_len) and either switch %ld to %zu for j or
cast j to long (prefer %zu to match size_t); adjust the log_info invocation
accordingly.
---
Nitpick comments:
In `@include/nng/supplemental/nanolib/conf.h`:
- Line 227: The new exclusions struct introduces topic_len as size_t which is
inconsistent with the topics struct fields remote_topic_len and local_topic_len
that use uint32_t; change topic_len to uint32_t to match existing types and
avoid platform-dependent size differences—update the declaration of topic_len in
the exclusions struct and verify any code that reads/writes
exclusions->topic_len uses uint32_t (referencing the exclusions struct and
topics' remote_topic_len/local_topic_len symbols to locate all related usages).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4457307-5452-49cb-88ee-439d90d47c37
📒 Files selected for processing (3)
include/nng/supplemental/nanolib/conf.hsrc/supplemental/nanolib/conf.csrc/supplemental/nanolib/conf_ver2.c
| typedef struct { | ||
| char *topic; | ||
| size_t topic_len; | ||
| } exclusions; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for usages of exclusions_list or exclusions_count beyond parsing/printing/cleanup
# Search for exclusions usage in the codebase
rg -n "exclusions" --type=c -C3 | grep -v "conf_ver2.c" | grep -v "conf\.c" | grep -v "conf\.h"Repository: nanomq/NanoNNG
Length of output: 72
🏁 Script executed:
# Search for all exclusions usage without filters to see full picture
rg -n "exclusions" --type=c -B2 -A2Repository: nanomq/NanoNNG
Length of output: 3934
🏁 Script executed:
# Search for message filtering/forwarding logic that might use exclusions_list
rg -n "forward|filter" --type=c -l | head -20Repository: nanomq/NanoNNG
Length of output: 654
🏁 Script executed:
# Check the conf.c file specifically for exclusions usage and filtering logic
fd -e c -e h "^conf\.c$" -x cat -n {}Repository: nanomq/NanoNNG
Length of output: 50371
🏁 Script executed:
# Search for bridge message forwarding/processing functions
rg -n "forward|bridge" --type=c -l | grep -E "bridge|mqtt" | head -20Repository: nanomq/NanoNNG
Length of output: 462
🏁 Script executed:
# Search for any use of exclusions_list for filtering in the codebase
rg -n "exclusions_list\[" --type=c -A3 -B3Repository: nanomq/NanoNNG
Length of output: 1099
🏁 Script executed:
# Look for functions that might perform topic filtering during message forwarding
rg -n "topic.*match|match.*topic|filter.*topic" --type=c -l | head -10Repository: nanomq/NanoNNG
Length of output: 278
🏁 Script executed:
# Find bridge-related source files
fd -e c "bridge" src/ | head -20Repository: nanomq/NanoNNG
Length of output: 40
🏁 Script executed:
# Look for functions that process bridge messages/forwarding
rg -n "bridge.*forward|forward.*bridge|bridge.*message" --type=c -l | head -10Repository: nanomq/NanoNNG
Length of output: 166
🏁 Script executed:
# Search for any function that checks or matches exclusions
rg -n "exclusion" --type=c -B5 -A5 | head -100Repository: nanomq/NanoNNG
Length of output: 6778
Exclusions are parsed but never enforced at runtime.
The exclusions_list is populated during configuration parsing and freed during destruction, but there's no code that actually uses these exclusions to filter messages during bridge forwarding. This means the feature is incomplete—users can configure exclusions, but they won't have any effect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@include/nng/supplemental/nanolib/conf.h` around lines 225 - 228, The
exclusions parsed into the exclusions struct (exclusions { char *topic; size_t
topic_len; }) and stored in exclusions_list are never consulted during bridge
forwarding; update the bridge message-forwarding path (where messages are
forwarded across the bridge) to iterate the exclusions_list and skip forwarding
when a message's topic matches any exclusions[i].topic using topic_len-aware
comparison (memcmp or strncmp) or exact string compare as appropriate; ensure
you use the same lifetime/ownership semantics as the parser (don't free entries
when skipping) and add unit/inline checks so that the forwarding function
returns early when a match is found.
| cJSON *exclusions_list = hocon_get_obj("exclusions", obj); | ||
| cJSON *exclusion = NULL; | ||
| cJSON_ArrayForEach(exclusion, exclusions_list) | ||
| { | ||
| exclusions *exc = NNI_ALLOC_STRUCT(exc); | ||
| hocon_read_str(exc, topic, exclusion); | ||
| size_t topic_len = exc->topic ? strlen(exc->topic) : 0; | ||
| if (topic_len == 0) { | ||
| log_error("Exclusion topic should not be null or empty"); | ||
| NNI_FREE_STRUCT(exc); | ||
| break; | ||
| } | ||
| exc->topic_len = topic_len; | ||
| cvector_push_back(node->exclusions_list, exc); | ||
| } | ||
| node->exclusions_count = cvector_size(node->exclusions_list); |
There was a problem hiding this comment.
Using break on validation failure skips all remaining exclusions.
When an exclusion has an empty or null topic, the code breaks out of the loop entirely, discarding any subsequent valid exclusions. Consider using continue instead to skip only the invalid entry while still processing the rest.
Additionally, unlike the similar forwards/subscription parsing, there's no maximum topic length validation here. Extremely long topic strings could cause issues.
🐛 Proposed fix
cJSON *exclusions_list = hocon_get_obj("exclusions", obj);
cJSON *exclusion = NULL;
cJSON_ArrayForEach(exclusion, exclusions_list)
{
exclusions *exc = NNI_ALLOC_STRUCT(exc);
hocon_read_str(exc, topic, exclusion);
size_t topic_len = exc->topic ? strlen(exc->topic) : 0;
- if (topic_len == 0) {
- log_error("Exclusion topic should not be null or empty");
- NNI_FREE_STRUCT(exc);
+ if (topic_len == 0 || topic_len > UINT16_MAX) {
+ log_warn("Invalid exclusion topic length: %zu, skipping", topic_len);
+ if (exc->topic) {
+ nng_strfree(exc->topic);
+ }
+ NNI_FREE_STRUCT(exc);
- break;
+ continue;
}
exc->topic_len = topic_len;
cvector_push_back(node->exclusions_list, exc);
}
node->exclusions_count = cvector_size(node->exclusions_list);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cJSON *exclusions_list = hocon_get_obj("exclusions", obj); | |
| cJSON *exclusion = NULL; | |
| cJSON_ArrayForEach(exclusion, exclusions_list) | |
| { | |
| exclusions *exc = NNI_ALLOC_STRUCT(exc); | |
| hocon_read_str(exc, topic, exclusion); | |
| size_t topic_len = exc->topic ? strlen(exc->topic) : 0; | |
| if (topic_len == 0) { | |
| log_error("Exclusion topic should not be null or empty"); | |
| NNI_FREE_STRUCT(exc); | |
| break; | |
| } | |
| exc->topic_len = topic_len; | |
| cvector_push_back(node->exclusions_list, exc); | |
| } | |
| node->exclusions_count = cvector_size(node->exclusions_list); | |
| cJSON *exclusions_list = hocon_get_obj("exclusions", obj); | |
| cJSON *exclusion = NULL; | |
| cJSON_ArrayForEach(exclusion, exclusions_list) | |
| { | |
| exclusions *exc = NNI_ALLOC_STRUCT(exc); | |
| hocon_read_str(exc, topic, exclusion); | |
| size_t topic_len = exc->topic ? strlen(exc->topic) : 0; | |
| if (topic_len == 0 || topic_len > UINT16_MAX) { | |
| log_warn("Invalid exclusion topic length: %zu, skipping", topic_len); | |
| if (exc->topic) { | |
| nng_strfree(exc->topic); | |
| } | |
| NNI_FREE_STRUCT(exc); | |
| continue; | |
| } | |
| exc->topic_len = topic_len; | |
| cvector_push_back(node->exclusions_list, exc); | |
| } | |
| node->exclusions_count = cvector_size(node->exclusions_list); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/supplemental/nanolib/conf_ver2.c` around lines 1267 - 1282, The loop over
exclusions_list currently breaks on an invalid empty/null topic which aborts
processing remaining exclusions; change the behavior in the
cJSON_ArrayForEach(exclusion, exclusions_list) loop so that after
hocon_read_str(exc, topic, exclusion) you validate topic_len and on invalid
topic call log_error, free the allocated struct (NNI_FREE_STRUCT(exc)) and
continue (not break) to skip only that entry; additionally add the same
maximum-topic-length check used in forwards/subscription parsing (compare
topic_len against the established MAX_TOPIC_LEN constant or the same validation
routine), free exc and continue if it exceeds the max, otherwise set
exc->topic_len and cvector_push_back(node->exclusions_list, exc); finally ensure
node->exclusions_count = cvector_size(node->exclusions_list) remains after the
loop.
| log_info("%sbridge.mqtt.%s.exclusions: ", prefix, node->name); | ||
| for (size_t j = 0; j < node->exclusions_count; j++) { | ||
| log_info( | ||
| "\t[%ld] topic:\t\t%.*s", j, | ||
| node->exclusions_list[j]->topic_len, | ||
| node->exclusions_list[j]->topic); | ||
| } |
There was a problem hiding this comment.
Potential format string issue: %.*s precision expects int, but topic_len is size_t.
The %.*s format specifier expects an int for the precision argument, but topic_len is size_t. On 64-bit platforms where size_t is 64-bit, this could cause undefined behavior or incorrect output. The same issue exists for the loop index j being size_t but used with %ld.
🛡️ Proposed fix to cast the precision argument
log_info("%sbridge.mqtt.%s.exclusions: ", prefix, node->name);
for (size_t j = 0; j < node->exclusions_count; j++) {
log_info(
- "\t[%ld] topic:\t\t%.*s", j,
- node->exclusions_list[j]->topic_len,
+ "\t[%zu] topic:\t\t%.*s", j,
+ (int)node->exclusions_list[j]->topic_len,
node->exclusions_list[j]->topic);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/supplemental/nanolib/conf.c` around lines 3770 - 3776, The format string
is unsafe: %.*s expects an int precision and %ld expects a long, but topic_len
is size_t and j is size_t; update the logging call in the loop that uses
node->exclusions_count, node->exclusions_list and
node->exclusions_list[j]->topic_len so the types match the format
specifiers—cast topic_len to int for the precision argument (e.g.,
(int)node->exclusions_list[j]->topic_len) and either switch %ld to %zu for j or
cast j to long (prefer %zu to match size_t); adjust the log_info invocation
accordingly.
1 participant
Recreated after i completly destroid #1464. Git submodules are such fun.
This pull request introduces support for "exclusions" in the bridge configuration, allowing specific topics to be excluded from bridge operations. The changes include updates to the bridge node structure, parsing logic, cleanup routines, and configuration printing to handle exclusions alongside existing forwards and subscriptions.
Bridge exclusions support:
exclusionsstruct and correspondingexclusions_listandexclusions_countfields to theconf_bridge_nodestructure inconf.h, enabling storage of excluded topics. [1] [2]conf_bridge_node_parse) to read and validate theexclusionsarray from the configuration, allocate exclusion entries, and track their count.conf_bridge_node_destroy) to properly free all allocated exclusions and their topics, preventing memory leaks.print_bridge_conf) to display the exclusions for each bridge node, improving debuggability and transparency.Minor code cleanup:
Summary by CodeRabbit
Release Notes