in_snmp: an input plugin for collect metrics by SNMP request #7671
Conversation
k402xxxcenxxx
changed the title
k402xxxcenxxx
force-pushed
the
feature/in_snmp
branch
from
869e75c to
eb87261
Compare
k402xxxcenxxx
requested review from
edsiper,
fujimotos,
koleini,
leonardo-albertovich,
niedbalski and
patrick-stephens
as code owners
|
I have found a leak in the code, and it can be resolved by applying this patch: net-snmp/net-snmp@4bd0d9a. However, I am unsure about the process of incorporating this fix into the fluent-bit project. Any suggestions or guidance on this matter would be greatly appreciated! |
CMakeLists.txt
Outdated
| option(FLB_IN_PROC "Enable Process input plugin" Yes) | ||
| option(FLB_IN_SYSTEMD "Enable Systemd input plugin" Yes) | ||
| option(FLB_IN_DUMMY "Enable Dummy input plugin" Yes) | ||
| option(FLB_IN_SNMP "Enable SNMP input plugin" Yes) |
There was a problem hiding this comment.
To confirm - does this work on Windows and macOS as well? It will need disabling if not in their specific override cmake/ files.
| flex \ | ||
| bison \ | ||
| libyaml-dev \ | ||
| libsnmp-dev \ |
There was a problem hiding this comment.
We presumably will also need this in the various package builds under packaging/distros too.
|
@patrick-stephens thanks, I will try to build and run on Windows device |
k402xxxcenxxx
force-pushed
the
feature/in_snmp
branch
from
eb87261 to
02ae8c0
Compare
k402xxxcenxxx
force-pushed
the
feature/in_snmp
branch
from
02ae8c0 to
63597e4
Compare
k402xxxcenxxx
force-pushed
the
feature/in_snmp
branch
2 times, most recently
from
265dab6 to
1c78626
Compare
|
I still fail to build it on Windows, so I choose to disable it in https://github.com/fluent/fluent-bit/blob/master/cmake/windows-setup.cmake. |
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
@k402xxxcenxxx What's needed to finish this PR? Do you need help testing on a Windows device? |
Hi @davidvasandani , Thanks for asking. I've been too busy lately to maintain this PR and I'm also considering not supporting this on Windows. Thanks again for your kind reminding. |
k402xxxcenxxx
force-pushed
the
feature/in_snmp
branch
from
1c78626 to
9a4ed5a
Compare
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
@k402xxxcenxxx would you mind resolving the conflicts in this PR? |
k402xxxcenxxx
force-pushed
the
feature/in_snmp
branch
from
9a4ed5a to
98a6aea
Compare
WalkthroughAdds a new SNMP input plugin (sources, header, build, tests, mocks), a CMake Find module for Net‑SNMP, a CMake option (FLB_IN_SNMP, default ON) with Windows default disabled, and installs Net‑SNMP dev packages across Dockerfiles/packaging and CI script. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Init as Plugin Init
participant SNMP as Net‑SNMP
participant Collector as in_snmp.collect
participant Enc as Encoder
participant Out as Fluent Bit Output
Init->>SNMP: snmp_sess_init / open session (host,port,version,community)
Init->>Enc: init encoder
loop periodic collection
Collector->>SNMP: SNMP GET or GETNEXT (walk)
SNMP-->>Collector: Response (OID, value) / error
Collector->>Collector: parse & build MsgPack record(s)
Collector->>Enc: append record(s)
Collector->>Out: emit event
end
Collector->>SNMP: cleanup session, unload MIBs
Collector->>Enc: cleanup encoder
sequenceDiagram
autonumber
participant Test as Test Runner
participant FB as Fluent Bit (test)
participant Plugin as in_snmp
participant Mock as Mock SNMP responder
participant Validator as MsgPack Validator
Test->>FB: start in test mode (input + dummy output)
FB->>Plugin: init plugin
loop scenarios (GET / WALK)
Plugin->>Mock: request SNMP data (mock)
Mock-->>Plugin: return mocked response
Plugin->>Plugin: encode to MsgPack
Plugin->>Validator: submit emitted record(s)
end
Test->>FB: stop and cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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: 15
🧹 Nitpick comments (10)
packaging/distros/ubuntu/Dockerfile (1)
28-28: SNMP headers included — good; minor package list cleanup on Xenial.Additions of libsnmp-dev are consistent and OK. On Ubuntu 16.04 stanza, software-properties-common appears twice (Line 27 and Line 28). Deduplicate to reduce noise.
- libpq-dev postgresql-server-dev-all software-properties-common \ - software-properties-common libyaml-dev libsnmp-dev apt-transport-https \ + libpq-dev postgresql-server-dev-all software-properties-common \ + libyaml-dev libsnmp-dev apt-transport-https \Also applies to: 60-60, 94-94, 124-124, 150-150, 174-174, 200-200, 224-224, 250-250
tests/runtime/in_snmp.c (3)
56-63: Avoid relying on map index/order; match by key.MsgPack maps are unordered. Search for the OID key and then validate its value to make the test robust.
- TEST_CHECK(obj->type == MSGPACK_OBJECT_MAP); - TEST_CHECK(strncmp("iso.3.6.1.2.1.1.3.0", - obj->via.map.ptr[0].key.via.str.ptr, - obj->via.map.ptr[0].key.via.str.size) == 0); - TEST_CHECK(strncmp("123", - obj->via.map.ptr[0].val.via.str.ptr, - obj->via.map.ptr[0].val.via.str.size) == 0); + TEST_CHECK(obj->type == MSGPACK_OBJECT_MAP); + bool found = false; + for (uint32_t k = 0; k < obj->via.map.size; k++) { + msgpack_object mkey = obj->via.map.ptr[k].key; + msgpack_object mval = obj->via.map.ptr[k].val; + if (mkey.type == MSGPACK_OBJECT_STR && + mkey.via.str.size == strlen("iso.3.6.1.2.1.1.3.0") && + strncmp("iso.3.6.1.2.1.1.3.0", mkey.via.str.ptr, mkey.via.str.size) == 0) { + TEST_CHECK(mval.type == MSGPACK_OBJECT_STR); + TEST_CHECK(mval.via.str.size == strlen("123")); + TEST_CHECK(strncmp("123", mval.via.str.ptr, mval.via.str.size) == 0); + found = true; + break; + } + } + TEST_CHECK(found == true);
83-86: Confirm expected WALK value quoting.expected_vals include quotes (e.g., ""Fa0/0""). If the plugin emits raw strings without extra quotes, the comparison will fail. Please verify and adjust expected values accordingly.
Also applies to: 101-104
124-133: Add allocation checks and clean up env vars after the test.records allocation can fail (see flb_calloc behavior in tests). Also unset env vars to avoid cross-test interference.
- records = flb_calloc(1, sizeof(struct callback_records)); - records->num_records = 0; - records->records = NULL; + records = flb_calloc(1, sizeof(struct callback_records)); + TEST_CHECK(records != NULL); + if (!records) { + return; + } + records->num_records = 0; + records->records = NULL;And near the end:
- flb_destroy(ctx); + flb_destroy(ctx); + unsetenv("FLB_SNMP_PLUGIN_UNDER_TEST"); + unsetenv("TEST_SNMP_RESPONSE");plugins/in_snmp/CMakeLists.txt (1)
1-2: Wire include dirs and set link scope; optionally guard build on detection.Use the discovered include dirs and mark link as PRIVATE. If not already gated at top-level, wrap with NETSNMP_FOUND.
set(NETSNMP_LIBRARIES ${NETSNMP_LIBRARY}) set(NETSNMP_INCLUDE_DIRS ${NETSNMP_INCLUDE_DIR}) @@ FLB_PLUGIN(in_snmp "${src}" "") -target_link_libraries(flb-plugin-in_snmp ${NETSNMP_LIBRARIES}) +target_include_directories(flb-plugin-in_snmp PRIVATE ${NETSNMP_INCLUDE_DIRS}) +target_link_libraries(flb-plugin-in_snmp PRIVATE ${NETSNMP_LIBRARIES})If not globally enforced, consider:
# Optional guard if top-level doesn't gate this plugin already if (NOT NETSNMP_FOUND) message(FATAL_ERROR "Net-SNMP not found; in_snmp cannot be built") endif()Also applies to: 7-8
plugins/in_snmp/in_snmp.h (1)
7-7: Minimize transitive dependency from headers.Including net-snmp headers in a public plugin header couples consumers to Net-SNMP. Consider moving netsnmp_session into the .c file and storing an opaque pointer in the header.
plugins/in_snmp/in_snmp.c (2)
112-143: bool usage without an explicit include.If bool is not provided transitively, this will fail on some toolchains. Add <stdbool.h> or switch to int.
321-329: Collector interval is hard-coded to 1s, but default SNMP timeout is 5s.This can cause overlapping scheduled wakeups and continuous backlog under slow devices. Expose an interval option or set a safer default (>= timeout).
cmake/FindNetsnmp.cmake (2)
21-48: Library search paths are too narrow; prefer pkg-config and cover lib64/multiarch.Relying on hard-coded /usr/lib misses /usr/lib64, Debian multiarch, and Homebrew. Also, only netsnmp is typically required here.
Prefer:
find_package(PkgConfig REQUIRED) pkg_check_modules(NETSNMP REQUIRED IMPORTED_TARGET net-snmp) set(NETSNMP_INCLUDE_DIR ${NETSNMP_INCLUDE_DIR:-${NETSNMP_INCLUDE_DIRS}}) set(NETSNMP_LIBRARIES PkgConfig::NETSNMP)If keeping manual search, extend PATHS with /usr/lib64, /usr/lib/, /opt/homebrew/lib and set NETSNMP_INCLUDE_DIRS in addition to NETSNMP_INCLUDE_DIR for consumers.
Also applies to: 50-56
59-64: Expose include dirs consistently.Downstream CMake typically expects NETSNMP_INCLUDE_DIRS. Set it alongside NETSNMP_INCLUDE_DIR and include it in FIND_PACKAGE_HANDLE_STANDARD_ARGS.
-FIND_PACKAGE_HANDLE_STANDARD_ARGS(NETSNMP - DEFAULT_MSG - NETSNMP_INCLUDE_DIR - NETSNMP_LIBRARIES -) +set(NETSNMP_INCLUDE_DIRS ${NETSNMP_INCLUDE_DIR}) +FIND_PACKAGE_HANDLE_STANDARD_ARGS(NETSNMP + DEFAULT_MSG + NETSNMP_INCLUDE_DIRS + NETSNMP_LIBRARIES +)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
CMakeLists.txt(1 hunks)cmake/FindNetsnmp.cmake(1 hunks)cmake/windows-setup.cmake(1 hunks)dockerfiles/Dockerfile(1 hunks)dockerfiles/Dockerfile.centos7(1 hunks)packaging/distros/amazonlinux/Dockerfile(4 hunks)packaging/distros/centos/Dockerfile(8 hunks)packaging/distros/debian/Dockerfile(8 hunks)packaging/distros/raspbian/Dockerfile(3 hunks)packaging/distros/ubuntu/Dockerfile(9 hunks)plugins/CMakeLists.txt(1 hunks)plugins/in_snmp/CMakeLists.txt(1 hunks)plugins/in_snmp/in_snmp.c(1 hunks)plugins/in_snmp/in_snmp.h(1 hunks)tests/runtime/CMakeLists.txt(1 hunks)tests/runtime/in_snmp.c(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-08T11:21:33.975Z
Learnt from: cosmo0920
PR: fluent/fluent-bit#10851
File: include/fluent-bit/flb_simd.h:60-66
Timestamp: 2025-09-08T11:21:33.975Z
Learning: Fluent Bit currently only supports MSVC compiler on Windows, so additional compiler compatibility guards may be unnecessary for Windows-specific code paths.
Applied to files:
CMakeLists.txt
🧬 Code graph analysis (2)
plugins/in_snmp/in_snmp.c (4)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_log_event_encoder.c (6)
flb_log_event_encoder_begin_record(246-254)flb_log_event_encoder_set_current_timestamp(289-292)flb_log_event_encoder_commit_record(256-274)flb_log_event_encoder_reset(31-40)flb_log_event_encoder_init(42-74)flb_log_event_encoder_destroy(99-116)include/fluent-bit/flb_input.h (1)
flb_input_config_map_set(715-741)src/flb_input.c (4)
flb_input_set_context(1610-1613)flb_input_set_collector_time(1685-1704)flb_input_collector_pause(2035-2072)flb_input_collector_resume(2095-2151)
tests/runtime/in_snmp.c (3)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)plugins/out_s3/s3.c (1)
setenv(57-60)src/flb_lib.c (9)
flb_create(138-220)flb_input(261-271)flb_input_set(300-330)flb_output(274-284)flb_output_set(515-546)flb_start(914-925)flb_stop(942-985)flb_lib_free(693-700)flb_destroy(223-258)
🔇 Additional comments (10)
dockerfiles/Dockerfile (1)
58-58: LGTM! Dependency addition aligns with SNMP plugin integration.The addition of
libsnmp-devenables SNMP functionality in the builder-base stage. This addresses the earlier review comment about needing SNMP libraries across packaging targets, which has been consistently applied throughout the distribution-specific Dockerfiles in this PR.dockerfiles/Dockerfile.centos7 (1)
13-13: LGTM! Correct SNMP dependency for CentOS.The addition of
libsnmp-devel(RPM package naming) is consistent with the SNMP plugin integration and aligns with the Debian-based Dockerfile changes.cmake/windows-setup.cmake (1)
84-84: LGTM! Windows exclusion aligns with PR discussion.Explicitly disabling the SNMP input plugin on Windows is consistent with the PR comments indicating build failures on Windows and the decision to not support the platform initially. The placement among other Windows-disabled plugins (lines 49-84) follows the established pattern.
plugins/CMakeLists.txt (1)
284-284: LGTM! Plugin registration follows standard pattern.The SNMP input plugin registration is placed appropriately in alphabetical order and follows the standard registration pattern. The
REGISTER_IN_PLUGINmacro (lines 40-73) will automatically skip registration ifFLB_IN_SNMPis disabled (e.g., on Windows per cmake/windows-setup.cmake:84).tests/runtime/CMakeLists.txt (1)
49-49: LGTM! Runtime test registration follows established pattern.The SNMP runtime test registration is correctly placed in alphabetical order and uses the standard
FLB_RT_TESTmacro, which will conditionally compile the test only whenFLB_IN_SNMPis enabled.packaging/distros/raspbian/Dockerfile (3)
21-21: LGTM! Consistent SNMP dependency across Raspbian variants.The addition of
libsnmp-devto the Raspbian buster base image aligns with the SNMP plugin integration.
34-34: LGTM! Consistent SNMP dependency across Raspbian variants.The addition of
libsnmp-devto the Raspbian bullseye base image maintains consistency with other distribution variants.
47-47: LGTM! Consistent SNMP dependency across Raspbian variants.The addition of
libsnmp-devto the Raspbian bookworm base image completes the consistent SNMP support across all Raspbian versions (buster, bullseye, bookworm).packaging/distros/centos/Dockerfile (1)
26-26: LGTM! Comprehensive SNMP dependency coverage across CentOS variants.The addition of
libsnmp-develto all CentOS base images (versions 7, 8, 9, 10 across both x86_64 and arm64v8 architectures) provides complete SNMP plugin support across the CentOS packaging matrix. This addresses the earlier review feedback requesting packaging/distros updates.Also applies to: 61-61, 105-105, 144-144, 178-178, 207-207, 241-241, 270-270
packaging/distros/debian/Dockerfile (1)
31-31: SNMP development libs added across Debian variants — looks good.No issues spotted; package naming matches Debian conventions.
Also applies to: 62-62, 86-86, 112-112, 136-136, 162-162, 186-186, 212-212
| include(ExternalProject) | ||
| include(cmake/FindJournald.cmake) | ||
| include(cmake/FindMonkey.cmake) | ||
| include(cmake/FindNetsnmp.cmake) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify macOS support status for the SNMP plugin.
The inclusion of cmake/FindNetsnmp.cmake enables Net-SNMP discovery during configuration. While Windows support is explicitly disabled (cmake/windows-setup.cmake:84), the past review comments requested confirmation of macOS compatibility.
Please confirm whether macOS support is:
- Intended and tested (Net-SNMP is available via Homebrew)
- Disabled via cmake/macos-setup.cmake (similar to Windows)
- Left to fail at build-time if dependencies are missing
🏁 Script executed:
#!/bin/bash
# Check if macOS-specific SNMP configuration exists
echo "=== Checking for macOS-specific SNMP handling ==="
if [ -f "cmake/macos-setup.cmake" ]; then
echo "Found cmake/macos-setup.cmake, checking for SNMP references:"
rg -n "SNMP|snmp" cmake/macos-setup.cmake || echo "No SNMP configuration found in macOS setup"
else
echo "No cmake/macos-setup.cmake found"
fi
echo ""
echo "=== Checking plugin registration for platform conditionals ==="
rg -n -A2 -B2 "REGISTER_IN_PLUGIN.*in_snmp" plugins/CMakeLists.txtLength of output: 390
🏁 Script executed:
#!/bin/bash
# Examine FindNetsnmp.cmake for platform-specific handling
echo "=== FindNetsnmp.cmake content ==="
cat cmake/FindNetsnmp.cmake
echo ""
echo "=== Checking plugins/in_snmp/CMakeLists.txt ==="
cat plugins/in_snmp/CMakeLists.txt
echo ""
echo "=== Checking windows-setup.cmake for SNMP handling ==="
rg -n -A3 -B3 "SNMP|snmp" cmake/windows-setup.cmake || echo "No SNMP references found"Length of output: 2098
🏁 Script executed:
#!/bin/bash
# Find how FLB_IN_SNMP is used in the build system
echo "=== FLB_IN_SNMP usage throughout CMakeLists.txt files ==="
rg -n "FLB_IN_SNMP" --type cmake
echo ""
echo "=== Check plugins/CMakeLists.txt around REGISTER_IN_PLUGIN(in_snmp) for conditionals ==="
rg -n -B10 "REGISTER_IN_PLUGIN.*in_snmp" plugins/CMakeLists.txt | head -30
echo ""
echo "=== Check if NETSNMP_FOUND is verified before using in_snmp plugin ==="
rg -n "NETSNMP_FOUND" --type cmake
echo ""
echo "=== Check main CMakeLists.txt for in_snmp conditionals ==="
rg -n "in_snmp\|IN_SNMP" CMakeLists.txtLength of output: 855
🏁 Script executed:
#!/bin/bash
# Find how in_snmp plugin directory is included in the build
echo "=== Search for add_subdirectory calls for in_snmp ==="
rg -n "add_subdirectory.*in_snmp" --type cmake
echo ""
echo "=== Check plugins/CMakeLists.txt for how FLB_IN_* flags gate plugin inclusion ==="
rg -n -B2 "add_subdirectory" plugins/CMakeLists.txt | head -50
echo ""
echo "=== Search for how FLB_IN_SNMP actually gates plugin building ==="
rg -n "FLB_IN_SNMP" . --type cmake -A2 -B2Length of output: 1753
🏁 Script executed:
#!/bin/bash
# Find REGISTER_IN_PLUGIN macro definition
echo "=== Search for REGISTER_IN_PLUGIN macro definition ==="
rg -n "macro.*REGISTER_IN_PLUGIN\|function.*REGISTER_IN_PLUGIN" --type cmake
echo ""
echo "=== Check where FindNetsnmp.cmake is included ==="
rg -n "FindNetsnmp\|find_package.*NETSNMP\|include.*FindNetsnmp" --type cmake
echo ""
echo "=== Check main CMakeLists.txt for NETSNMP handling ==="
rg -n -B3 -A3 "find_package.*SNMP\|FIND_PACKAGE.*SNMP\|include.*FindNetsnmp" CMakeLists.txt
echo ""
echo "=== Check CMakeLists.txt line 125 context (the line being reviewed) ==="
sed -n '120,130p' CMakeLists.txtLength of output: 601
🏁 Script executed:
#!/bin/bash
# Final verification: Check if there's any error handling or conditional build for SNMP
echo "=== Check plugins/in_snmp/CMakeLists.txt for NETSNMP_FOUND checks ==="
cat plugins/in_snmp/CMakeLists.txt
echo ""
echo "=== Search for conditional NETSNMP handling in cmake files ==="
rg -n "if.*NETSNMP_FOUND\|if.*NETSNMP_LIBRARY" --type cmake
echo ""
echo "=== Check if main CMakeLists.txt has error handling after FindNetsnmp.cmake ==="
sed -n '125,145p' CMakeLists.txt
echo ""
echo "=== Verify windows-setup.cmake applies before plugin registration ==="
rg -n -A20 "if.*WIN32\|if.*WINDOWS" CMakeLists.txt | head -30Length of output: 1096
Update FindNetsnmp.cmake to support Homebrew, or explicitly disable SNMP on macOS.
macOS support status remains unresolved. The current implementation will fail to build on macOS with Homebrew-installed Net-SNMP:
- FindNetsnmp.cmake searches only
/usr/liband/usr/local/lib, missing Homebrew's standard install paths (/usr/local/opt/net-snmpon Intel,/opt/homebrew/opt/net-snmpon Apple Silicon) - plugins/in_snmp/CMakeLists.txt unconditionally links against potentially empty
${NETSNMP_LIBRARY}without checkingNETSNMP_FOUND - in_snmp plugin registration (plugins/CMakeLists.txt:284) is unconditional with no macOS guard, unlike the pattern used for other platform-specific plugins
The Windows disable mechanism (FLB_IN_SNMP=No in cmake/windows-setup.cmake:84) only gates tests, not the actual plugin build. Add equivalent macOS handling or update the SNMP finder to support Homebrew paths.
🤖 Prompt for AI Agents
In CMakeLists.txt around line 125 where FindNetsnmp.cmake is included, the build
will fail on macOS Homebrew installs because the finder only checks /usr/lib and
/usr/local/lib and the in_snmp plugin is unconditionally linked/registered;
update FindNetsnmp.cmake to also probe Homebrew prefixes
(/usr/local/opt/net-snmp and /opt/homebrew/opt/net-snmp) for include and lib
paths and set NETSNMP_FOUND appropriately, modify plugins/in_snmp/CMakeLists.txt
to conditionalize target_link_libraries and add_library calls on NETSNMP_FOUND
(error out or skip if not found), and guard the plugins/CMakeLists.txt in_snmp
registration with a platform or option check (e.g., skip on Apple or when
FLB_IN_SNMP=No) so macOS/Homebrew users can disable the plugin like Windows
does.
| char *target_host; | ||
| int port; | ||
| int timeout; |
There was a problem hiding this comment.
Port option is not used by the session.
ctx->port is never applied to netsnmp_session, so the configured port is ignored. Compose peername with the port or set the session remote port during init.
Apply in init (see suggested diff in in_snmp.c).
🤖 Prompt for AI Agents
In plugins/in_snmp/in_snmp.h around lines 16-18 the ctx->port field is never
applied to the netsnmp_session so user-configured ports are ignored; update the
SNMP session initialization in in_snmp.c to apply ctx->port by either composing
the session peername to include ":<port>" or by setting the session remote port
(e.g., ss->remote_port = ctx->port) after ss is created, ensuring you handle
defaults and validate the port value before assignment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmake/FindNetsnmp.cmake (1)
23-51: Consider broadening library search paths.The FIND_LIBRARY calls explicitly specify only
/usr/liband/usr/local/lib. While CMake will also search system default paths, you may want to either:
- Remove the explicit
PATHSspecification to rely fully on CMake's default search behavior- Add additional common paths such as
/usr/lib64,/usr/lib/x86_64-linux-gnu,/opt/homebrew/lib, and/opt/local/libfor better cross-platform supportExample for the first library:
SET(NETSNMP_NAMES netsnmp) FIND_LIBRARY(NETSNMP_LIBRARY NAMES ${NETSNMP_NAMES} - PATHS /usr/lib /usr/local/lib )Or add more paths:
SET(NETSNMP_NAMES netsnmp) FIND_LIBRARY(NETSNMP_LIBRARY NAMES ${NETSNMP_NAMES} - PATHS /usr/lib /usr/local/lib + PATHS /usr/lib /usr/lib64 /usr/local/lib /opt/homebrew/lib /opt/local/lib )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmake/FindNetsnmp.cmake(1 hunks)cmake/plugins_options.cmake(1 hunks)
🔇 Additional comments (3)
cmake/plugins_options.cmake (1)
52-52: LGTM! Plugin option follows the standard pattern.The SNMP input plugin option is correctly defined and positioned alphabetically among other input plugins.
cmake/FindNetsnmp.cmake (2)
16-21: Include path search looks good after previous fix.The FIND_PATH now correctly searches for
net-snmp/net-snmp-includes.hin common include directories. This addresses the previous review concern about searching for the wrong header.
58-58: Verify whether netsnmptrapd is intentionally excluded.The
NETSNMPTRAPD_LIBRARYis discovered but commented out of the finalNETSNMP_LIBRARIESaggregation. For a plugin that performs SNMP polling (rather than receiving traps), excluding trapd makes sense. However, please confirm this is intentional and consider adding a comment explaining why it's excluded.If the plugin indeed doesn't need trap handling, consider adding a clarifying comment:
${NETSNMPMIBS_LIBRARY} -# ${NETSNMPTRAPD_LIBRARY} +# ${NETSNMPTRAPD_LIBRARY} # trapd not needed for polling-only plugin )
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cmake/FindNetsnmp.cmake (2)
16-51: Consider using pkg-config for more robust discovery.Net-SNMP typically provides
net-snmp-configand pkg-config metadata. Modern CMake Find modules often usepkg_check_modulesorpkg_search_moduleas a primary or fallback discovery mechanism, which automatically handles distro-specific paths, linker flags, and versions without hardcoding paths.Example pattern to try pkg-config first:
# Try pkg-config first FIND_PACKAGE(PkgConfig QUIET) IF(PKG_CONFIG_FOUND) PKG_CHECK_MODULES(PC_NETSNMP QUIET netsnmp) ENDIF() FIND_PATH(NETSNMP_INCLUDE_DIR net-snmp/net-snmp-includes.h HINTS ${PC_NETSNMP_INCLUDE_DIRS} PATHS /usr/include /usr/local/include /opt/homebrew/include /opt/local/include ) FIND_LIBRARY(NETSNMP_LIBRARY NAMES ${NETSNMP_NAMES} HINTS ${PC_NETSNMP_LIBRARY_DIRS} PATHS /usr/lib /usr/local/lib /opt/homebrew/lib /opt/local/lib )
58-58: Document whyNETSNMPTRAPD_LIBRARYis excluded.The
netsnmptrapdlibrary is searched (lines 47–51) but commented out ofNETSNMP_LIBRARIES. Adding a brief comment explaining the reason (e.g., "not required for input plugin" or "optional component") would help future maintainers understand the intent.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
plugins/in_snmp/in_snmp.c (4)
458-462: Help text does not match actual behavior.The
oid_typehelp text mentions "Use a 'field'…" but the accepted values are "get" (for a single OID retrieval) and "walk" (to traverse a subtree), as validated in lines 142-150.Update the description for clarity:
{ FLB_CONFIG_MAP_STR, "oid_type", "get", 0, FLB_TRUE, offsetof(struct flb_snmp, oid_type), - "set the type of the SNMP request. Use a 'field' to collect a variable by OID." + "set the type of SNMP request: 'get' for a single OID, 'walk' to traverse an OID subtree." },
199-285: Critical memory leak: buffers reallocated per variable but never freed in loop.
oid_bufandbufare allocated inside the per-variable loop (lines 211 and 228) but only freed once at the end of the function (lines 308-309). In an SNMP walk returning multiple variables, all but the last allocation for each buffer will be leaked because the pointer is overwritten on each iteration.Apply this fix to free the buffers after each variable is processed:
} else { flb_plg_error(ctx->ins, "[TRUNCATED]"); if (response) { snmp_free_pdu(response); } ret = -1; goto collect_cleanup; } } + + /* Free per-iteration buffers to avoid leaks */ + if (buf) { + flb_free(buf); + buf = NULL; + } + if (oid_buf) { + flb_free(oid_buf); + oid_buf = NULL; + } if ((vars->type != SNMP_ENDOFMIBVIEW) &&
152-159: Pass NULL explicitly to snmp_error when session is NULL.When
snmp_open()fails and returns NULL, the first argument tosnmp_error()should beNULLrather than the NULLsspointer for clarity.Apply this diff:
ss = snmp_open(&ctx->session); if (!ss) { - snmp_error(ss, NULL, NULL, &err); + snmp_error(NULL, NULL, NULL, &err); flb_plg_error(ctx->ins, "%s", err); ret = -1; goto collect_cleanup; }
375-392: Port configuration ignored; session timeout/retries only set globally.Line 378 assigns only
target_hosttopeername, ignoring the configuredportvalue. SNMP requests will always use the default port 161. Additionally, timeout and retries are set as library-wide defaults (lines 370-371) but not on the session itself.Apply this fix to honor the port and set session-specific timeouts:
snmp_sess_init(&ctx->session); - ctx->session.peername = ctx->target_host; + /* Compose peername with port */ + { + size_t need = strlen(ctx->target_host) + 32; + char *peer = flb_calloc(1, need); + if (!peer) { + flb_plg_error(ctx->ins, "failed to allocate peername"); + flb_log_event_encoder_destroy(&ctx->log_encoder); + flb_free(ctx); + return -1; + } + snprintf(peer, need, "%s:%d", ctx->target_host, ctx->port); + ctx->session.peername = peer; + /* Note: Remember to free this in in_snmp_exit */ + } + + /* Set session-specific timeout (in microseconds) and retries */ + ctx->session.timeout = ctx->timeout * 1000000L; + ctx->session.retries = ctx->retries; if (strcmp(ctx->version, "1") == 0) {Then in
in_snmp_exit(), free the allocated peername:static int in_snmp_exit(void *data, struct flb_config *config) { (void) config; struct flb_snmp *ctx = data; flb_log_event_encoder_destroy(&ctx->log_encoder); + /* Free allocated peername if it differs from target_host */ + if (ctx->session.peername && ctx->session.peername != ctx->target_host) { + flb_free((void *)ctx->session.peername); + } + flb_free(ctx);
🧹 Nitpick comments (1)
plugins/in_snmp/in_snmp.c (1)
276-280: Consider freeing err immediately after logging for cleaner resource management.While the current cleanup path at line 307 does free
err, freeing it immediately after use would make the code more maintainable and reduce the chance of leaks if the code is modified later.Optional improvement:
} else { snmp_error(ss, NULL, NULL, &err); flb_plg_error(ctx->ins, "%s", err); + if (err) { + free(err); + err = NULL; + } running = 0; }Note: Use
free()rather thanflb_free()sincesnmp_error()allocates withmalloc().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/in_snmp/in_snmp.c(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/in_snmp/in_snmp.c (4)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_log_event_encoder.c (5)
flb_log_event_encoder_begin_record(246-254)flb_log_event_encoder_commit_record(256-274)flb_log_event_encoder_reset(31-40)flb_log_event_encoder_init(42-74)flb_log_event_encoder_destroy(99-116)include/fluent-bit/flb_input.h (1)
flb_input_config_map_set(715-741)src/flb_input.c (4)
flb_input_set_context(1610-1613)flb_input_set_collector_time(1685-1704)flb_input_collector_pause(2035-2072)flb_input_collector_resume(2095-2151)
🔇 Additional comments (5)
plugins/in_snmp/in_snmp.c (5)
16-117: Test helper implementation looks good.The test support functions properly handle PDU cleanup and NULL checks for environment variables as recommended in previous reviews.
160-165: Proper cleanup path established.The OID parsing error path now correctly uses
goto collect_cleanupto ensure resources are freed, addressing the previous review concern.
304-312: Cleanup block is functional.The cleanup successfully frees allocated resources. Note that
responseis already freed in the loop (line 283) or was never allocated, so line 305 is defensive but doesn't leak.
396-424: Lifecycle functions correctly implemented.Pause, resume, and exit callbacks properly manage the collector state and cleanup resources. Previous concerns about config dereferencing and OpenSSL cleanup have been addressed.
471-482: Plugin registration structure is correct.The plugin registration properly declares all required callbacks and metadata for integration with Fluent Bit.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
plugins/in_snmp/in_snmp.c (1)
307-312: Consider freeing response PDU defensively in cleanup.Line 308 sets
responseto NULL without freeing it. While current code paths ensure response is freed elsewhere (line 286 or before eachgoto), defensive programming suggests actually freeing it here:collect_cleanup: - if (response) { response = NULL; } + if (response) { snmp_free_pdu(response); response = NULL; } if (ss) { snmp_close(ss); ss = NULL; } if (err) { flb_free(err); err = NULL; } if (buf) { flb_free(buf); buf = NULL; } if (oid_buf) { flb_free(oid_buf); oid_buf = NULL; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/in_snmp/in_snmp.c(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/in_snmp/in_snmp.c (4)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_log_event_encoder.c (6)
flb_log_event_encoder_begin_record(246-254)flb_log_event_encoder_set_current_timestamp(289-292)flb_log_event_encoder_commit_record(256-274)flb_log_event_encoder_reset(31-40)flb_log_event_encoder_init(42-74)flb_log_event_encoder_destroy(99-116)include/fluent-bit/flb_input.h (1)
flb_input_config_map_set(715-741)src/flb_input.c (4)
flb_input_set_context(1610-1613)flb_input_set_collector_time(1685-1704)flb_input_collector_pause(2035-2072)flb_input_collector_resume(2095-2151)
🔇 Additional comments (4)
plugins/in_snmp/in_snmp.c (4)
16-117: LGTM! Test helpers properly handle PDU cleanup and environment checks.The mock functions correctly mirror
snmp_synch_responsesemantics by freeing the request PDU and checking for NULL environment variables before proceeding.
199-254: LGTM! Per-iteration buffer management is correct.The code properly frees
bufandoid_bufafter each loop iteration (lines 252-253) and frees the response PDU on error paths before jumping to cleanup. This fixes the memory leaks identified in previous reviews.
413-427: LGTM! Exit function correctly avoids dereferencing config and process-wide cleanup.The function properly:
- Casts config without dereferencing (line 415)
- Destroys the encoder and frees context
- Shuts down SNMP and cleans up sockets
- Omits the problematic
OPENSSL_cleanup()callNote: If you apply the port fix suggested above (allocating peername), remember to free it here before freeing ctx.
429-485: LGTM! Configuration map help text is clear and accurate.The
oid_typedescription (line 464) now correctly documents the accepted values ('get' and 'walk') and their behavior, matching the implementation.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
plugins/in_snmp/in_snmp.c (2)
124-140: Initialize pointer variables to NULL to prevent undefined behavior in cleanup.Three pointer variables (
ss,response,err) are declared but not initialized. If an early error occurs (e.g., lines 149, 157, 164) and execution jumps tocollect_cleanup, the cleanup code at lines 309-310 will test uninitialized pointers.Apply this fix:
- netsnmp_session *ss; + netsnmp_session *ss = NULL; netsnmp_pdu *pdu; - netsnmp_pdu *response; + netsnmp_pdu *response = NULL; oid anOID[MAX_OID_LEN], name[MAX_OID_LEN], end_oid[MAX_OID_LEN]; size_t anOID_len, name_len, end_len; netsnmp_variable_list *vars; int status; u_char *buf = NULL, *oid_buf = NULL; size_t buf_len = 256, oid_buf_len = 256, out_len = 0, oid_out_len = 0; int running = 1; int buf_overflow = 0; bool is_walk = false; - char *err; + char *err = NULL;
442-444: Free the allocated peername before freeing ctx.The
session.peernamestring is dynamically allocated inin_snmp_init(line 389) and assigned at line 404. This allocated memory must be freed inin_snmp_exitto prevent a leak.Apply this fix:
flb_log_event_encoder_destroy(&ctx->log_encoder); + /* Free the allocated peername */ + if (ctx->session.peername) { + flb_free((void *)ctx->session.peername); + } + flb_free(ctx);
🧹 Nitpick comments (1)
plugins/in_snmp/in_snmp.c (1)
382-382: Use debug-level logging instead of error-level for informational messages.Lines 382 and 402 use
flb_plg_errorto log the target port and peer name. Since these are informational (not error conditions), consider usingflb_plg_debuginstead, or remove them entirely before production release.Apply this diff:
- flb_plg_error(ctx->ins, "target_port : %d", ctx->target_port); + flb_plg_debug(ctx->ins, "target_port : %d", ctx->target_port); /* compose peername by using target_host and port */ size_t host_len = strlen(ctx->target_host); int port_len = snprintf(NULL, 0, "%d", ctx->target_port); size_t static_len = strlen("udp::"); // "udp:" + ":" size_t need = static_len + host_len + port_len + 1; char *peer = flb_calloc(1, need); if (!peer) { flb_plg_error(ctx->ins, "oom building peername"); flb_free(ctx); return -1; } int written = snprintf(peer, need, "udp:%s:%d", ctx->target_host, ctx->target_port); if (written < 0 || (size_t)written >= need) { flb_plg_error(ctx->ins, "peername truncated, need %zu bytes", need); flb_free(peer); flb_free(ctx); return -1; } - flb_plg_error(ctx->ins, "peer : %s", peer); + flb_plg_debug(ctx->ins, "peer : %s", peer);Also applies to: 402-402
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packaging/distros/ubuntu/Dockerfile(9 hunks)plugins/in_snmp/in_snmp.c(1 hunks)plugins/in_snmp/in_snmp.h(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packaging/distros/ubuntu/Dockerfile
- plugins/in_snmp/in_snmp.h
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/in_snmp/in_snmp.c (4)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_log_event_encoder.c (6)
flb_log_event_encoder_begin_record(246-254)flb_log_event_encoder_set_current_timestamp(289-292)flb_log_event_encoder_commit_record(256-274)flb_log_event_encoder_reset(31-40)flb_log_event_encoder_init(42-74)flb_log_event_encoder_destroy(99-116)include/fluent-bit/flb_input.h (1)
flb_input_config_map_set(715-741)src/flb_input.c (4)
flb_input_set_context(1610-1613)flb_input_set_collector_time(1685-1704)flb_input_collector_pause(2035-2072)flb_input_collector_resume(2095-2151)
| collect_cleanup: | ||
| if (response) { response = NULL; } | ||
| if (ss) { snmp_close(ss); ss = NULL; } | ||
| if (err) { flb_free(err); err = NULL; } | ||
| if (buf) { flb_free(buf); buf = NULL; } | ||
| if (oid_buf) { flb_free(oid_buf); oid_buf = NULL; } |
There was a problem hiding this comment.
Free response PDU in cleanup instead of just setting to NULL.
Line 308 checks if response is non-NULL but only sets it to NULL without freeing the PDU. Although response should normally be freed before reaching cleanup (line 285-287 or error paths at lines 213, 222, 230, 246), defensive cleanup should actually free it to prevent leaks in edge cases.
Apply this fix:
collect_cleanup:
- if (response) { response = NULL; }
+ if (response) { snmp_free_pdu(response); response = NULL; }
if (ss) { snmp_close(ss); ss = NULL; }
if (err) { flb_free(err); err = NULL; }
if (buf) { flb_free(buf); buf = NULL; }
if (oid_buf) { flb_free(oid_buf); oid_buf = NULL; }🤖 Prompt for AI Agents
In plugins/in_snmp/in_snmp.c around lines 307 to 312, the cleanup block
currently only sets the response pointer to NULL without freeing the SNMP PDU;
replace that no-op with a proper free by calling snmp_free_pdu(response) and
then set response to NULL so the PDU is released in all cleanup paths.
| { | ||
| flb_plg_error(ctx->ins, "target_port : %d", ctx->target_port); | ||
|
|
||
| /* compose peername by using target_host and port */ | ||
| size_t host_len = strlen(ctx->target_host); | ||
| int port_len = snprintf(NULL, 0, "%d", ctx->target_port); | ||
| size_t static_len = strlen("udp::"); // "udp:" + ":" | ||
| size_t need = static_len + host_len + port_len + 1; | ||
| char *peer = flb_calloc(1, need); | ||
| if (!peer) { | ||
| flb_plg_error(ctx->ins, "oom building peername"); | ||
| flb_free(ctx); | ||
| return -1; | ||
| } | ||
| int written = snprintf(peer, need, "udp:%s:%d", ctx->target_host, ctx->target_port); | ||
| if (written < 0 || (size_t)written >= need) { | ||
| flb_plg_error(ctx->ins, "peername truncated, need %zu bytes", need); | ||
| flb_free(peer); | ||
| flb_free(ctx); | ||
| return -1; | ||
| } | ||
| flb_plg_error(ctx->ins, "peer : %s", peer); | ||
|
|
||
| ctx->session.peername = peer; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that the allocated peername is freed in the exit function.
The peername string is dynamically allocated at line 389 and assigned to ctx->session.peername at line 404. This allocation must be freed in in_snmp_exit to prevent a memory leak.
🏁 Script executed:
#!/bin/bash
# Verify that the allocated peername is freed in in_snmp_exit
rg -n "session\.peername" plugins/in_snmp/in_snmp.c -B2 -A2Length of output: 364
---
🏁 Script executed:
```shell
#!/bin/bash
# Find the in_snmp_exit function and check for cleanup
rg -n "in_snmp_exit|session\.peername|flb_free.*peer" plugins/in_snmp/in_snmp.c -A3 -B1
Length of output: 795
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full in_snmp_exit function
sed -n '437,470p' plugins/in_snmp/in_snmp.c
Length of output: 882
Add explicit cleanup of ctx->session.peername before freeing ctx in in_snmp_exit.
The peername string is allocated at line 389 and assigned to ctx->session.peername at line 404. In in_snmp_exit (line 437), the context is freed with flb_free(ctx), but this does not free the separately allocated peername string. Add flb_free(ctx->session.peername); before flb_free(ctx); to prevent a memory leak.
Additionally, lines 382 and 402 use flb_plg_error for debug information (target_port and peer values). Consider using flb_plg_debug instead.
🤖 Prompt for AI Agents
In plugins/in_snmp/in_snmp.c around lines 381-405, the allocated peername
(created at ~line 389 and assigned to ctx->session.peername at ~line 404) is
never freed when the context is torn down; update in_snmp_exit (around line 437)
to call flb_free(ctx->session.peername) (guarded by a NULL check) before
flb_free(ctx) to avoid the leak, and change the two diagnostic calls at ~lines
382 and 402 from flb_plg_error to flb_plg_debug to use the appropriate log
level.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packaging/distros/amazonlinux/Dockerfile (1)
26-26: Use net-snmp-devel instead of libsnmp-devel on Amazon Linux.On RPM-based distributions like Amazon Linux, the correct package name is
net-snmp-devel, notlibsnmp-devel. Using the wrong package name will cause build failures.Also applies to: 51-51, 76-76, 103-103
plugins/in_snmp/in_snmp.c (2)
124-140: Initialize pointer variables to NULL to prevent undefined behavior.The pointers
ss,response, anderrare declared but not initialized. If an early error occurs (e.g., lines 149, 157, 164) before these pointers are set, the cleanup code at lines 308-310 will check uninitialized pointers, causing undefined behavior and potential crashes.Apply this fix:
- netsnmp_session *ss; + netsnmp_session *ss = NULL; netsnmp_pdu *pdu; - netsnmp_pdu *response; + netsnmp_pdu *response = NULL; oid anOID[MAX_OID_LEN], name[MAX_OID_LEN], end_oid[MAX_OID_LEN]; size_t anOID_len, name_len, end_len; netsnmp_variable_list *vars; int status; u_char *buf = NULL, *oid_buf = NULL; size_t buf_len = 256, oid_buf_len = 256, out_len = 0, oid_out_len = 0; int running = 1; int buf_overflow = 0; bool is_walk = false; - char *err; + char *err = NULL;
434-448: Free allocated peername to prevent memory leak.The peername string is dynamically allocated during initialization (line 387 in
in_snmp_init) and assigned toctx->session.peername(line 401), but it is never freed in the exit function. This causes a memory leak on plugin shutdown.Apply this fix:
static int in_snmp_exit(void *data, struct flb_config *config) { (void) config; struct flb_snmp *ctx = data; flb_log_event_encoder_destroy(&ctx->log_encoder); + if (ctx->session.peername) { + flb_free((void *)ctx->session.peername); + } flb_free(ctx); snmp_shutdown(PLUGIN_NAME); unload_all_mibs(); SOCK_CLEANUP; return 0; }
🧹 Nitpick comments (2)
plugins/in_snmp/in_snmp.c (2)
307-312: Free response PDU in cleanup for defensive programming.While normal code paths free the response PDU before reaching cleanup (lines 285-287, 213, 222, 230, 246), the cleanup function should defensively free any remaining response to guard against edge cases and future code changes.
Apply this fix:
collect_cleanup: - if (response) { response = NULL; } + if (response) { snmp_free_pdu(response); response = NULL; } if (ss) { snmp_close(ss); ss = NULL; } if (err) { flb_free(err); err = NULL; } if (buf) { flb_free(buf); buf = NULL; } if (oid_buf) { flb_free(oid_buf); oid_buf = NULL; }
6-6: Remove unused OpenSSL include.The
<openssl/crypto.h>include appears to be unused. It may be leftover from whenOPENSSL_cleanup()was called (now removed per previous review feedback). Consider removing it to keep includes clean.Apply this cleanup if the include is confirmed unused:
#include <net-snmp/net-snmp-config.h> #include <net-snmp/net-snmp-includes.h> -#include <openssl/crypto.h> #include <msgpack.h>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
CMakeLists.txt(1 hunks)cmake/FindNetsnmp.cmake(1 hunks)cmake/plugins_options.cmake(1 hunks)cmake/windows-setup.cmake(1 hunks)dockerfiles/Dockerfile(1 hunks)dockerfiles/Dockerfile.centos7(1 hunks)packaging/distros/amazonlinux/Dockerfile(4 hunks)packaging/distros/centos/Dockerfile(8 hunks)packaging/distros/debian/Dockerfile(8 hunks)packaging/distros/raspbian/Dockerfile(3 hunks)packaging/distros/ubuntu/Dockerfile(9 hunks)plugins/CMakeLists.txt(1 hunks)plugins/in_snmp/CMakeLists.txt(1 hunks)plugins/in_snmp/in_snmp.c(1 hunks)plugins/in_snmp/in_snmp.h(1 hunks)run_code_analysis.sh(1 hunks)tests/runtime/CMakeLists.txt(1 hunks)tests/runtime/in_snmp.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- dockerfiles/Dockerfile.centos7
- plugins/in_snmp/in_snmp.h
- packaging/distros/centos/Dockerfile
- plugins/in_snmp/CMakeLists.txt
- dockerfiles/Dockerfile
- tests/runtime/CMakeLists.txt
- packaging/distros/debian/Dockerfile
- CMakeLists.txt
- plugins/CMakeLists.txt
- cmake/FindNetsnmp.cmake
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
run_code_analysis.shcmake/plugins_options.cmakecmake/windows-setup.cmake
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
run_code_analysis.shcmake/plugins_options.cmakecmake/windows-setup.cmake
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
run_code_analysis.shcmake/plugins_options.cmake
📚 Learning: 2025-08-29T06:24:44.797Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:26-26
Timestamp: 2025-08-29T06:24:44.797Z
Learning: In Fluent Bit, ZSTD support is always available and enabled by default. The build system automatically detects and uses either the system libzstd library or builds the bundled ZSTD version. Unlike other optional dependencies like Arrow which use conditional compilation guards (e.g., FLB_HAVE_ARROW), ZSTD does not require conditional includes or build flags.
Applied to files:
run_code_analysis.sh
🧬 Code graph analysis (2)
plugins/in_snmp/in_snmp.c (4)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_log_event_encoder.c (6)
flb_log_event_encoder_begin_record(246-254)flb_log_event_encoder_set_current_timestamp(289-292)flb_log_event_encoder_commit_record(256-274)flb_log_event_encoder_reset(31-40)flb_log_event_encoder_init(42-74)flb_log_event_encoder_destroy(99-116)include/fluent-bit/flb_input.h (1)
flb_input_config_map_set(715-741)src/flb_input.c (4)
flb_input_set_context(1610-1613)flb_input_set_collector_time(1685-1704)flb_input_collector_pause(2035-2072)flb_input_collector_resume(2095-2151)
tests/runtime/in_snmp.c (2)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_lib.c (9)
flb_create(138-220)flb_input(261-271)flb_input_set(300-330)flb_output(274-284)flb_output_set(515-546)flb_start(914-925)flb_stop(942-985)flb_lib_free(693-700)flb_destroy(223-258)
🔇 Additional comments (15)
run_code_analysis.sh (1)
14-14: LGTM! Dependency addition is correct.Adding
libsnmp-devto the test environment dependencies is appropriate for building the new SNMP input plugin on Debian/Ubuntu-based systems.packaging/distros/raspbian/Dockerfile (1)
21-21: LGTM! Consistent SNMP dependency addition across Raspbian versions.The
libsnmp-devpackage is correctly added to all three Raspbian base images (buster, bullseye, bookworm) to support the new SNMP input plugin.Also applies to: 34-34, 47-47
packaging/distros/ubuntu/Dockerfile (1)
28-28: LGTM! Comprehensive SNMP dependency addition across all Ubuntu versions.The
libsnmp-devpackage is correctly and consistently added across all Ubuntu base images (16.04 through 24.04, including x86 and ARM variants) to enable SNMP plugin builds.Also applies to: 60-60, 94-94, 124-124, 150-150, 174-174, 200-200, 224-224, 250-250
cmake/windows-setup.cmake (1)
84-84: LGTM! Appropriate default for Windows platform.Disabling the SNMP input plugin by default on Windows aligns with the PR objectives, which note build failures on Windows. Users can still enable it explicitly if needed.
cmake/plugins_options.cmake (1)
52-52: LGTM! Plugin option correctly defined.The SNMP input plugin option follows the established pattern, is properly placed alphabetically, and is enabled by default (with Windows-specific override in windows-setup.cmake).
tests/runtime/in_snmp.c (3)
17-39: LGTM! Callback record accumulation is well-structured.The callback mechanism properly accumulates test records using realloc and handles allocation failures appropriately. Memory is managed correctly with proper size tracking.
41-67: LGTM! MsgPack parsing and validation are correct.The test validation functions properly:
- Reset offset (
off = 0) per record to avoid parsing errors- Include bounds checking to prevent out-of-bounds reads
- Validate expected OID/value pairs correctly
- Clean up resources appropriately
All previously flagged issues have been addressed.
Also applies to: 69-117
120-178: LGTM! Test harness is comprehensive.The test driver properly:
- Sets up test mode environment variables
- Initializes Fluent Bit context and plugin configuration
- Handles varargs for flexible configuration
- Waits for collection and validates results
- Cleans up all allocated resources including per-record data
plugins/in_snmp/in_snmp.c (7)
16-23: LGTM! Test mode detection is clean.The test mode check using an environment variable is straightforward and appropriate for runtime testing.
25-117: LGTM! Mock SNMP responses are well-implemented.The mock function properly:
- Frees the input PDU (line 33-35) to mirror snmp_synch_response semantics
- NULL-checks the environment variable (line 37)
- Constructs appropriate test responses for GET and WALK scenarios
- Handles errors with proper cleanup (lines 112-116)
142-315: LGTM! Collection logic is well-structured.The SNMP collection function properly handles:
- GET vs WALK mode selection (lines 142-150)
- Session opening with error handling (lines 152-158)
- OID parsing and validation (lines 160-165)
- Walk boundary setup (lines 167-171)
- Log event encoder initialization (lines 176-181)
- Per-iteration buffer allocation and cleanup (lines 211-253)
- Walk termination conditions (lines 201-206, 255-270)
- Error handling for timeouts and SNMP errors (lines 272-284)
- Record commit and append (lines 290-303)
The cleanup path at lines 307-312 is comprehensive, though it depends on the initialization fix noted in the previous comment.
318-418: LGTM! Initialization is comprehensive and correct.The initialization function properly:
- Allocates and configures the plugin context (lines 326-340)
- Sets up the collector with appropriate interval (lines 343-355)
- Initializes the log event encoder (lines 357-367)
- Configures Net-SNMP library defaults (lines 369-374)
- Initializes sockets and SNMP library (lines 376-377)
- Composes peername with host and port (lines 381-402) - addresses previous review concern
- Sets SNMP version and community (lines 404-415)
Note: The allocated peername at line 387 must be freed in the exit function (see comment on in_snmp_exit).
420-432: LGTM! Pause and resume functions are correctly implemented.The functions properly delegate to the collector pause/resume mechanisms, following the standard Fluent Bit plugin pattern.
450-493: LGTM! Configuration map is well-defined.All configuration options are properly structured with:
- Appropriate types (STR, INT)
- Sensible defaults (localhost:161, SNMP v2c, public community)
- Clear help text including the improved oid_type description (line 485)
- Correct struct offsets for config map processing
495-506: LGTM! Plugin registration is complete.The plugin structure properly registers all required callbacks and provides appropriate metadata for the SNMP input plugin.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
plugins/in_snmp/in_snmp.c (2)
307-307: Consider freeing response PDU defensively in cleanup block.Line 307 sets
responsetoNULLwithout callingsnmp_free_pdu(response). While your normal and error paths appear to free response before reaching cleanup, defensive cleanup should still free the PDU if it's somehow non-NULL.Apply this change:
collect_cleanup: - if (response) { response = NULL; } + if (response) { snmp_free_pdu(response); response = NULL; } if (ss) { snmp_close(ss); ss = NULL; }
380-401: Optional: Simplify peername buffer size calculation for clarity.The buffer size calculation is mathematically correct, but line 384 uses
strlen("udp::")with a comment"udp:" + ":"which doesn't clearly match the format string"udp:%s:%d"at line 392. Consider calculating the size more directly:- size_t static_len = strlen("udp::"); // "udp:" + ":" - size_t need = static_len + host_len + port_len + 1; + // Format "udp:%s:%d" needs: 4 ("udp:") + host_len + 1 (":") + port_len + 1 (null) + size_t need = 6 + host_len + port_len;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packaging/distros/amazonlinux/Dockerfile(4 hunks)plugins/in_snmp/in_snmp.c(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/in_snmp/in_snmp.c (4)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_log_event_encoder.c (6)
flb_log_event_encoder_begin_record(246-254)flb_log_event_encoder_set_current_timestamp(289-292)flb_log_event_encoder_commit_record(256-274)flb_log_event_encoder_reset(31-40)flb_log_event_encoder_init(42-74)flb_log_event_encoder_destroy(99-116)include/fluent-bit/flb_input.h (1)
flb_input_config_map_set(715-741)src/flb_input.c (4)
flb_input_set_context(1610-1613)flb_input_set_collector_time(1685-1704)flb_input_collector_pause(2035-2072)flb_input_collector_resume(2095-2151)
🔇 Additional comments (2)
packaging/distros/amazonlinux/Dockerfile (1)
26-26: LGTM! Package name correctly updated across all Amazon Linux variants.The change from
libsnmp-develtonet-snmp-develis correct for Amazon Linux 2 and Amazon Linux 2023. All four install blocks have been updated consistently.Also applies to: 51-51, 76-76, 103-103
plugins/in_snmp/in_snmp.c (1)
316-450: LGTM! Init, exit, lifecycle, and configuration properly implemented.The plugin initialization correctly sets up the SNMP session with the configured port (via composed peername), initializes the log encoder, and establishes timed collection. The exit function properly cleans up all allocated resources including the peername. Configuration map provides sensible defaults for all SNMP parameters.
Also applies to: 452-508
k402xxxcenxxx
force-pushed
the
feature/in_snmp
branch
3 times, most recently
from
d28aead to
8f5d8aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmake/FindNetsnmp.cmake (1)
53-59: Consider documenting why NETSNMPTRAPD is excluded.The
NETSNMPTRAPD_LIBRARYis found but commented out from the aggregated libraries. While this may be intentional (trapd is for SNMP trap handling, not typically needed by clients), a brief comment explaining the exclusion would help future maintainers.For example:
SET(NETSNMP_LIBRARIES ${NETSNMP_LIBRARY} ${NETSNMPAGENT_LIBRARY} ${NETSNMPHELPERS_LIBRARY} ${NETSNMPMIBS_LIBRARY} -# ${NETSNMPTRAPD_LIBRARY} + # ${NETSNMPTRAPD_LIBRARY} # Excluded: trapd is for SNMP trap daemon, not needed by client plugins )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
CMakeLists.txt(1 hunks)cmake/FindNetsnmp.cmake(1 hunks)cmake/plugins_options.cmake(1 hunks)cmake/windows-setup.cmake(1 hunks)dockerfiles/Dockerfile(1 hunks)dockerfiles/Dockerfile.centos7(1 hunks)packaging/distros/amazonlinux/Dockerfile(4 hunks)packaging/distros/centos/Dockerfile(8 hunks)packaging/distros/debian/Dockerfile(8 hunks)packaging/distros/raspbian/Dockerfile(3 hunks)packaging/distros/ubuntu/Dockerfile(9 hunks)plugins/CMakeLists.txt(1 hunks)plugins/in_snmp/CMakeLists.txt(1 hunks)plugins/in_snmp/in_snmp.c(1 hunks)plugins/in_snmp/in_snmp.h(1 hunks)run_code_analysis.sh(1 hunks)tests/runtime/CMakeLists.txt(1 hunks)tests/runtime/in_snmp.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/runtime/CMakeLists.txt
- packaging/distros/debian/Dockerfile
- CMakeLists.txt
- packaging/distros/centos/Dockerfile
- plugins/in_snmp/in_snmp.c
- plugins/in_snmp/CMakeLists.txt
- tests/runtime/in_snmp.c
- packaging/distros/ubuntu/Dockerfile
- plugins/CMakeLists.txt
- dockerfiles/Dockerfile
- dockerfiles/Dockerfile.centos7
- plugins/in_snmp/in_snmp.h
- cmake/plugins_options.cmake
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
cmake/windows-setup.cmakerun_code_analysis.sh
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
cmake/windows-setup.cmakerun_code_analysis.sh
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
run_code_analysis.sh
📚 Learning: 2025-08-29T06:24:44.797Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:26-26
Timestamp: 2025-08-29T06:24:44.797Z
Learning: In Fluent Bit, ZSTD support is always available and enabled by default. The build system automatically detects and uses either the system libzstd library or builds the bundled ZSTD version. Unlike other optional dependencies like Arrow which use conditional compilation guards (e.g., FLB_HAVE_ARROW), ZSTD does not require conditional includes or build flags.
Applied to files:
run_code_analysis.sh
🔇 Additional comments (7)
packaging/distros/raspbian/Dockerfile (1)
21-21: LGTM! Consistent SNMP dependency addition across Raspbian versions.The
libsnmp-devpackage is correctly added to all three Raspbian base images (buster, bullseye, bookworm), aligning with the SNMP input plugin support being added in this PR.Also applies to: 34-34, 47-47
run_code_analysis.sh (1)
14-14: LGTM! SNMP dependency added for code analysis.Adding
libsnmp-devto the test dependencies ensures the SNMP headers are available during code analysis and testing, consistent with the new SNMP plugin support.packaging/distros/amazonlinux/Dockerfile (1)
26-26: LGTM! Correct package name for Amazon Linux.The
net-snmp-develpackage is correct for RPM-based Amazon Linux distributions (AL2 and AL2023). The previous review's correction fromlibsnmp-develtonet-snmp-develhas been properly applied across all four base images.Also applies to: 51-51, 76-76, 103-103
cmake/FindNetsnmp.cmake (3)
16-21: LGTM! Correct header search paths.The module now correctly searches for
net-snmp/net-snmp-includes.h(the actual header used by the plugin) with appropriate search paths including macOS Homebrew and MacPorts directories.
23-51: LGTM! Library search paths aligned with include paths.All five library searches now include
/opt/homebrew/liband/opt/local/lib, ensuring consistency with the include path search and proper support for macOS Homebrew and MacPorts installations.
61-70: LGTM! Proper CMake convention support.The module correctly provides both singular
NETSNMP_INCLUDE_DIRand pluralNETSNMP_INCLUDE_DIRSforms, and usesFindPackageHandleStandardArgswith the plural form following CMake best practices.cmake/windows-setup.cmake (1)
84-84: LGTM! Appropriate to disable SNMP on Windows.Setting
FLB_IN_SNMPtoNofor Windows builds is consistent with the PR objectives, where the author reported build failures on Windows and chose not to support the platform initially. The placement and format are correct.
This commit introduces a new SNMP input plugin for Fluent Bit with the following key features: - Implement SNMP GET and WALK operations using NET-SNMP library - Add configuration options for timeout and retries - Include comprehensive unit tests for SNMP operations - Fix memory management and resource cleanup - Add NET-SNMP dependencies in various Dockerfiles - Configure CMake to properly find and link NET-SNMP - Improve error handling and validation The plugin allows monitoring of SNMP-enabled devices and supports both single OID queries and walking through SNMP trees. Signed-off-by: k402xxxcenxxx <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
plugins/in_snmp/in_snmp.h (1)
16-18: Ensure the configured port is applied to the SNMP session.As noted in a previous review, the
target_portfield must be applied to thenetsnmp_sessionduring initialization (inin_snmp.c). Verify that the session configuration properly incorporatesctx->target_portby either including it in thepeernamestring (e.g.,"host:port") or setting the session'sremote_portfield.#!/bin/bash # Verify that target_port is applied to the SNMP session rg -nA5 -B5 'target_port|remote_port|peername' plugins/in_snmp/in_snmp.c
🧹 Nitpick comments (2)
plugins/in_snmp/CMakeLists.txt (1)
1-2: Clarify the need for these variable assignments.These lines manually create plural variable names from singular ones (
NETSNMP_LIBRARY→NETSNMP_LIBRARIES,NETSNMP_INCLUDE_DIR→NETSNMP_INCLUDE_DIRS). Typically, CMake find modules provide both forms directly, or consumers use the canonical form consistently.Consider verifying whether
cmake/FindNetsnmp.cmakealready exportsNETSNMP_LIBRARIESandNETSNMP_INCLUDE_DIRS. If so, these assignments are redundant and can be removed.#!/bin/bash # Check what variables FindNetsnmp.cmake sets rg -n 'set\((NETSNMP_LIBRARIES|NETSNMP_INCLUDE_DIRS|NETSNMP_LIBRARY|NETSNMP_INCLUDE_DIR)' cmake/FindNetsnmp.cmaketests/runtime/in_snmp.c (1)
142-147: Add null check after flb_create().The test should verify that context creation succeeded before proceeding. Although test environments typically have sufficient resources, defensive checks improve test reliability.
Apply this diff:
/* initialize */ ctx = flb_create(); + TEST_CHECK(ctx != NULL); in_ffd = flb_input(ctx, "snmp", NULL);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
CMakeLists.txt(1 hunks)cmake/FindNetsnmp.cmake(1 hunks)cmake/plugins_options.cmake(1 hunks)cmake/windows-setup.cmake(1 hunks)dockerfiles/Dockerfile(1 hunks)dockerfiles/Dockerfile.centos7(1 hunks)packaging/distros/amazonlinux/Dockerfile(4 hunks)packaging/distros/centos/Dockerfile(8 hunks)packaging/distros/debian/Dockerfile(8 hunks)packaging/distros/raspbian/Dockerfile(3 hunks)packaging/distros/ubuntu/Dockerfile(9 hunks)plugins/CMakeLists.txt(1 hunks)plugins/in_snmp/CMakeLists.txt(1 hunks)plugins/in_snmp/in_snmp.c(1 hunks)plugins/in_snmp/in_snmp.h(1 hunks)run_code_analysis.sh(1 hunks)tests/runtime/CMakeLists.txt(1 hunks)tests/runtime/in_snmp.c(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- plugins/in_snmp/in_snmp.c
🚧 Files skipped from review as they are similar to previous changes (10)
- dockerfiles/Dockerfile.centos7
- run_code_analysis.sh
- tests/runtime/CMakeLists.txt
- packaging/distros/raspbian/Dockerfile
- cmake/plugins_options.cmake
- packaging/distros/amazonlinux/Dockerfile
- packaging/distros/centos/Dockerfile
- CMakeLists.txt
- dockerfiles/Dockerfile
- cmake/FindNetsnmp.cmake
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
plugins/in_snmp/CMakeLists.txtcmake/windows-setup.cmake
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
cmake/windows-setup.cmake
🧬 Code graph analysis (1)
tests/runtime/in_snmp.c (2)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_lib.c (9)
flb_create(138-220)flb_input(261-271)flb_input_set(300-330)flb_output(274-284)flb_output_set(515-546)flb_start(914-925)flb_stop(942-985)flb_lib_free(693-700)flb_destroy(223-258)
🔇 Additional comments (4)
packaging/distros/ubuntu/Dockerfile (1)
28-28: Consistent and correct SNMP dependency additions.The libsnmp-dev package is correctly and consistently added across all Ubuntu base images (16.04, 18.04, 20.04, 22.04, 24.04) and their respective arm64v8 variants. Package naming and placement in the apt-get install lists are appropriate for Debian-based systems.
Also applies to: 60-60, 94-94, 124-124, 150-150, 174-174, 200-200, 224-224, 250-250
packaging/distros/debian/Dockerfile (1)
31-31: Consistent and correct SNMP dependency additions.The libsnmp-dev package is correctly and consistently added across all Debian base images (buster, bullseye, bookworm, trixie) and their respective arm64v8 variants. Package naming and placement in the apt-get install lists are appropriate and match the Ubuntu Dockerfile changes.
Also applies to: 62-62, 86-86, 112-112, 136-136, 162-162, 186-186, 212-212
plugins/CMakeLists.txt (1)
284-284: LGTM! Plugin registration follows the standard pattern.The SNMP input plugin is correctly registered using the standard macro, consistent with all other input plugin registrations in this file.
cmake/windows-setup.cmake (1)
84-84: LGTM! Disabling SNMP on Windows is appropriate.Given the Windows build failures mentioned in the PR objectives and the author's indication that Windows support may not be included, explicitly disabling the SNMP plugin for Windows builds is the correct approach.
| FLB_PLUGIN(in_snmp "${src}" "") | ||
| target_link_libraries(flb-plugin-in_snmp ${NETSNMP_LIBRARIES}) No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
Verify that NETSNMP include directories are properly configured.
The plugin links against NETSNMP_LIBRARIES but doesn't explicitly call include_directories(${NETSNMP_INCLUDE_DIRS}) or target_include_directories(). While the includes may be propagated through other means, it's worth confirming the plugin can find the Net-SNMP headers during compilation.
Run the following to check if includes are set elsewhere or need to be added here:
🏁 Script executed:
#!/bin/bash
# Search for NETSNMP include directory configuration
rg -n 'include_directories.*NETSNMP|target_include_directories.*NETSNMP' --type=cmake
# Check if the plugin source includes net-snmp headers
rg -n '#include.*net-snmp' plugins/in_snmp/in_snmp.c plugins/in_snmp/in_snmp.hLength of output: 201
I need to investigate how NETSNMP_LIBRARIES is configured in the project to determine if it automatically propagates include directories.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find where NETSNMP is configured
rg -n 'find_package.*NETSNMP|FindNetSNMP|NETSNMP_INCLUDE_DIRS|NETSNMP_LIBRARIES' --type=cmake -B2 -A2
# Check for CMakeLists.txt in root and other levels
fd 'CMakeLists.txt' | head -20
Length of output: 2091
I found that NETSNMP_INCLUDE_DIRS is defined locally but never used. Let me check the complete CMakeLists.txt file for this plugin:
---
🏁 Script executed:
```shell
#!/bin/bash
# View the complete CMakeLists.txt for the snmp plugin
cat plugins/in_snmp/CMakeLists.txt
Length of output: 208
Let me check what the FLB_PLUGIN macro does to see if it handles include directories:
---
🏁 Script executed:
```shell
#!/bin/bash
# Find FLB_PLUGIN macro definition
rg -n 'macro.*FLB_PLUGIN|function.*FLB_PLUGIN' --type=cmake -A10
# Also check for include() or include_guard patterns that might auto-include
rg -n 'FLB_PLUGIN' cmake/ | head -5
Length of output: 748
Add target_include_directories() to configure Net-SNMP headers.
The variable NETSNMP_INCLUDE_DIRS is defined on line 2 but never used. The FLB_PLUGIN macro does not handle include directories—it only links libraries. Add the following after line 8:
target_include_directories(flb-plugin-in_snmp PRIVATE ${NETSNMP_INCLUDE_DIRS})🤖 Prompt for AI Agents
In plugins/in_snmp/CMakeLists.txt around lines 7 to 8, NETSNMP_INCLUDE_DIRS is
set but never applied; FLB_PLUGIN only links libraries so you must add the
Net-SNMP include dirs to the plugin target. After the existing
target_link_libraries line (line 8), add a call to target_include_directories
for flb-plugin-in_snmp with PRIVATE and ${NETSNMP_INCLUDE_DIRS} so the compiler
can find Net-SNMP headers.
| void flb_test_snmp_records_message_get(struct callback_records *records) | ||
| { | ||
| int i; | ||
| msgpack_unpacked result; | ||
| msgpack_object *obj; | ||
| size_t off = 0; | ||
| struct flb_time ftm; | ||
|
|
||
| TEST_CHECK(records->num_records > 0); | ||
| for (i = 0; i < records->num_records; i++) { | ||
| off = 0; | ||
| msgpack_unpacked_init(&result); | ||
|
|
||
| while (msgpack_unpack_next(&result, records->records[i].data, | ||
| records->records[i].size, &off) == MSGPACK_UNPACK_SUCCESS) { | ||
| flb_time_pop_from_msgpack(&ftm, &result, &obj); | ||
| TEST_CHECK(obj->type == MSGPACK_OBJECT_MAP); | ||
| TEST_CHECK(strncmp("iso.3.6.1.2.1.1.3.0", | ||
| obj->via.map.ptr[0].key.via.str.ptr, | ||
| obj->via.map.ptr[0].key.via.str.size) == 0); | ||
| TEST_CHECK(strncmp("123", | ||
| obj->via.map.ptr[0].val.via.str.ptr, | ||
| obj->via.map.ptr[0].val.via.str.size) == 0); | ||
| } | ||
| msgpack_unpacked_destroy(&result); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add map size validation before accessing elements.
The function accesses obj->via.map.ptr[0] at lines 59-60 and 62-63 without first verifying that obj->via.map.size >= 1. If the map is empty, this will cause undefined behavior.
Apply this diff to validate the map size:
while (msgpack_unpack_next(&result, records->records[i].data,
records->records[i].size, &off) == MSGPACK_UNPACK_SUCCESS) {
flb_time_pop_from_msgpack(&ftm, &result, &obj);
TEST_CHECK(obj->type == MSGPACK_OBJECT_MAP);
+ TEST_CHECK(obj->via.map.size >= 1);
TEST_CHECK(strncmp("iso.3.6.1.2.1.1.3.0",
obj->via.map.ptr[0].key.via.str.ptr,
obj->via.map.ptr[0].key.via.str.size) == 0);🤖 Prompt for AI Agents
In tests/runtime/in_snmp.c around lines 41 to 67, the code assumes
obj->via.map.ptr[0] exists without checking the map size; add a validation that
obj is a map and obj->via.map.size >= 1 before accessing ptr[0]. If the map size
is < 1, fail the test or skip that unpacked object (e.g., call TEST_CHECK(false)
or continue) to avoid undefined behavior; otherwise proceed to access
obj->via.map.ptr[0] as before.
8f5d8aa to
5c61d01
Compare
4 participants
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
There is one leak and can be fixed after this patch is release: net-snmp/net-snmp@4bd0d9a
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
fluent/fluent-bit-docs#1162
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
New Features
Build / Packaging
Tests