Skip to content

RDKB-63098: Handle IPv6 delegation for business vs residential Partner ID as part of single build#215

Open
rirfha948 wants to merge 18 commits intodevelopfrom
topic/onestack
Open

RDKB-63098: Handle IPv6 delegation for business vs residential Partner ID as part of single build#215
rirfha948 wants to merge 18 commits intodevelopfrom
topic/onestack

Conversation

@rirfha948
Copy link
Contributor

No description provided.

rirfha948 and others added 4 commits February 6, 2026 04:54
Reason for change: Added One Stack MACRO handling
Test Procedure:
  - TBD
Risks: None
Priority: P3
Signed-off-by: rirfha948 <rasina_irfhan@comcast.com>
Reason for change: Added One Stack MACRO handling
Test Procedure:
  - TBD
Risks: None
Priority: P3
Signed-off-by: rirfha948 <rasina_irfhan@comcast.com>
Reason for change: Added One Stack MACRO handling
Test Procedure:
  - TBD
Risks: None
Priority: P3
Signed-off-by: rirfha948 <rasina_irfhan@comcast.com>
@rirfha948 rirfha948 marked this pull request as ready for review February 12, 2026 05:28
@rirfha948 rirfha948 requested review from a team as code owners February 12, 2026 05:28
Copilot AI review requested due to automatic review settings February 12, 2026 05:28
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This pull request implements runtime feature detection for IPv6 prefix delegation to enable a single build that supports both business (with IPv6 delegation) and residential (without IPv6 delegation) Partner IDs. The changes introduce the _ONESTACK_PRODUCT_REQ_ build flag along with runtime checks using isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION) to dynamically select the appropriate code path based on the current operating mode.

Changes:

  • Added runtime feature detection for IPv6 delegation using isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION) within _ONESTACK_PRODUCT_REQ_ builds
  • Wrapped existing conditional compilation blocks (based on CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) with runtime checks to support dynamic behavior selection
  • Updated preprocessor directives across multiple files to include _ONESTACK_PRODUCT_REQ_ alongside existing feature flags

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
source/service_routed/service_routed.c Adds runtime checks for IPv6 delegation in route configuration, interface management, and zebra configuration generation; includes scope fix for last_broadcasted_prefix variable
source/service_ipv6/service_ipv6.c Adds runtime checks for IPv6 delegation in DHCPv6 pool configuration and active LAN interface detection
source/scripts/init/c_registration/20_routing.c Updates preprocessor directive to include _ONESTACK_PRODUCT_REQ_ for routing service event registration
source/scripts/init/c_registration/15_dhcpv6_server.c Updates preprocessor directive to include _ONESTACK_PRODUCT_REQ_ for DHCPv6 server event registration
source/scripts/init/c_registration/15_dhcpv6_client.c Updates preprocessor directive to include _ONESTACK_PRODUCT_REQ_ for DHCPv6 client event registration
source/firewall/firewall_ipv6.c Adds runtime checks for IPv6 delegation in firewall rules; changes prepare_ipv6_multinet from static to non-static visibility
source/firewall/firewall.h Updates function declaration for prepare_ipv6_multinet to match implementation changes

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

