Implementation of IPFIX functionality for the OpenThread Border Router#3194
Implementation of IPFIX functionality for the OpenThread Border Router#3194vskgreg wants to merge 1 commit intoopenthread:mainfrom
Conversation
Signed-off-by: Gregory Voskertchian <voskgreg@gmail.com>
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @vskgreg, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the OpenThread Border Router by integrating IPFIX capabilities. This allows the Border Router to capture and export network flow information, providing valuable insights into network traffic patterns, performance, and security. The implementation includes a new, modular IPFIX exporter component, configurable via CMake, which leverages external libraries to efficiently manage and transmit flow data to an IPFIX collector. This feature is crucial for advanced network monitoring and analysis in Thread networks. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces IPFIX functionality for the OpenThread Border Router, a significant new feature. The implementation adds an IpfixExporter that leverages libfixbuf to capture and export flow data. The changes span across CMake build files, application lifecycle management, and the new ipfix module. While the feature implementation is comprehensive, the review has identified several critical issues concerning resource management (memory leaks, double-free risks), build configuration correctness, and error handling. There are also high-severity concerns regarding the hardcoded collector configuration and a brittle network interface detection mechanism. Additionally, several medium-severity issues related to C++ best practices, code style, and maintainability have been noted. Addressing these points will be crucial for ensuring the stability, correctness, and maintainability of this new functionality.
| return; | ||
| } | ||
|
|
||
| void IpfixExporter::Stop() |
There was a problem hiding this comment.
After freeing resources, the corresponding pointers (mBuf, mSession, mInfoModel) should be set to nullptr. In the current implementation, mSession and mInfoModel are not nulled out after being freed. This can lead to use-after-free or double-free bugs if Stop() is called more than once.
| otbr-host | ||
| otbr-common | ||
| otbr-utils | ||
| otbr-ipfix |
There was a problem hiding this comment.
The otbr-ipfix library should only be linked when the OTBR_IPFIX option is enabled. Without conditional linking, the build will fail when OTBR_IPFIX is OFF because the otbr-ipfix target will not be defined. Please use a generator expression to link it conditionally.
$<$<BOOL:${OTBR_IPFIX}>:otbr-ipfix>
| return (uint8_t)OT_IPFIX_INTERFACE_AIL_NETWORK; | ||
| } | ||
|
|
||
| void IpfixExporter::Start() |
There was a problem hiding this comment.
This function has resource leak issues.
-
Multiple
Start()calls: The checkVerifyOrExit(!mStarted)is at the end of the function. IfStart()is called a second time, it will re-allocate resources likemInfoModelandmSessionbefore exiting, causing a memory leak. The check formStartedshould be at the very beginning of the function. -
Early exit: If an allocation fails and the function returns early (e.g.,
if (!mExporter) { return; }), previously allocated resources are not freed. The function should have a proper cleanup path (e.g., usinggoto exitwith cleanup logic at theexitlabel) to prevent memory leaks.
| #define IPFIX_COLLECTOR_HOST "mycollectorhost" ///< IP address of the IPFIX collector (collecting process). | ||
| #define IPFIX_COLLECTOR_PORT "4739" ///< Destination port number of the IPFIX collector (standard UDP IPFIX port). |
There was a problem hiding this comment.
The IPFIX collector host and port are hardcoded. This limits the flexibility of the feature, as users cannot direct the exported data to their own collector without modifying the source code. These values should be made configurable, for example, through command-line arguments or a configuration file.
| mTemplateId = fbSessionAddTemplate(mSession, TRUE, FB_TID_AUTO, mTemplate, &err); | ||
| fbSessionAddTemplate(mSession, FALSE, mTemplateId, mTemplate, &err); |
There was a problem hiding this comment.
The GError is not being checked after these calls to fbSessionAddTemplate. If these functions fail, err will be set, but the code will continue execution with potentially invalid state (mTemplateId), which can lead to undefined behavior later. You should check the return value and the err object after each call that can fail.
| // ============================================================= | ||
|
|
||
| #if OTBR_ENABLE_IPFIX | ||
| mIpfixExporter = MakeUnique<IpfixExporter>(rcpHost); | ||
| #endif | ||
|
|
||
| // ======================================================== |
There was a problem hiding this comment.
These decorative comment blocks are inconsistent with the project's coding style. Please remove them to improve code consistency and readability.
| // ============================================================= | |
| #if OTBR_ENABLE_IPFIX | |
| mIpfixExporter = MakeUnique<IpfixExporter>(rcpHost); | |
| #endif | |
| // ======================================================== | |
| #if OTBR_ENABLE_IPFIX | |
| mIpfixExporter = MakeUnique<IpfixExporter>(rcpHost); | |
| #endif |
| # ==================================================================// | ||
|
|
||
| if (OTBR_IPFIX) | ||
| add_subdirectory(ipfix) | ||
| endif() | ||
|
|
||
| # ==================================================================// |
There was a problem hiding this comment.
These decorative comment blocks are inconsistent with the project's style and use a non-standard // for a CMake comment. To maintain a clean and consistent codebase, please remove these separators.
if (OTBR_IPFIX)
add_subdirectory(ipfix)
endif()
|
|
||
| } // namespace otbr | ||
|
|
||
| #endif // OTBR_ENABLE_IPFIX No newline at end of file |
There was a problem hiding this comment.
The file does not end with a newline character. It's a common convention to end files with a newline.
#endif // OTBR_ENABLE_IPFIX|
|
||
| #include <chrono> | ||
| #include <cstdint> | ||
| #include <stdint.h> |
There was a problem hiding this comment.
The C-style header <stdint.h> is included here. For C++ code, you should prefer the C++ version <cstdint>. Since <cstdint> is already included on the line above, this line is redundant and should be removed.
| # ==================================================================================================== | ||
| set(OT_IPFIX ${OTBR_IPFIX} CACHE STRING "enable IPFIX packet/frame capture and export" FORCE) | ||
| # ==================================================================================================== |
There was a problem hiding this comment.
The decorative comment blocks are not consistent with the style of this file. Please remove them to maintain code consistency. Also, there is trailing whitespace on line 57.
set(OT_IPFIX ${OTBR_IPFIX} CACHE STRING "enable IPFIX packet/frame capture and export" FORCE)
1 participant
This pull request contains the implementation of an IPFIX functionality for the OpenThread Border Router (part: ot-br-posix).
Link for the related openthread pull request : openthread/openthread#12310
This work is done in the context of a Master's Thesis at the University of Liège in Belgium.
Author : Voskertchian Gregory
Supervisor: @evyncke
Sat Jan 17 04:34:22 PM CET 2026