Cp prep #388
Cp prep #388
Conversation
📝 WalkthroughWalkthroughThe PR exposes Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
subprojects/packagefiles/frr/meson-add-dependency-definition.patch (2)
1-6: Patch date inconsistency.The patch header shows "Date: Thu, 7 Aug 2025" but this PR was created on November 7, 2025. This could cause confusion in the git history.
119-128: Installation during build phase is non-standard.The build target runs
make installduring the build phase (line 119), which goes against typical Meson practices where build and install are separate phases. This can cause issues with packaging systems and staged installations.Consider using Meson's
meson.add_install_script()or properly separating the install phase. At minimum, document why this approach was chosen.
🧹 Nitpick comments (2)
subprojects/packagefiles/frr/meson-add-dependency-definition.patch (2)
51-54: Fragile sed pattern - consider a more robust approach.This sed command uses nested escaping and complex regex patterns that are difficult to maintain and could fail silently. Consider checking if the log option already exists first, or using a more straightforward string replacement approach.
Example alternative:
-sed -i -e "/^zebra_options=/{ - /--log[[:space:]]*$(printf '%s' "file:$log_path" | sed 's/[\/&]/\\&/g')/! \ - s|\"$| --log file:$log_path\"| -}" "$conf_file" +if ! grep -q "zebra_options=.*--log file:$log_path" "$conf_file"; then + sed -i "s|^zebra_options=\"\(.*\)\"|zebra_options=\"\1 --log file:$log_path\"|" "$conf_file" +fi
69-69: Consider making the install prefix configurable.The prefix is hardcoded to
meson.global_build_root() / 'frr_install', which limits deployment flexibility. Standard Meson projects useget_option('prefix')to allow users to control the installation location.-prefix = meson.global_build_root() / 'frr_install' +prefix = get_option('prefix').startswith('/') ? get_option('prefix') : meson.global_build_root() / 'frr_install'
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
main/api.c(1 hunks)main/api.h(1 hunks)main/event.c(2 hunks)modules/infra/control/gr_netlink.h(1 hunks)modules/infra/control/netlink.c(2 hunks)subprojects/packagefiles/frr/meson-add-dependency-definition.patch(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
main/api.hmodules/infra/control/gr_netlink.hmodules/infra/control/netlink.cmain/event.cmain/api.c
🧬 Code graph analysis (4)
main/api.h (1)
main/api.c (1)
api_send_notifications(86-128)
modules/infra/control/gr_netlink.h (1)
modules/infra/control/netlink.c (1)
netlink_set_ifalias(298-320)
modules/infra/control/netlink.c (1)
api/gr_errno.h (1)
errno_set(9-12)
main/event.c (1)
main/api.c (1)
api_send_notifications(86-128)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
- GitHub Check: build-and-tests (clang-18, none, debugoptimized, ubuntu-24.04, false)
- GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
- GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
- GitHub Check: rpm
- GitHub Check: deb
🔇 Additional comments (8)
subprojects/packagefiles/frr/meson-add-dependency-definition.patch (2)
87-89: User/group detection may not work in all build environments.Running
id -unandid -gnat configure time assumes the build user is the runtime user. This can break in cross-compilation, containerized builds, or CI/CD environments where the build and runtime environments differ.Consider making these configurable via Meson options with sensible defaults (e.g., 'frr' user/group).
107-111: Verify the enablement of ospfd, ospf6d, isisd, and bfdd daemons.According to the PR summary, the disable flags for ospfd, ospf6d, isisd, and bfdd were removed, effectively enabling these routing protocol daemons. Enabling additional daemons increases build time, binary size, and potential attack surface.
Confirm this change is intentional and document why these specific routing protocols are needed for the project. Consider whether all four daemons are required or if a subset would suffice.
main/api.h (1)
10-10: LGTM!Clean API declaration. The function was previously static in api.c and is now properly exported for use by event.c.
modules/infra/control/gr_netlink.h (1)
21-21: LGTM!Declaration follows the established pattern for netlink control APIs.
modules/infra/control/netlink.c (1)
17-18: LGTM!The macro definition and comment are clear and helpful.
main/event.c (2)
4-5: LGTM!Proper include to expose the new API function.
31-31: LGTM!Clean integration. The call is properly placed after internal subscribers are notified, making the dependency on the API notification system explicit.
main/api.c (1)
86-86: LGTM!Good refactoring. Exposing this function and calling it directly from
event.cmakes the dependency explicit rather than relying on the subscription mechanism.
| int netlink_set_ifalias(const char *ifname, const char *ifalias) { | ||
| char buf[NLMSG_SPACE(sizeof(struct ifinfomsg) + NLA_SPACE(IFALIASZ))]; | ||
| struct ifinfomsg *ifm; | ||
| struct nlmsghdr *nlh; | ||
| int ifindex; | ||
|
|
||
| ifindex = if_nametoindex(ifname); | ||
| if (!ifindex) | ||
| return errno_set(ENODEV); | ||
|
|
||
| memset(buf, 0, sizeof(buf)); | ||
| nlh = mnl_nlmsg_put_header(buf); | ||
| nlh->nlmsg_type = RTM_NEWLINK; | ||
| nlh->nlmsg_flags = NLM_F_REQUEST | NLM_F_ACK; | ||
|
|
||
| ifm = mnl_nlmsg_put_extra_header(nlh, sizeof(*ifm)); | ||
| ifm->ifi_family = AF_UNSPEC; | ||
| ifm->ifi_index = ifindex; | ||
|
|
||
| mnl_attr_put_strz(nlh, IFLA_IFALIAS, ifalias); | ||
|
|
||
| return netlink_send_req(nlh); | ||
| } |
There was a problem hiding this comment.
Validate ifalias length before constructing the message.
The function doesn't check if the ifalias string length fits within IFALIASZ (256 bytes) before calling mnl_attr_put_strz. If the string is too long, this could cause buffer overflow or silent truncation.
Add validation before constructing the message:
int netlink_set_ifalias(const char *ifname, const char *ifalias) {
char buf[NLMSG_SPACE(sizeof(struct ifinfomsg) + NLA_SPACE(IFALIASZ))];
struct ifinfomsg *ifm;
struct nlmsghdr *nlh;
int ifindex;
+ if (strlen(ifalias) >= IFALIASZ)
+ return errno_set(EINVAL);
+
ifindex = if_nametoindex(ifname);
if (!ifindex)
return errno_set(ENODEV);🤖 Prompt for AI Agents
In modules/infra/control/netlink.c around lines 298 to 320, validate the ifalias
length before building the netlink message: check for NULL ifalias and use
strnlen(ifalias, IFALIASZ) (or equivalent) and if the length is >= IFALIASZ
return an error (e.g. return errno_set(EINVAL)) so you don't call
mnl_attr_put_strz with a too-long string; perform this check before memset(buf,
...) and constructing the nlmsg.
Enable ISISd, OSPFd, OSPF6d and BFDd. Signed-off-by: Christophe Fontaine <[email protected]> Reviewed-by: Robin Jarry <[email protected]>
Add a new netlink helper to configure the ifalias (aka description). Signed-off-by: Christophe Fontaine <[email protected]> Reviewed-by: Robin Jarry <[email protected]>
We don't have a way to order the event subscription. Yet, some internal events handlers must be processed before other applications. Ensure the internal handlers are called before the apps connected on the grout socket. Signed-off-by: Christophe Fontaine <[email protected]> Reviewed-by: Robin Jarry <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
subprojects/packagefiles/frr/meson-add-dependency-definition.patch (1)
87-89: Build non-reproducibility: user/group hardcoded at configure time.Running
id -unandid -gnat configure time embeds the current user into the build, making it non-reproducible across different users or build environments. Consider using a fixed user/group (e.g., 'frr'/'frr' or 'nobody'/'nogroup') or exposing these as meson options.Apply this diff to use meson options:
+option('frr_user', type: 'string', value: 'frr', description: 'FRR user') +option('frr_group', type: 'string', value: 'frr', description: 'FRR group')Then in meson.build:
-user_name = run_command('id', '-un', check: true).stdout().strip() -group_name = run_command('id', '-gn', check: true).stdout().strip() -extra_configure_option = '--enable-user=' + user_name + ' ' + '--enable-group=' + group_name +user_name = get_option('frr_user') +group_name = get_option('frr_group') +extra_configure_option = '--enable-user=' + user_name + ' --enable-group=' + group_name
🧹 Nitpick comments (2)
subprojects/packagefiles/frr/meson-add-dependency-definition.patch (2)
51-54: Simplify the sed command for maintainability.This nested sed/printf/escaping is hard to read and maintain. Consider using a simpler approach like
grep -qto check if the log option exists, then append if missing, or use a more straightforward text processing tool.Example alternative:
-sed -i -e "/^zebra_options=/{ - /--log[[:space:]]*$(printf '%s' "file:$log_path" | sed 's/[\/&]/\\&/g')/! \ - s|\"$| --log file:$log_path\"| -}" "$conf_file" +if ! grep -q "file:$log_path" "$conf_file"; then + sed -i "/^zebra_options=/ s|\"$| --log file:$log_path\"|" "$conf_file" +fi
119-119: Remove '-x' flag from shell invocation.The
sh -xflag echoes all commands to stderr, creating verbose build logs and potentially exposing internal paths. Useshorsh -einstead.Apply this diff:
-install_cmd = 'make install && sh -x ' + srcdir + '/frr_install.sh ' + srcdir + ' ' + prefix + ' && ' +install_cmd = 'make install && sh ' + srcdir + '/frr_install.sh ' + srcdir + ' ' + prefix + ' && '
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
main/api.c(1 hunks)main/api.h(1 hunks)main/event.c(2 hunks)modules/infra/control/gr_netlink.h(1 hunks)modules/infra/control/netlink.c(2 hunks)subprojects/packagefiles/frr/meson-add-dependency-definition.patch(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- main/api.h
- main/event.c
- modules/infra/control/netlink.c
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{c,h}
⚙️ CodeRabbit configuration file
**/*.{c,h}: -gr_vec_*()functions cannot fail. No need to check their return value.
gr_vec_free(x)always setsx = NULL. There is no risk of double free.ec_node_*()functions consume theirec_nodearguments. No leaks on error.rte_node->ctxis an uint8_t array of size 16, not a pointer.- Don't suggest to replace
assert()with graceful error checking.- We compile with
-std=gnu2x. Unnamed parameters in function definitions are valid.
Files:
modules/infra/control/gr_netlink.hmain/api.c
🧬 Code graph analysis (1)
modules/infra/control/gr_netlink.h (1)
modules/infra/control/netlink.c (1)
netlink_set_ifalias(298-320)
🔇 Additional comments (4)
main/api.c (1)
86-128: Integration verified. No issues found.The static subscription has been removed, the function is properly exported with a public declaration in
api.h, and the call fromgr_event_pushoccurs at the correct point in the event flow. No duplicate notifications.subprojects/packagefiles/frr/meson-add-dependency-definition.patch (3)
69-69: Verify prefix location is intentional.Using
meson.global_build_root()installs FRR into the build tree rather than a standard prefix. If this is intentional for bundling FRR as a subproject dependency, consider adding a comment explaining this choice. Otherwise, useget_option('prefix')or a proper installation directory.
107-111: Configure flags look reasonable.The reordering and selection of disabled components (ripd, ripngd, pimd, pim6d, pbrd, fabricd, etc.) appears intentional for your use case. The flags are correctly formatted.
140-140: Verifysources: [build]usage is correct.Using a custom_target output as a source in
declare_dependencyis unusual. Typically, you'd use this for generated source files, not for entire build artifacts. Verify this achieves the intended dependency tracking - you may wantdependencies:or another approach instead.
| int netlink_add_addr6(const char *ifname, const struct rte_ipv6_addr *ip); | ||
| int netlink_del_addr6(const char *ifname, const struct rte_ipv6_addr *ip); | ||
| int netlink_set_addr_gen_mode_none(const char *ifname); | ||
| int netlink_set_ifalias(const char *ifname, const char *ifalias); |
There was a problem hiding this comment.
Buffer overflow risk in implementation (netlink.c)
The declaration follows existing patterns. However, the implementation in netlink.c:297-319 lacks validation that ifalias doesn't exceed IFALIASZ-1 bytes. Since mnl_attr_put_strz (line 319) will copy the full string into a fixed-size buffer, an oversized input can overflow the message buffer.
Apply this fix in netlink.c after line 302:
int netlink_set_ifalias(const char *ifname, const char *ifalias) {
char buf[NLMSG_SPACE(sizeof(struct ifinfomsg) + NLA_SPACE(IFALIASZ))];
struct ifinfomsg *ifm;
struct nlmsghdr *nlh;
int ifindex;
+ if (strlen(ifalias) >= IFALIASZ)
+ return errno_set(EINVAL);
+
ifindex = if_nametoindex(ifname);Committable suggestion skipped: line range outside the PR's diff.
2 participants
Summary by CodeRabbit
New Features
Refactor
Chores