#ifndef CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION
#if !defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)
#ifdef _ONESTACK_PRODUCT_REQ_
if (!isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conditional statement on this line lacks proper indentation. It should be indented to match the pattern used in other similar conditional blocks in this file (see lines 430, 459, 518, 675, 1027, 1037, etc.). The missing indentation makes the code structure less readable and inconsistent with the rest of the file.

Copilot uses AI. Check for mistakes.
Comment on lines +516 to +520
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(MULTILAN_FEATURE) || defined(_ONESTACK_PRODUCT_REQ_)
#if defined(_ONESTACK_PRODUCT_REQ_)
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
#endif
{
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The runtime feature check only evaluates FEATURE_IPV6_DELEGATION, but the preprocessor condition at line 516 also includes MULTILAN_FEATURE. This creates an inconsistency: if _ONESTACK_PRODUCT_REQ_ is defined alongside MULTILAN_FEATURE, and IPv6 delegation is disabled at runtime, this code block will not execute even though multi-LAN functionality may still be needed. Consider whether the runtime check should also account for multi-LAN requirements, or if the logic should be restructured to handle MULTILAN and PD features independently.

Copilot uses AI. Check for mistakes.
#endif
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(MULTILAN_FEATURE)
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(MULTILAN_FEATURE) || defined(_ONESTACK_PRODUCT_REQ_)
#ifdef _ONESTACK_PRODUCT_REQ_
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The runtime feature check only evaluates FEATURE_IPV6_DELEGATION, but the preprocessor condition at line 673 also includes MULTILAN_FEATURE. This creates the same inconsistency as seen in route_set: if _ONESTACK_PRODUCT_REQ_ is defined alongside MULTILAN_FEATURE, and IPv6 delegation is disabled at runtime, this cleanup code will not execute even though multi-LAN functionality may still require cleanup. The logic should be consistent with route_set and may need to handle MULTILAN independently of PD features.

Suggested change
#ifdef _ONESTACK_PRODUCT_REQ_
#if defined(_ONESTACK_PRODUCT_REQ_) && !defined(MULTILAN_FEATURE)

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings February 12, 2026 06:43
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +428 to +432
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)
#if defined(_ONESTACK_PRODUCT_REQ_)
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
#endif
{
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When _ONESTACK_PRODUCT_REQ_ is enabled, this file calls isFeatureSupportedInCurrentMode() and references FEATURE_IPV6_DELEGATION, but it does not include the feature-gate header. This will fail to compile in C99+ due to missing declarations/macros. Add the appropriate #include <rdkb_feature_mode_gate.h> under the same _ONESTACK_PRODUCT_REQ_ guard (and ensure the service_routed binary links against the feature-gate library).

Copilot uses AI. Check for mistakes.

if (pd_enabled || multilan_enabled)
{
get_active_lanif(sefd, setok, l2_insts, &enabled_iface_num);
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In an _ONESTACK_PRODUCT_REQ_ build where MULTILAN_FEATURE is not defined, multilan_enabled stays 0 and pd_enabled can be 0 when IPv6 delegation is disabled. In that case this if (pd_enabled || multilan_enabled) block will be skipped, but it currently encloses the main per-interface zebra config generation (the block only closes much later), resulting in zebra.conf being written with just the base header and no interface RA configuration. Restructure so that the normal single-LAN zebra.conf generation still runs when PD is disabled; only the PD/multilan enumeration logic should be conditional.

Suggested change
get_active_lanif(sefd, setok, l2_insts, &enabled_iface_num);
get_active_lanif(sefd, setok, l2_insts, &enabled_iface_num);
}

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings February 12, 2026 07:57
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +677 to +679
#ifdef _ONESTACK_PRODUCT_REQ_
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
#endif
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same gating issue as route_set(): in ONESTACK builds this cleanup logic only runs when FEATURE_IPV6_DELEGATION is enabled. If MULTILAN_FEATURE is enabled but the feature gate is false, per-LAN IPv6 rules may be left behind on stop/restart. Consider conditioning this on multilan enabled OR PD enabled, not only on the PD feature gate.

Suggested change
#ifdef _ONESTACK_PRODUCT_REQ_
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
#endif
#ifdef _ONESTACK_PRODUCT_REQ_
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) && !defined(MULTILAN_FEATURE)
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
#endif
#endif

Copilot uses AI. Check for mistakes.
syscfg_get(NULL, "ra_interval", ra_interval, sizeof(ra_interval));
#ifdef CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION

#ifdef WAN_FAILOVER_SUPPORTED
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last_broadcasted_prefix is now declared unconditionally under WAN_FAILOVER_SUPPORTED, but in CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION builds (without ONESTACK_PRODUCT_REQ) the code paths that reference it are compiled out. This can introduce an unused-variable warning (and could fail builds that use -Werror). Consider moving the declaration into the non-PD branch where it’s used, or wrapping the declaration with the same preprocessor condition as its usages.

Suggested change
#ifdef WAN_FAILOVER_SUPPORTED
#if defined(WAN_FAILOVER_SUPPORTED) && (!defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_))

Copilot uses AI. Check for mistakes.
Comment on lines +1719 to +1727
int prepare_ipv6_multinet(FILE *fp)
{
#ifdef _ONESTACK_PRODUCT_REQ_
if (!isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
{
/* PD feature disabled */
return 0;
}
#endif
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Within prepare_ipv6_multinet(), the code later does a do/while loop that formats with the first token from strtok(active_insts, " "). If the sysevent value "multinet-instances" is empty, strtok returns NULL and that loop will dereference a NULL pointer. Add an early return when the first token is NULL (or switch to a while(token != NULL) loop).

Copilot uses AI. Check for mistakes.
Comment on lines +520 to +522
#if defined(_ONESTACK_PRODUCT_REQ_)
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
#endif
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new ONESTACK_PRODUCT_REQ runtime gating (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION)) changes whether route rules are applied, but the existing gtest target for service_routed doesn’t compile with ONESTACK_PRODUCT_REQ and there are no unit tests covering the ONESTACK enabled/disabled branches. Add tests for both feature states to prevent regressions (including the MULTILAN_FEATURE interaction).

Copilot uses AI. Check for mistakes.
Comment on lines +397 to +411
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)
#if defined(_ONESTACK_PRODUCT_REQ_)
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION) == true)
#endif
{
DHCPV6S_SYSCFG_GETS(DHCPV6S_NAME, "pool", cfg->index, "", 0, "IAInterface", iface_name);
}
#endif
#if !defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)
#if defined(_ONESTACK_PRODUCT_REQ_)
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION) == false)
#endif
{
DHCPV6S_SYSCFG_GETS(DHCPV6S_NAME, "pool", cfg->index, "", 0, "Interface", iface_name);
}
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ONESTACK-specific behavior is now introduced in get_dhcpv6s_pool_cfg() to choose between IAInterface vs Interface based on FEATURE_IPV6_DELEGATION, but the existing service_ipv6 unit tests don’t build with ONESTACK_PRODUCT_REQ and don’t validate either branch. Add unit tests that exercise both feature states (mocking isFeatureSupportedInCurrentMode) to ensure the correct syscfg key is read.

