ctlplane: handle interface renaming

[rjarry]

When a grout interface is renamed, rename its control plane counterpart.

[coderabbitai]

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Adds a new netlink API netlink_link_set_name(ifindex, ifname) that sends RTM_SETLINK with IFLA_IFNAME to rename interfaces. cp_update() now, when iface->cp_id > 0, obtains the kernel name via if_indextoname() and calls netlink_link_set_name() if it differs from iface->name before performing ioctl operations. netlink_init() now ABORTs on NETLINK_ROUTE socket open failure.

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 736ea16 and 05d5f82.

📒 Files selected for processing (3)

• modules/infra/control/ctlplane.c (1 hunks)
• modules/infra/control/gr_netlink.h (1 hunks)
• modules/infra/control/netlink.c (1 hunks)

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 08ebd15 and 736ea16.

📒 Files selected for processing (3)

• modules/infra/control/ctlplane.c (1 hunks)
• modules/infra/control/gr_netlink.h (1 hunks)
• modules/infra/control/netlink.c (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 sets x = NULL. There is no risk of double free.
• ec_node_*() functions consume their ec_node arguments. No leaks on error.
• rte_node->ctx is an uint8_t array of size 16, not a pointer.
• Never 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.h
• modules/infra/control/ctlplane.c
• modules/infra/control/netlink.c

🧬 Code graph analysis (2)

modules/infra/control/gr_netlink.h (1)

modules/infra/control/netlink.c (1)

• netlink_link_set_name (331-348)

modules/infra/control/ctlplane.c (2)

modules/infra/control/gr_iface.h (1)

• iface (16-25)

modules/infra/control/netlink.c (1)

• netlink_link_set_name (331-348)

⏰ 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-18, none, debugoptimized, ubuntu-24.04, false)
• GitHub Check: build-and-tests (clang-15, none, debugoptimized, ubuntu-22.04, false)
• GitHub Check: build-and-tests (gcc-14, address, debug, ubuntu-24.04, -Dfrr=enabled, true)
• GitHub Check: build-and-tests (clang-16, none, debugoptimized, ubuntu-24.04, false)
• GitHub Check: build-and-tests (gcc-13, none, debugoptimized, ubuntu-24.04, -Dfrr=enabled, false)
• GitHub Check: rpm
• GitHub Check: deb

🔇 Additional comments (2)

modules/infra/control/gr_netlink.h (1)

12-12: LGTM!

The function declaration is consistent with the existing API style and correctly exposes the new interface renaming capability.

modules/infra/control/netlink.c (1)

331-348: LGTM!

The implementation correctly constructs a RTM_SETLINK netlink message with IFLA_IFNAME to rename the interface. The buffer sizing is appropriate, and the function follows the established pattern used by other netlink operations in this file.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Handle rename failure.

The return value from netlink_link_set_name is not checked. If the rename fails, the code continues silently, potentially causing a mismatch between iface->name and the actual kernel interface name. This could lead to subsequent ioctl operations failing or operating on the wrong interface.

Apply this diff to add error handling:

 	if (iface->cp_id > 0) {
 		if (if_indextoname(iface->cp_id, current_name) != NULL
 		    && strcmp(current_name, iface->name) != 0) {
-			netlink_link_set_name(iface->cp_id, iface->name);
+			if (netlink_link_set_name(iface->cp_id, iface->name) < 0) {
+				LOG(ERR, "netlink_link_set_name(%u, %s): %s",
+				    iface->cp_id, iface->name, strerror(errno));
+			}
 		}
 	}

🤖 Prompt for AI Agents

In modules/infra/control/ctlplane.c around lines 391-395, the call to
netlink_link_set_name lacks error handling; check its return value and if it
indicates failure, log the error (include errno/strerror(errno) or the
function's error string) and prevent further operations on the wrong interface —
for example, clear iface->cp_id or return an error from the enclosing function
so subsequent ioctl calls are not made against a mismatched kernel name. Ensure
the log message includes iface->cp_id and iface->name for context.

[christophefontaine]

Open question: should we allow interface renaming ?
Maybe that whole part of the code could be deleted instead ?

[rjarry]

Open question: should we allow interface renaming ? Maybe that whole part of the code could be deleted instead ?

Interface renaming was supported since the beginning. Why would it be a problem?

[christophefontaine]

I'm not saying it is a problem, but asking if we ever needed that feature once since the beginning.
I don't see any usecase where we would need this in the future.

[rjarry]

I'm not saying it is a problem, but asking if we ever needed that feature once since the beginning. I don't see any usecase where we would need this in the future.

Oh, I must admit I never questioned the need. I just implemented renaming since the beginning since it was so trivial to do. Before CP interfaces were a thing, renaming was just informative and just an update of the name field.