Copilot uses AI. Check for mistakes.
Comment on lines 22 to 25
AM_LDFLAGS = -lccsp_common -lsecure_wrapper



Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing whitespace and multiple blank lines were introduced around AM_LDFLAGS. Please trim whitespace and avoid extra blank lines to keep Makefile diffs clean and consistent.

Suggested change
AM_LDFLAGS = -lccsp_common -lsecure_wrapper
AM_LDFLAGS = -lccsp_common -lsecure_wrapper

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings February 12, 2026 08:55
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

source/scripts/init/c_registration/20_routing.c:55

  • With _ONESTACK_PRODUCT_REQ_ enabled, this switches the routed service’s custom event list to the prefix-delegation variant (includes dhcpv6_option_changed and omits ipv6_prefix|ipv6_nameserver). The default handler script in this repo (source/scripts/init/service.d/service_routed.sh) doesn’t handle dhcpv6_option_changed, and it uses ipv6_prefix|ipv6_nameserver to trigger radv-restart, so ONESTACK builds may miss RA/zebra refresh triggers. Consider registering a superset of events for ONESTACK (include both sets), or ensure the handler invoked in ONESTACK images supports dhcpv6_option_changed.
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)
const char* SERVICE_CUSTOM_EVENTS[] = { 
                                        "wan-status|/etc/utopia/service.d/service_routed.sh",
                                        "lan-status|/etc/utopia/service.d/service_routed.sh",
                                        "dhcpv6_option_changed|/etc/utopia/service.d/service_routed.sh|NULL|"TUPLE_FLAG_EVENT,
                                        "ripd-restart|/etc/utopia/service.d/service_routed.sh|NULL|"TUPLE_FLAG_EVENT,
                                        "zebra-restart|/etc/utopia/service.d/service_routed.sh|NULL|"TUPLE_FLAG_EVENT,
                                        "staticroute-restart|/etc/utopia/service.d/service_routed.sh|NULL|"TUPLE_FLAG_EVENT,
                                        NULL

source/scripts/init/c_registration/15_dhcpv6_server.c:54

  • Including _ONESTACK_PRODUCT_REQ_ in this branch changes the dhcpv6_server registration to only listen for dhcpv6_option_changed. The handler referenced here (/etc/utopia/service.d/service_dhcpv6_server.sh, as shipped in this repo) does not handle that event, and instead restarts on lan-status|ipv6_nameserver|.... For ONESTACK builds where PD is disabled at runtime, this can prevent dhcpv6 server reconfig/restart on relevant changes. Consider using a combined event list for ONESTACK or aligning the handler script for ONESTACK images.
#elif defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)
const char* SERVICE_CUSTOM_EVENTS[] = { 
                                        "dhcpv6_option_changed|/etc/utopia/service.d/service_dhcpv6_server.sh|NULL|"TUPLE_FLAG_EVENT,
                                        NULL
                                      };

source/scripts/init/c_registration/15_dhcpv6_client.c:66

  • Including _ONESTACK_PRODUCT_REQ_ in this branch changes the dhcpv6_client registration to erouter_mode-updated|phylink_wan_state|.... The handler referenced here (/etc/utopia/service.d/service_dhcpv6_client.sh, as shipped in this repo) does not handle these events (it restarts on lan-status|wan-status|ipv6-status|...), so ONESTACK builds may not restart the client when expected. Consider registering the union of events for ONESTACK or ensuring the ONESTACK image uses a handler implementation that supports these events.
#elif defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)
const char* SERVICE_CUSTOM_EVENTS[] = {
                                        "erouter_mode-updated|/etc/utopia/service.d/service_dhcpv6_client.sh",
                                        "phylink_wan_state|/etc/utopia/service.d/service_dhcpv6_client.sh",
                                        "current_wan_ifname|/etc/utopia/service.d/service_dhcpv6_client.sh",
                                        "bridge_mode|/etc/utopia/service.d/service_dhcpv6_client.sh",
                                        NULL

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +676 to +680
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(MULTILAN_FEATURE) || defined(_ONESTACK_PRODUCT_REQ_)
#ifdef _ONESTACK_PRODUCT_REQ_
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
#endif
{
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In _ONESTACK_PRODUCT_REQ_ builds where FEATURE_IPV6_DELEGATION is disabled at runtime, this #if ... || defined(_ONESTACK_PRODUCT_REQ_) block compiles in and the #elif !defined(WAN_MANAGER_UNIFICATION_ENABLED) legacy cleanup path is compiled out, so route_unset() will skip removing the default brlan0 IPv6 rule/route that route_set() may have added. Consider compiling both code paths for ONESTACK and selecting between them at runtime (similar to get_active_lanif() / gen_zebra_conf()), or refactor the legacy cleanup into a helper that can be called when the feature gate is false.

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings February 12, 2026 10:09
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1169 to 1188
#if defined(MULTILAN_FEATURE) || defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined (_ONESTACK_PRODUCT_REQ_)
int multilan_enabled = 0;
int pd_enabled = 0;

#if defined(MULTILAN_FEATURE)
multilan_enabled = 1;
#endif
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION)
pd_enabled = 1;
#endif

#ifdef _ONESTACK_PRODUCT_REQ_
pd_enabled = isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION);
#endif

if (pd_enabled || multilan_enabled)
{
get_active_lanif(sefd, setok, l2_insts, &enabled_iface_num);
for (i = 0; i < enabled_iface_num; i++)
{
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new #if defined(MULTILAN_FEATURE) || defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_) block starts an if (pd_enabled || multilan_enabled) / for (...) section, but later in this function there is an #endif that closes this preprocessor guard in the middle of that loop. With the new structure, that #endif will prematurely terminate the conditional compilation and can leave the closing braces under a different #if, causing build failures. Please re-audit the matching #if/#endif placement so the guard spans exactly the intended braces.

Copilot uses AI. Check for mistakes.
Comment on lines +1719 to 1730
int prepare_ipv6_multinet(FILE *fp)
{
#ifdef _ONESTACK_PRODUCT_REQ_
if (!isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
{
/* PD feature disabled */
return 0;
}
#endif
char active_insts[32] = {0};
char lan_pd_if[128] = {0};
char *p = NULL;
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In prepare_ipv6_multinet(), after p = strtok(active_insts, " ") the code uses a do { ... } while (...) loop further down. If active_insts is empty, p will be NULL and the first iteration will still execute, leading to undefined behavior when formatting %s with p. Add a NULL check before entering the loop or convert to while (p != NULL) so an empty instance list is handled safely.

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings February 12, 2026 12:01
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

$(top_builddir)/source/utctx/lib/libutctx.la \
$(top_builddir)/source/ulog/libulog.la \
-ltelemetry_msgsender
service_routed_LDFLAGS = -L${PKG_CONFIG_SYSROOT_DIR}$(libdir) -ldevicemode -lonestack_syscfg -lonestack_log -lrdkb_feature_mode_gate
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

service_routed_LDFLAGS unconditionally links OneStack-specific libraries (-ldevicemode/-lonestack_* /-lrdkb_feature_mode_gate). This is inconsistent with other components (e.g., service_ipv6/firewall) that guard these libs behind the ONESTACK_PRODUCT_REQ automake conditional, and will break non-OneStack builds where these libs aren’t available. Gate these flags behind if ONESTACK_PRODUCT_REQ (and keep -lnet appended separately as needed).

Suggested change
service_routed_LDFLAGS = -L${PKG_CONFIG_SYSROOT_DIR}$(libdir) -ldevicemode -lonestack_syscfg -lonestack_log -lrdkb_feature_mode_gate
service_routed_LDFLAGS = -L${PKG_CONFIG_SYSROOT_DIR}$(libdir)
if ONESTACK_PRODUCT_REQ
service_routed_LDFLAGS += -ldevicemode -lonestack_syscfg -lonestack_log -lrdkb_feature_mode_gate
endif

Copilot uses AI. Check for mistakes.
Comment on lines +1025 to +1027
#ifdef WAN_FAILOVER_SUPPORTED
char last_broadcasted_prefix[64] = {0};
#endif
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last_broadcasted_prefix is declared under #ifdef WAN_FAILOVER_SUPPORTED before the PD/non-PD compile-time split, but it is only referenced inside the non-PD branch (#if !defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) ...). In builds where WAN_FAILOVER_SUPPORTED and CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION are enabled (and _ONESTACK_PRODUCT_REQ_ is not), this becomes an unused local and can fail builds that treat warnings as errors. Move the declaration into the same preprocessor block(s) where it is used, or otherwise ensure it’s referenced in all compilation variants where it’s declared.

Copilot uses AI. Check for mistakes.
Comment on lines 1151 to 1156
sysevent_get(sefd, setok, "ipv6_prefix_prdtime", preferred_lft, sizeof(preferred_lft));
sysevent_get(sefd, setok, "ipv6_prefix_vldtime", valid_lft, sizeof(valid_lft));
#endif
syscfg_get(NULL, "lan_ifname", lan_if, sizeof(lan_if));
}
#endif
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

syscfg_get("lan_ifname", ...) is now inside the #if !defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_) block. For CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION-only builds this means lan_if may never be initialized (unless later overwritten in the active-interface loop), but it is used after the loop (e.g., for the final "interface %s" stanza). Initialize lan_if unconditionally (or at least ensure it has a safe default) before any possible use when enabled_iface_num ends up 0.

Copilot uses AI. Check for mistakes.
Comment on lines +1641 to 1648
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)
#if defined(_ONESTACK_PRODUCT_REQ_)
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
#endif
{
memset(name_servs, 0, sizeof(name_servs));
}
#endif
Copy link

Copilot AI Feb 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

memset(name_servs, 0, ...) is now guarded by the PD-enabled path only. When PD is disabled (the non-PD branch), name_servs may retain data from earlier iterations before appending DNS server tokens, producing incorrect rdnss output. Clear name_servs unconditionally before populating it (e.g., once per interface iteration or before processing option values), not only when prefix delegation is enabled.

Suggested change
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)
#if defined(_ONESTACK_PRODUCT_REQ_)
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
#endif
{
memset(name_servs, 0, sizeof(name_servs));
}
#endif
/* Clear name_servs before populating DNS server tokens for this option */
memset(name_servs, 0, sizeof(name_servs));

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings February 17, 2026 09:20
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +397 to +403
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)
#if defined(_ONESTACK_PRODUCT_REQ_)
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION) == true)
#endif
{
DHCPV6S_SYSCFG_GETS(DHCPV6S_NAME, "pool", cfg->index, "", 0, "IAInterface", iface_name);
}
Copy link

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New ONESTACK runtime gating changes which syscfg key is read (IAInterface vs Interface), but the unit tests under source/test/service_ipv6 don’t appear to cover the ONESTACK path. Please add tests that mock isFeatureSupportedInCurrentMode() for both enabled/disabled and verify the expected DHCPV6S_SYSCFG_GETS key is used.

Copilot uses AI. Check for mistakes.
Comment on lines +645 to +649
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)
#if defined(_ONESTACK_PRODUCT_REQ_)
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
#endif
{
Copy link

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_active_lanif() now adds ONESTACK-specific runtime branching based on FEATURE_IPV6_DELEGATION. The existing service_ipv6 unit tests don’t appear to cover this new feature-gated behavior; please add tests for ONESTACK builds that exercise both branches (feature enabled vs disabled) and assert the expected sysevent/syscfg calls.

Copilot uses AI. Check for mistakes.
rirfha948 and others added 2 commits February 19, 2026 09:19
Reason for change: Added One Stack MACRO handling
Test Procedure:
  - TBD
Risks: None
Priority: P3
Signed-off-by: rirfha948 <rasina_irfhan@comcast.com>
Copilot AI review requested due to automatic review settings February 19, 2026 05:45
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 9 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 436 to 438
char lan_pd_if[128] = {0};
char if_name[16] = {0};
char buf[64] = {0};
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variable scope issue: When ONESTACK_PRODUCT_REQ is defined and CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION is not defined, the variables lan_pd_if, if_name, and buf are declared inside a runtime-conditional block (lines 436-438) but are used within that block's scope. This is acceptable since they're only used within the same block. However, consider declaring these variables at the function start (like active_insts at line 427) for consistency and clarity, as the else path (lines 464-473) uses the same active_insts variable but declares it earlier.

Copilot uses AI. Check for mistakes.
Comment on lines 431 to 473
@@ -449,8 +455,13 @@ STATIC int get_active_lanif(int sefd, token_t setok, unsigned int *insts, unsign

p = strtok(NULL, " ");
}

#else
}
#endif
#if !defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)
#if defined(_ONESTACK_PRODUCT_REQ_)
if (!isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
#endif
{

/* service_ipv6 sets active IPv6 interfaces instances. */
sysevent_get(sefd, setok, "ipv6_active_inst", active_insts, sizeof(active_insts));
@@ -459,14 +470,14 @@ STATIC int get_active_lanif(int sefd, token_t setok, unsigned int *insts, unsign
insts[i++] = atoi(p);
p = strtok(NULL, " ");
}

}
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential runtime execution path issue: When ONESTACK_PRODUCT_REQ is defined, both conditional blocks (lines 431-458 and 460-473) are compiled. The runtime checks use complementary conditions (FEATURE_IPV6_DELEGATION vs !FEATURE_IPV6_DELEGATION), which ensures mutual exclusivity. However, this creates a subtle issue: the variable 'i' is modified in both branches, and 'active_insts' is reused but gets overwritten in the second branch. If the feature check returns false for delegation, the second block will overwrite the 'active_insts' buffer that was already initialized at line 427, which is acceptable. Ensure that isFeatureSupportedInCurrentMode() is deterministic and cannot change between the two calls within the same function execution, as inconsistent results would cause undefined behavior.

Copilot uses AI. Check for mistakes.
DHCPV6S_SYSCFG_GETS(DHCPV6S_NAME, "pool", cfg->index, "", 0, "IAInterface", iface_name);
#else
DHCPV6S_SYSCFG_GETS(DHCPV6S_NAME, "pool", cfg->index, "", 0, "Interface", iface_name);
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent spacing in preprocessor directive: There are two spaces between '||' and 'defined(ONESTACK_PRODUCT_REQ)'. For consistency with other preprocessor directives in the codebase, use a single space.

Suggested change
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)

Copilot uses AI. Check for mistakes.
DHCPV6S_SYSCFG_GETS(DHCPV6S_NAME, "pool", cfg->index, "", 0, "IAInterface", iface_name);
}
#endif
#if !defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent spacing in preprocessor directive: There are two spaces between '||' and 'defined(ONESTACK_PRODUCT_REQ)'. For consistency with other preprocessor directives in the codebase, use a single space.

Suggested change
#if !defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)
#if !defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)

Copilot uses AI. Check for mistakes.
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION) == false)
#endif
{
DHCPV6S_SYSCFG_GETS(DHCPV6S_NAME, "pool", cfg->index, "", 0, "Interface", iface_name);
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent indentation: This line uses a tab character for indentation instead of spaces. The surrounding code uses spaces (4 spaces per indentation level). Replace the tab with spaces for consistency.

Suggested change
DHCPV6S_SYSCFG_GETS(DHCPV6S_NAME, "pool", cfg->index, "", 0, "Interface", iface_name);
DHCPV6S_SYSCFG_GETS(DHCPV6S_NAME, "pool", cfg->index, "", 0, "Interface", iface_name);

Copilot uses AI. Check for mistakes.
#endif
#if !defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)
#if defined(_ONESTACK_PRODUCT_REQ_)
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION) == false)
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent boolean comparison style: This explicit comparison with 'false' is inconsistent with other calls to isFeatureSupportedInCurrentMode() in this PR. For example, line 462 in service_routed.c uses the negation operator '!'. For consistency, use the negation operator '!' instead of explicit comparison with 'false'.

Suggested change
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION) == false)
if (!isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))

Copilot uses AI. Check for mistakes.
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) && ! defined(_CBR_PRODUCT_REQ_)
#if (defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) && !defined(_CBR_PRODUCT_REQ_)) || defined(_ONESTACK_PRODUCT_REQ_)
#if defined(_ONESTACK_PRODUCT_REQ_)
if ( isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION) == true)
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent boolean comparison style: This explicit comparison with 'true' is inconsistent with other calls to isFeatureSupportedInCurrentMode() in this PR. For example, line 433 in service_routed.c omits the '== true'. For consistency, either use the explicit comparison everywhere or omit it everywhere. If the function returns a boolean type, the explicit comparison is redundant.

Suggested change
if ( isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION) == true)
if ( isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION) )

Copilot uses AI. Check for mistakes.
#ifndef CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION
#if !defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)
#ifdef _ONESTACK_PRODUCT_REQ_
if (!isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent indentation: This runtime check should be indented with 4 spaces to align with the surrounding preprocessor conditional blocks and improve code readability.

Copilot uses AI. Check for mistakes.
DHCPV6S_SYSCFG_GETS(DHCPV6S_NAME, "pool", cfg->index, "", 0, "Interface", iface_name);
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)
#if defined(_ONESTACK_PRODUCT_REQ_)
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION) == true)
Copy link

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent boolean comparison style: This explicit comparison with 'true' is inconsistent with other calls to isFeatureSupportedInCurrentMode() in this PR. For example, line 647 in service_routed.c omits the '== true'. For consistency, either use the explicit comparison everywhere or omit it everywhere. If the function returns a boolean type, the explicit comparison is redundant.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Comments