Skip to content

RDKB-63098: [ONESTACK]-Handle IPv6 delegation for business vs residential Partner ID as part of single build#238

Open
rirfha948 wants to merge 8 commits intodevelopfrom
topic/onestack_pd
Open

RDKB-63098: [ONESTACK]-Handle IPv6 delegation for business vs residential Partner ID as part of single build#238
rirfha948 wants to merge 8 commits intodevelopfrom
topic/onestack_pd

Conversation

@rirfha948
Copy link
Contributor

@rirfha948 rirfha948 commented Feb 24, 2026

Reason for change: Prefix delegation handling
Test Procedure:

            vs residential Partner ID as part of single build

Reason for change: Prefix delegation handling
Test Procedure:
  - Build OneStack Image
  - In Business-mode, Check dibbler server is started and server.conf
    has prefix-delegation class
  - In Residential-mode,  check whether device behaves as a non-CBR device
Risks: None
Priority: P1
Signed-off-by: rirfha948 <rasina_irfhan@comcast.com>
@rirfha948 rirfha948 marked this pull request as ready for review February 26, 2026 14:38
@rirfha948 rirfha948 requested review from a team as code owners February 26, 2026 14:38
Copilot AI review requested due to automatic review settings February 26, 2026 14:38
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

Adds OneStack-aware IPv6 prefix delegation handling, toggling behavior between “business” (PD enabled) and “residential” (PD disabled) modes in a single build.

Changes:

  • Gate PD-related routed/IPv6/firewall behavior at runtime via rdkb_feature_mode_gate (FEATURE_IPV6_DELEGATION).
  • Register different sysevent custom events for business vs residential mode (including new ipv6_prefix_delegation event).
  • Update build/link integration to include OneStack libraries when ONESTACK_PRODUCT_REQ is enabled.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
source/util/utils/Makefile.am Links OneStack feature/mode-gate libs when enabled
source/service_routed/service_routed.c Runtime-gates PD vs non-PD code paths
source/service_routed/Makefile.am Adds include/link flags and OneStack libs
source/service_ipv6/service_ipv6.c Runtime-gates PD behavior; updates helper and start/stop logic
source/service_ipv6/Makefile.am Links OneStack feature/mode-gate libs when enabled
source/scripts/init/service.d/service_routed_bci.sh Adds ipv6_prefix_delegation event handling; expands model list
source/scripts/init/service.d/service_registration_functions.sh Adds debug prints during event registration
source/scripts/init/service.d/service_dhcpv6_server_bci.sh Adds ipv6_prefix_delegation event handling; expands model list
source/scripts/init/service.d/service_crond.sh Expands model list for cron entry
source/scripts/init/c_registration/Makefile.am Links OneStack feature/mode-gate libs when enabled
source/scripts/init/c_registration/20_routing.c Chooses custom events list based on PD feature mode
source/scripts/init/c_registration/15_dhcpv6_server.c Chooses custom events list based on PD feature mode
source/scripts/init/c_registration/15_dhcpv6_client.c Enables client custom events under OneStack
source/firewall/firewall_ipv6.c Runtime-gates multinet firewall rules under OneStack
source/firewall/firewall.h Exposes multinet firewall prototype for OneStack builds
source/firewall/Makefile.am Links OneStack feature/mode-gate libs when enabled

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

"dhcpv6_server-stop|/etc/utopia/service.d/service_dhcpv6_server_bci.sh",
"dhcpv6_server-restart|/etc/utopia/service.d/service_dhcpv6_server_bci.sh",
"ipv6_prefix_delegation|/etc/utopia/service.d/service_dhcpv6_server_bci.sh",
"dhcpv6_option_changed|/etc/utopia/service.d/service_dhcpv6_server_bci.sh|NULL|"TUPLE_FLAG_EVENT,NULL
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

This array initializer is syntactically invalid: the final element combines a string literal and NULL without a separating comma, which will fail compilation. Split the last entry into a string element and a separate NULL terminator (and ensure the TUPLE_FLAG_EVENT concatenation remains part of the string literal).

Suggested change
"dhcpv6_option_changed|/etc/utopia/service.d/service_dhcpv6_server_bci.sh|NULL|"TUPLE_FLAG_EVENT,NULL
"dhcpv6_option_changed|/etc/utopia/service.d/service_dhcpv6_server_bci.sh|NULL|"TUPLE_FLAG_EVENT,
NULL

Copilot uses AI. Check for mistakes.
Comment on lines +49 to +50
#if defined(_ONESTACK_PRODUCT_REQ_)
const char** SERVICE_CUSTOM_EVENTS = NULL;
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

Under configurations where _ONESTACK_PRODUCT_REQ_ and CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION are both defined, this will produce a redefinition of SERVICE_CUSTOM_EVENTS with incompatible types (const char** vs const char* []). Refactor to define SERVICE_CUSTOM_EVENTS exactly once via a single #if / #elif / #else chain, and consider using const char* const *SERVICE_CUSTOM_EVENTS for the pointer form to preserve const-correctness.

Suggested change
#if defined(_ONESTACK_PRODUCT_REQ_)
const char** SERVICE_CUSTOM_EVENTS = NULL;
#if defined(_ONESTACK_PRODUCT_REQ_) && !defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION)
const char* const *SERVICE_CUSTOM_EVENTS = NULL;

Copilot uses AI. Check for mistakes.
Comment on lines +45 to 50
#if defined(_ONESTACK_PRODUCT_REQ_)
const char** SERVICE_CUSTOM_EVENTS = NULL;
#endif

#if defined(_CBR_PRODUCT_REQ_) && !defined(_CBR2_PRODUCT_REQ_)
const char* SERVICE_CUSTOM_EVENTS[] = {
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

This introduces the same potential symbol clash as in 20_routing.c: if _ONESTACK_PRODUCT_REQ_ is defined alongside another branch that defines SERVICE_CUSTOM_EVENTS[], compilation will fail due to redefinition. Prefer a single compile-time selection that declares SERVICE_CUSTOM_EVENTS once, and use a const-correct pointer type (const char* const *) when you need to re-point it at different static arrays.

Copilot uses AI. Check for mistakes.
Comment on lines +72 to +73
"routeset-ula|/usr/bin/service_routed|NULL|"TUPLE_FLAG_EVENT,
"routeunset-ula|/usr/bin//service_routed|NULL|"TUPLE_FLAG_EVENT,
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

The routeunset-ula handler path contains a double slash (/usr/bin//service_routed). While it may resolve on many systems, it’s an easy source of inconsistency and can break path comparisons/tools. Use /usr/bin/service_routed.

Copilot uses AI. Check for mistakes.
Comment on lines +263 to +264
#if defined( _CBR_PRODUCT_REQ_) || defined (_ONESTACK_PRODUCT_REQ_)
STATIC uint64_t helper_ntoh64(const long long unsigned int *inputval)
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

Changing the parameter type from const uint64_t * to const long long unsigned int * is not portable: uint64_t is not guaranteed to be unsigned long long on all toolchains, and this can introduce incompatible-pointer warnings/errors at call sites. Keep the parameter as const uint64_t * (or change to a byte-oriented type like const uint8_t */const void * since the implementation treats it as raw bytes).

Copilot uses AI. Check for mistakes.

STATIC int serv_ipv6_stop(struct serv_ipv6 *si6)
{
fprintf(stderr, "Entered serv_ipv6_stop \n");
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

These unconditional fprintf(stderr, ...) debug prints will spam logs in production and are inconsistent with the rest of the service logging patterns. Please gate them behind an existing debug macro/compile flag (or remove them) and use the project’s standard logging facility if available.

Suggested change
fprintf(stderr, "Entered serv_ipv6_stop \n");

Copilot uses AI. Check for mistakes.
return 0
fi

echo "[DEBUG] sm_rm_event ${SMR_SERVICE} ${SMR_EVENT_NAME}"
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

These debug echo statements are unconditional and will add noise to boot/service manager logs on all devices. Consider guarding them behind a debug flag/environment variable (or using an existing structured logger like ulog if that’s the convention for this repo).

Suggested change
echo "[DEBUG] sm_rm_event ${SMR_SERVICE} ${SMR_EVENT_NAME}"
ulog srvmgr status "[DEBUG] sm_rm_event ${SMR_SERVICE} ${SMR_EVENT_NAME}"

Copilot uses AI. Check for mistakes.
IFS=';'
for custom in $SM_CUSTOM_EVENTS ; do
if [ -n "$custom" ] && [ " " != "$custom" ] ; then
echo "[DHCPV6][DEBUG] Registering custom event: '$custom' for service '$SM_SERVICE'"
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

These debug echo statements are unconditional and will add noise to boot/service manager logs on all devices. Consider guarding them behind a debug flag/environment variable (or using an existing structured logger like ulog if that’s the convention for this repo).

Suggested change
echo "[DHCPV6][DEBUG] Registering custom event: '$custom' for service '$SM_SERVICE'"
ulog dhcpv6 debug "Registering custom event: '$custom' for service '$SM_SERVICE'"

Copilot uses AI. Check for mistakes.
-I$(top_srcdir)/source/util/utils \
$(DBUS_CFLAGS)

AM_LDFLAGS = -lsecure_wrapper -lccsp_common $(DBUS_LIBS)
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

These Makefile changes put libraries (-l...) into AM_LDFLAGS / service_routed_LDFLAGS. In Automake, *_LDFLAGS is intended for linker flags (e.g., -L, -Wl, options), while libraries should generally go into *_LDADD (or LIBS). Moving -lccsp_common, $(DBUS_LIBS), -lnet, and the OneStack libraries to service_routed_LDADD will avoid link-order issues and align with standard Automake conventions.

Copilot uses AI. Check for mistakes.
Comment on lines +47 to +52
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
if CORE_NET_LIB_FEATURE_SUPPORT
service_routed_LDFLAGS = -lnet
service_routed_LDFLAGS += -lnet
Copy link

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

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

These Makefile changes put libraries (-l...) into AM_LDFLAGS / service_routed_LDFLAGS. In Automake, *_LDFLAGS is intended for linker flags (e.g., -L, -Wl, options), while libraries should generally go into *_LDADD (or LIBS). Moving -lccsp_common, $(DBUS_LIBS), -lnet, and the OneStack libraries to service_routed_LDADD will avoid link-order issues and align with standard Automake conventions.

Copilot uses AI. Check for mistakes.
            vs residential Partner ID as part of single build

Reason for change: Prefix delegation handling
Test Procedure:
  - Build OneStack Image
  - In Business-mode, Check dibbler server is started and server.conf
    has prefix-delegation class
  - In Residential-mode,  check whether device behaves as a non-CBR device
Risks: None
Priority: P1
Signed-off-by: rirfha948 <rasina_irfhan@comcast.com>
Copilot AI review requested due to automatic review settings February 27, 2026 09:25
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 14 out of 14 changed files in this pull request and generated 9 comments.

Comments suppressed due to low confidence (1)

source/service_routed/service_routed.c:1795

  • Inconsistent indentation and formatting. The 'if' statement on line 1786 is not indented properly, and the variable declarations starting at line 1789 should follow standard C89/C90 convention of being declared at the beginning of the block (before any executable statements), or the code should be compiled with C99 or later standards. Consider adding proper indentation and following the code style of the surrounding context.
if (!isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
#endif
{
char cmd[100];
char out[100];
char interface_name[32] = {0};
char *token = NULL; 
char *pt;
char pref_rx[16];


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

"ipv6_ntp_server|/etc/utopia/service.d/service_dhcpv6_server.sh",
"dhcp_domain|/etc/utopia/service.d/service_dhcpv6_server.sh",
"current_lan_ipv6address|/etc/utopia/service.d/service_dhcpv6_server.sh",
NULL
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

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

Inconsistent indentation in array. Line 70 uses a tab character while surrounding lines use spaces. This creates visual inconsistency and should be standardized to match the indentation style of the rest of the array.

Suggested change
NULL
NULL

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 27, 2026

Choose a reason for hiding this comment

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

Missing space in the preprocessor directive condition. The code has '#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(ONESTACK_PRODUCT_REQ)' with two spaces after '||'. While this doesn't cause a compilation error, it's inconsistent with the single space used elsewhere in the file.

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 27, 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. This line has two spaces after '||' while other similar conditions use a single space. Consider using consistent spacing throughout.

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 27, 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 while the surrounding code uses spaces. This creates visual inconsistency and can cause issues with different editor tab settings.

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.
Comment on lines 2108 to +2400
@@ -2098,10 +2125,17 @@ STATIC int serv_ipv6_start(struct serv_ipv6 *si6)
sysevent_set(si6->sefd, si6->setok, "service_ipv6-status", "starting", 0);

/* Fix for IPv6 prefix not getting updated in dibbler server conf file on WAN mode change */
#if defined(_CBR2_PRODUCT_REQ_)
#if defined(_CBR2_PRODUCT_REQ_)
sysevent_get(si6->sefd, si6->setok, "ipv6_prefix", si6->mso_prefix, sizeof(si6->mso_prefix));
sysevent_set(si6->sefd, si6->setok, "ipv6_prefix-divided", "", 0);
#endif
#if defined(_ONESTACK_PRODUCT_REQ_)
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
{
sysevent_get(si6->sefd, si6->setok, "ipv6_prefix_delegation", si6->mso_prefix, sizeof(si6->mso_prefix));
sysevent_set(si6->sefd, si6->setok, "ipv6_prefix-divided", "", 0);
}
#endif

/*
* Update MTU of all the enabled IPv6 instances
@@ -2118,13 +2152,22 @@ STATIC int serv_ipv6_start(struct serv_ipv6 *si6)
* 5) Send RA, start DHCPv6 server
*/
/* For CBR product the lan(brlan0) v6 address set is done as part of PandM process*/
#if !defined(_CBR_PRODUCT_REQ_) && !defined(_BWG_PRODUCT_REQ_)
#if !defined(_CBR_PRODUCT_REQ_) && !defined(_BWG_PRODUCT_REQ_) && !defined(_ONESTACK_PRODUCT_REQ_)
if (lan_addr6_set(si6) !=0) {
fprintf(stderr, "assign IPv6 address for lan interfaces error!\n");
sysevent_set(si6->sefd, si6->setok, "service_ipv6-status", "error", 0);
return -1;
}
#endif
#if defined (_ONESTACK_PRODUCT_REQ_)
if (!isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION)){
if (lan_addr6_set(si6) !=0) {
fprintf(stderr, "assign IPv6 address for lan interfaces error!\n");
sysevent_set(si6->sefd, si6->setok, "service_ipv6-status", "error", 0);
return -1;
}
}
#endif

//Intel Proposed RDKB Generic Bug Fix from XB6 SDK
/*start zebra*/
@@ -2152,6 +2195,7 @@ STATIC int serv_ipv6_start(struct serv_ipv6 *si6)

STATIC int serv_ipv6_stop(struct serv_ipv6 *si6)
{
fprintf(stderr, "Entered serv_ipv6_stop \n");
if (!serv_can_stop(si6->sefd, si6->setok, "service_ipv6"))
return -1;

@@ -2162,7 +2206,7 @@ STATIC int serv_ipv6_stop(struct serv_ipv6 *si6)
sysevent_set(si6->sefd, si6->setok, "service_ipv6-status", "error", 0);
return -1;
}
#if !defined(_CBR_PRODUCT_REQ_) && !defined(_BWG_PRODUCT_REQ_)
#if !defined(_CBR_PRODUCT_REQ_) && !defined(_BWG_PRODUCT_REQ_) && !defined(_ONESTACK_PRODUCT_REQ_)
del_addr6_flg = false;
if (lan_addr6_unset(si6) !=0) {
fprintf(stderr, "unset IPv6 address for lan interfaces error!\n");
@@ -2171,6 +2215,18 @@ STATIC int serv_ipv6_stop(struct serv_ipv6 *si6)
return -1;
}
del_addr6_flg = true;
#endif
#if defined (_ONESTACK_PRODUCT_REQ_)
if (!isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION)){
del_addr6_flg = false;
if (lan_addr6_unset(si6) !=0) {
fprintf(stderr, "unset IPv6 address for lan interfaces error!\n");
sysevent_set(si6->sefd, si6->setok, "service_ipv6-status", "error", 0);
del_addr6_flg = true;
return -1;
}
del_addr6_flg = true;
}
#endif
sysevent_set(si6->sefd, si6->setok, "service_ipv6-status", "stopped", 0);
return 0;
@@ -2219,12 +2275,28 @@ STATIC int serv_ipv6_init(struct serv_ipv6 *si6)
return -1;
#endif
}

#if defined (_ONESTACK_PRODUCT_REQ_)
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
{
sysevent_get(si6->sefd, si6->setok, "ipv6_prefix_delegation", si6->mso_prefix, sizeof(si6->mso_prefix));
}
else
{
sysevent_get(si6->sefd, si6->setok, "ipv6_prefix", si6->mso_prefix, sizeof(si6->mso_prefix));
}
#else
sysevent_get(si6->sefd, si6->setok, "ipv6_prefix", si6->mso_prefix, sizeof(si6->mso_prefix));
#endif
fprintf(stderr, "IPv6 Prefix :%s\n", si6->mso_prefix);
if (strlen(si6->mso_prefix))
{
si6->wan_ready = true;
}
else
{
fprintf(stderr, "[%s] -- Init ipv6_prefix retrieval failed\n", PROG_NAME);
return -1;
}

sysevent_get(si6->sefd, si6->setok, "erouter_topology-mode", buf, sizeof(buf));
switch(atoi(buf)) {
@@ -2307,8 +2379,10 @@ int service_ipv6_main(int argc, char *argv[])
}

if (serv_ipv6_init(&si6) != 0)
{
fprintf(stderr, "[%s] -- Service IPV6 Initialization failed\n", PROG_NAME);
exit(1);

}
for (i = 0; i < NELEMS(cmd_ops); i++) {
if (strcmp(argv[1], cmd_ops[i].cmd) != 0 || !cmd_ops[i].exec)
continue;
@@ -2323,6 +2397,7 @@ int service_ipv6_main(int argc, char *argv[])
if (i == NELEMS(cmd_ops))
fprintf(stderr, "[%s] unknown command: %s\n", PROG_NAME, argv[1]);

fprintf(stderr, "[%s] -- DHCPv6 start in progress\n", PROG_NAME);
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

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

The debug fprintf statements added throughout this function should be removed or converted to proper logging. Debug print statements left in production code can impact performance and clutter logs. Consider using a conditional debug macro or removing these before merging.

Copilot uses AI. Check for mistakes.
IFS=';'
for custom in $SM_CUSTOM_EVENTS ; do
if [ -n "$custom" ] && [ " " != "$custom" ] ; then
echo "[DHCPV6][DEBUG] Registering custom event: '$custom' for service '$SM_SERVICE'"
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

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

Debug echo statements are being added to shell scripts. These should be removed or made conditional before production deployment to avoid cluttering logs and potentially exposing sensitive information.

Suggested change
echo "[DHCPV6][DEBUG] Registering custom event: '$custom' for service '$SM_SERVICE'"

Copilot uses AI. Check for mistakes.
Comment on lines +1491 to +1501
#if defined (_COSA_BCM_MIPS_)
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION)
// For CBR platform, the captive portal redirection feature was removed
// inWifiCp = 1;
#else
inWifiCp = 1;
inWifiCp = 1;
#endif
#else
inWifiCp = 1;
inWifiCp = 1;
#endif
}
}
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

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

Inconsistent indentation: this code block uses tab characters while the surrounding code may use spaces. The lines 1491-1501 should use consistent indentation matching the rest of the file to maintain code style uniformity.

Copilot uses AI. Check for mistakes.
Comment on lines +71 to +75
#ifdef WAN_FAILOVER_SUPPORTED
"routeset-ula|/usr/bin/service_routed|NULL|"TUPLE_FLAG_EVENT,
"routeunset-ula|/usr/bin//service_routed|NULL|"TUPLE_FLAG_EVENT,
#endif
NULL
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

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

Using preprocessor directives inside array initialization is non-standard and could cause compilation issues or unexpected behavior depending on the compiler. If WAN_FAILOVER_SUPPORTED is not defined, this will result in a trailing comma before NULL, which while typically handled by most C compilers, is technically a C99 feature and may not be portable. Consider defining two complete separate arrays or restructuring to avoid conditionals within the array.

Copilot uses AI. Check for mistakes.
Copilot AI review requested due to automatic review settings February 27, 2026 11:56
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 12 out of 13 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 +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 27, 2026

Choose a reason for hiding this comment

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

In route_unset(), adding _ONESTACK_PRODUCT_REQ_ to the #if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(MULTILAN_FEATURE) ... condition makes the preprocessor always select this branch for OneStack builds, so the #elif !defined(WAN_MANAGER_UNIFICATION_ENABLED) route/rule cleanup path is not compiled at all. In residential mode (when FEATURE_IPV6_DELEGATION is not supported), the runtime if (isFeatureSupportedInCurrentMode(...)) will skip the body and route_unset() effectively becomes a no-op, potentially leaving stale IPv6 rules/routes. Restructure this conditional so OneStack can execute the non-PD cleanup path when the feature gate is false (e.g., compile both paths for OneStack and select at runtime).

Copilot uses AI. Check for mistakes.
Comment on lines +263 to 265
#if defined( _CBR_PRODUCT_REQ_) || defined (_ONESTACK_PRODUCT_REQ_)
STATIC uint64_t helper_ntoh64(const long long unsigned int *inputval)
{
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

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

helper_ntoh64()/helper_hton64() now take const long long unsigned int *, which is non-portable (on some platforms uint64_t is unsigned long). Prefer using const uint64_t * and changing tmp_prefix/sub_prefix to uint64_t (or casting at the callsites) to avoid ABI and pointer-type mismatches.

Copilot uses AI. Check for mistakes.
Comment on lines +2297 to 2298
fprintf(stderr, "[%s] -- Init ipv6_prefix retrieval failed\n", PROG_NAME);
return -1;
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

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

serv_ipv6_init() now returns -1 when the prefix is missing. Since service_ipv6_main() exits on init failure, this can prevent stop/restart flows from running during boot or transient WAN states. Consider not failing init solely due to an empty prefix; set wan_ready = false and let serv_ipv6_start() handle readiness.

Suggested change
fprintf(stderr, "[%s] -- Init ipv6_prefix retrieval failed\n", PROG_NAME);
return -1;
fprintf(stderr, "[%s] -- Init ipv6_prefix retrieval failed\n", PROG_NAME);
si6->mso_prefix[0] = '\0';
si6->wan_ready = false;

Copilot uses AI. Check for mistakes.
Comment on lines +1183 to +1184

if (pd_enabled || multilan_enabled)
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

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

In gen_zebra_conf(), the OneStack logic sets pd_enabled from isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION) and then wraps the subsequent zebra.conf emission under if (pd_enabled || multilan_enabled). In residential mode (feature not supported) with MULTILAN_FEATURE off, this condition is false and the expected RA/interface config won’t be written. Consider ensuring the non-PD single-LAN zebra.conf path still runs when the feature gate is false.

Suggested change
if (pd_enabled || multilan_enabled)
#ifdef _ONESTACK_PRODUCT_REQ_
/* In OneStack builds, always run the zebra.conf emission block so that
* the non-PD single-LAN configuration is still generated even when
* IPv6 delegation is not supported in the current mode. PD-specific
* behavior inside the block remains controlled by pd_enabled.
*/
if (1)
#else
if (pd_enabled || multilan_enabled)
#endif

Copilot uses AI. Check for mistakes.

STATIC int serv_ipv6_start(struct serv_ipv6 *si6)
{
fprintf(stderr, "Entered serv_ipv6_start \n");
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

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

The new unconditional fprintf(stderr, "Entered serv_ipv6_start") debug print will spam logs on every start. Please remove it or gate it behind an existing debug macro/log level to avoid noisy production logs.

Suggested change
fprintf(stderr, "Entered serv_ipv6_start \n");

Copilot uses AI. Check for mistakes.
Comment on lines +45 to +47
#if defined(_ONESTACK_PRODUCT_REQ_)
const char** SERVICE_CUSTOM_EVENTS = NULL;
#endif
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

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

For OneStack, SERVICE_CUSTOM_EVENTS is declared as a pointer (const char**), but the file can also define a const char* SERVICE_CUSTOM_EVENTS[] array in other preprocessor branches. If _ONESTACK_PRODUCT_REQ_ is ever combined with those branches, this will be a symbol redefinition compile error. Please adjust the preprocessor logic or rename the OneStack variable so SERVICE_CUSTOM_EVENTS is defined exactly once per build.

Copilot uses AI. Check for mistakes.
Comment on lines +431 to +435
#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 27, 2026

Choose a reason for hiding this comment

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

The OneStack feature-gated IPv6 delegation behavior (e.g., runtime branching on isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION) for active LAN selection / config generation) is not covered by the existing unit tests under source/test/service_routed. Please add tests that exercise both feature modes to prevent regressions (at minimum verifying route_unset() and zebra.conf generation behave correctly in business vs residential mode).

Copilot uses AI. Check for mistakes.
Comment on lines +49 to 53
#if defined(_ONESTACK_PRODUCT_REQ_)
const char** SERVICE_CUSTOM_EVENTS = NULL;
#endif
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION)
const char* SERVICE_CUSTOM_EVENTS[] = {
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

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

OneStack introduces const char** SERVICE_CUSTOM_EVENTS = NULL; but this file also defines const char* SERVICE_CUSTOM_EVENTS[] under CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION. If both macros are enabled, this will not compile (symbol redefinition). Please restructure/guard so only one SERVICE_CUSTOM_EVENTS definition exists for any macro combination.

Copilot uses AI. Check for mistakes.
Comment on lines +2132 to +2136
#if defined(_ONESTACK_PRODUCT_REQ_)
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
{
sysevent_get(si6->sefd, si6->setok, "ipv6_prefix_delegation", si6->mso_prefix, sizeof(si6->mso_prefix));
sysevent_set(si6->sefd, si6->setok, "ipv6_prefix-divided", "", 0);
Copy link

Copilot AI Feb 27, 2026

Choose a reason for hiding this comment

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

The new OneStack branch in serv_ipv6_start() (switching to ipv6_prefix_delegation and skipping lan_addr6_set() when delegation is enabled) isn’t covered by existing source/test/service_ipv6 unit tests. Please add tests that mock isFeatureSupportedInCurrentMode() and verify both business vs residential paths.

Copilot uses AI. Check for mistakes.
#ifdef _CBR_PRODUCT_REQ_
STATIC uint64_t helper_ntoh64(const uint64_t *inputval)
#if defined( _CBR_PRODUCT_REQ_) || defined (_ONESTACK_PRODUCT_REQ_)
STATIC uint64_t helper_ntoh64(const long long unsigned int *inputval)
Copy link
Contributor

Choose a reason for hiding this comment

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

why we need this change ? this will impact CBRv2 as well

Copy link
Contributor Author

@rirfha948 rirfha948 Mar 1, 2026

Choose a reason for hiding this comment

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

For Little Endian devices , this is needed, Both CBRv2 and XB10 are little endian devices , ```
root@Docsis-Gateway:# deviceinfo.sh -mo
CGA4332COM
root@Docsis-Gateway:
# lscpu | grep -i endian
Byte Order: Little Endian
root@Docsis-Gateway:~#

@rirfha948 rirfha948 requested a review from rajkamal-cv March 1, 2026 09:40
@rirfha948 rirfha948 force-pushed the topic/onestack_pd branch from bb5a727 to 8836593 Compare March 2, 2026 09:47
Copilot AI review requested due to automatic review settings March 2, 2026 10:54
@rirfha948 rirfha948 force-pushed the topic/onestack_pd branch from 8836593 to 7b599ff Compare March 2, 2026 10:54
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 8 comments.


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

Comment on lines +519 to +523
#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 Mar 2, 2026

Choose a reason for hiding this comment

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

On OneStack builds, this #if ... || defined(_ONESTACK_PRODUCT_REQ_) forces compilation into the PD/MULTILAN branch even when FEATURE_IPV6_DELEGATION is false at runtime, which also prevents the compile-time fallback branch (the #elif !defined(WAN_MANAGER_UNIFICATION_ENABLED) path) from being compiled at all. This risks skipping the required non-PD routing behavior in residential mode. Restructure so OneStack compiles both behaviors and selects via runtime if/else (e.g., make OneStack a separate #if defined(_ONESTACK_PRODUCT_REQ_) wrapper with an explicit else calling the non-PD logic), while keeping the original compile-time selection for non-OneStack builds.

Copilot uses AI. Check for mistakes.
Comment on lines +712 to +713
}
#elif !defined(WAN_MANAGER_UNIFICATION_ENABLED)
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

Same issue as route_set(): for OneStack, the #elif !defined(WAN_MANAGER_UNIFICATION_ENABLED) block will never be compiled (because _ONESTACK_PRODUCT_REQ_ makes the first #if true), even though the runtime feature check can be false. This can drop the non-PD route-unset behavior in residential mode. Refactor the preprocessor structure so that OneStack builds compile both code paths and choose at runtime (explicit if/else), instead of using _ONESTACK_PRODUCT_REQ_ in the compile-time condition that controls the #elif.

Copilot uses AI. Check for mistakes.
Comment on lines +49 to 53
#if defined(_ONESTACK_PRODUCT_REQ_)
const char** SERVICE_CUSTOM_EVENTS = NULL;
#endif
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION)
const char* SERVICE_CUSTOM_EVENTS[] = {
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

This introduces a duplicate definition of SERVICE_CUSTOM_EVENTS when both _ONESTACK_PRODUCT_REQ_ and CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION are defined (pointer + array with the same identifier), which will fail compilation. Fix by making the OneStack pointer declaration mutually exclusive with the other SERVICE_CUSTOM_EVENTS[] declarations (e.g., fold it into the same #if/#elif/#else chain), or rename the OneStack-selected pointer (e.g., SERVICE_CUSTOM_EVENTS_PTR) and pass that to sm_register().

Copilot uses AI. Check for mistakes.
Comment on lines +1025 to 1033
#ifdef WAN_FAILOVER_SUPPORTED
char last_broadcasted_prefix[64] = {0};
#endif
#if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_)
#if defined(_ONESTACK_PRODUCT_REQ_)
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
#endif
{
sysevent_get(sefd, setok, "previous_ipv6_prefix", orig_prefix, sizeof(orig_prefix));
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

When WAN_FAILOVER_SUPPORTED is enabled but CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION is defined and _ONESTACK_PRODUCT_REQ_ is not, last_broadcasted_prefix is declared but the only code that uses it is not compiled (it lives in the non-PD branch). This can trigger an unused-variable build warning. Declare last_broadcasted_prefix only within the branch where it’s used, or guard its declaration with the same preprocessor conditions as the usage.

Copilot uses AI. Check for mistakes.
Comment on lines +263 to 266
#if defined( _CBR_PRODUCT_REQ_) || defined (_ONESTACK_PRODUCT_REQ_)
STATIC uint64_t helper_ntoh64(const long long unsigned int *inputval)
{
uint64_t returnval;
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

Changing the parameter type from const uint64_t * to const long long unsigned int * makes the API less portable and can cause type-mismatch warnings/errors depending on how uint64_t is typedef’d on the target toolchain. Prefer keeping the signature as const uint64_t * (and adjust call sites if needed), since the function is explicitly operating on 64-bit values.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is called only in CBR and XB10 devices, Due to Architecture incompatability error, quoted below, this has been changed

In CBRv2, ---> 32bit Arch ----> uint64_t is unsigned long long
In XB10, ---> 64bit Arch ----> uint64_t is unsigned long 
/git/source/service_ipv6/service_ipv6.c
| ../../../git/source/service_ipv6/service_ipv6.c: In function 'divide_ipv6_prefix':
| ../../../git/source/service_ipv6/service_ipv6.c:909:36: error: passing argument 1 of 'helper_ntoh64' from incompatible pointer type [-Werror=incompatible-pointer-types]
|   909 |         tmp_prefix = helper_ntoh64(&tmp_prefix); // The memcpy is copying in reverse order due to LEndianess
|       |                                    ^~~~~~~~~~~
|       |                                    |
|       |                                    long long unsigned int *
| ../../../git/source/service_ipv6/service_ipv6.c:264:47: note: expected 'const uint64_t *' {aka 'const long unsigned int *'} but argument is of type 'long long unsigned int *'
|   264 | STATIC uint64_t helper_ntoh64(const uint64_t *inputval)
|       |                               ~~~~~~~~~~~~~~~~^~~~~~~~
| ../../../git/source/service_ipv6/service_ipv6.c:922:44: error: passing argument 1 of 'helper_hton64' from incompatible pointer type [-Werror=incompatible-pointer-types]
|   922 |                 sub_prefix = helper_hton64(&sub_prefix);// The memcpy is copying in reverse order due to LEndianess
|       |                                            ^~~~~~~~~~~
|       |                                            |
|       |                                            long long unsigned int *
| ../../../git/source/service_ipv6/service_ipv6.c:281:47: note: expected 'const uint64_t *' {aka 'const long unsigned int *'} but argument is of type 'long long unsigned int *'
|   281 | STATIC uint64_t helper_hton64(const uint64_t *inputval)
|       |                               ~~~~~~~~~~~~~~~~^~~~~~~~
| ../../../git/source/service_ipv6/service_ipv6.c:950:36: error: passing argument 1 of 'helper_ntoh64' from incompatible pointer type [-Werror=incompatible-pointer-types]

Comment on lines +22 to +26
AM_CPPFLAGS = -I$(top_srcdir)/source/include \
-I$(top_srcdir)/source/util/utils \
$(DBUS_CFLAGS)

AM_LDFLAGS = -lsecure_wrapper -lccsp_common $(DBUS_LIBS)
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

Using AM_LDFLAGS to add libraries (-l... and $(DBUS_LIBS)) is non-idiomatic Automake and can lead to link ordering issues. Prefer moving libraries into service_routed_LDADD (or LIBS) and keep *_LDFLAGS for linker flags like -L.../-Wl,....

Copilot uses AI. Check for mistakes.
15_hotspot_SOURCES = 15_hotspot.c
endif
if ONESTACK_PRODUCT_REQ
LDFLAGS += -lrdkb_feature_mode_gate
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

Appending -l... to global LDFLAGS affects all targets in this directory and mixes libraries into a flags variable. Prefer adding -lrdkb_feature_mode_gate to the specific program’s LDADD/LIBS (or a target-specific *_LDADD) so only the binaries that include rdkb_feature_mode_gate.h link against it.

Suggested change
LDFLAGS += -lrdkb_feature_mode_gate
02_devicemode_LDADD = -lrdkb_feature_mode_gate

Copilot uses AI. Check for mistakes.

STATIC int serv_ipv6_stop(struct serv_ipv6 *si6)
{
fprintf(stderr, "Entered serv_ipv6_stop \n");
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

Same as above: avoid unconditional stderr debug prints in the stop path; use the existing logging mechanism or a debug-only guard.

Suggested change
fprintf(stderr, "Entered serv_ipv6_stop \n");
#ifdef DEBUG
fprintf(stderr, "Entered serv_ipv6_stop \n");
#endif

Copilot uses AI. Check for mistakes.
@rirfha948 rirfha948 force-pushed the topic/onestack_pd branch from 7b599ff to 9280c3f Compare March 2, 2026 11:35
Copilot AI review requested due to automatic review settings March 3, 2026 00: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

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


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

Comment on lines +1259 to +1267
if(isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
{
sysevent_get(sysevent_fd, sysevent_token, "ipv6_prefix_delegation", prefix, sizeof(prefix));
}
else
{
sysevent_get(sysevent_fd, sysevent_token, "ipv6_prefix", prefix, sizeof(prefix));
}
}
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

isFeatureSupportedInCurrentMode() is called inside the WAN_FAILOVER_SUPPORTED branch without an _ONESTACK_PRODUCT_REQ_ guard, but the header/library are only included/linked for ONESTACK builds. If WAN_FAILOVER_SUPPORTED is enabled on a non-ONESTACK build this will fail to compile/link. Please wrap these calls with #ifdef _ONESTACK_PRODUCT_REQ_ (and fall back to ipv6_prefix otherwise), or make the feature-gate dependency unconditional for the configs that enable this path.

Suggested change
if(isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
{
sysevent_get(sysevent_fd, sysevent_token, "ipv6_prefix_delegation", prefix, sizeof(prefix));
}
else
{
sysevent_get(sysevent_fd, sysevent_token, "ipv6_prefix", prefix, sizeof(prefix));
}
}
#ifdef _ONESTACK_PRODUCT_REQ_
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
{
sysevent_get(sysevent_fd, sysevent_token, "ipv6_prefix_delegation", prefix, sizeof(prefix));
}
else
{
sysevent_get(sysevent_fd, sysevent_token, "ipv6_prefix", prefix, sizeof(prefix));
}
#else
sysevent_get(sysevent_fd, sysevent_token, "ipv6_prefix", prefix, sizeof(prefix));
#endif
}

Copilot uses AI. Check for mistakes.
Comment on lines +2081 to +2086
{
if(isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
{
sysevent_get(sysevent_fd, sysevent_token, "ipv6_prefix_delegation", prefix, sizeof(prefix));
}
else
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

applyRoutingRules() is compiled when WAN_FAILOVER_SUPPORTED is enabled, but it now calls isFeatureSupportedInCurrentMode() unconditionally (no _ONESTACK_PRODUCT_REQ_ guard). Since rdkb_feature_mode_gate.h/-lrdkb_feature_mode_gate are only present for ONESTACK builds, this introduces a build break for non-ONESTACK + WAN_FAILOVER configurations. Guard the feature-gate call with _ONESTACK_PRODUCT_REQ_ (and default to ipv6_prefix otherwise).

Suggested change
{
if(isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
{
sysevent_get(sysevent_fd, sysevent_token, "ipv6_prefix_delegation", prefix, sizeof(prefix));
}
else
{
#ifdef _ONESTACK_PRODUCT_REQ_
if(isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
{
sysevent_get(sysevent_fd, sysevent_token, "ipv6_prefix_delegation", prefix, sizeof(prefix));
}
else
#endif

Copilot uses AI. Check for mistakes.
Comment on lines 45 to 60
@@ -47,11 +54,29 @@ const char* SERVICE_CUSTOM_EVENTS[] = {
"dhcpv6_server-restart|/usr/bin/service_ipv6",
NULL
};
#elif defined (CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION)
#elif defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION)
const char* SERVICE_CUSTOM_EVENTS[] = {
"dhcpv6_option_changed|/etc/utopia/service.d/service_dhcpv6_server.sh|NULL|"TUPLE_FLAG_EVENT,
NULL
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

Same issue as in 20_routing.c: _ONESTACK_PRODUCT_REQ_ adds a const char** SERVICE_CUSTOM_EVENTS declaration, but the CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION branch defines const char* SERVICE_CUSTOM_EVENTS[]. If both macros are defined this will not compile due to redefinition/type mismatch. Restructure so there is only one SERVICE_CUSTOM_EVENTS definition in any build configuration (or use distinct names for the pointer vs arrays).

Copilot uses AI. Check for mistakes.
if (i == NELEMS(cmd_ops))
fprintf(stderr, "[%s] unknown command: %s\n", PROG_NAME, argv[1]);

fprintf(stderr, "[%s] -- DHCPv6 start in progress\n", PROG_NAME);
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

The log line "-- DHCPv6 start in progress" is printed unconditionally after processing any command (including stop, restart, or even an unknown command). This is misleading for operators and makes logs noisier. Consider removing it or changing it to a command-agnostic message (or only printing it for the start paths).

Suggested change
fprintf(stderr, "[%s] -- DHCPv6 start in progress\n", PROG_NAME);
fprintf(stderr, "[%s] -- DHCPv6 command processing complete\n", PROG_NAME);

Copilot uses AI. Check for mistakes.
@rirfha948 rirfha948 force-pushed the topic/onestack_pd branch from 7fe7748 to e1f892c Compare March 3, 2026 00:46
Copilot AI review requested due to automatic review settings March 3, 2026 04:06
@rirfha948 rirfha948 force-pushed the topic/onestack_pd branch from e1f892c to 33c34ce Compare March 3, 2026 04:06
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 13 out of 13 changed files in this pull request and generated 3 comments.


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

Comment on lines +1169 to 1186
#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);
Copy link

Copilot AI Mar 3, 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 starting here appears to be closed by an existing #endif shortly after the first few statements (around where lan_addr is fetched). That early close will compile out the opening if/for braces when the condition is false, but the closing braces later in gen_zebra_conf() still compile, causing a build break in configurations without MULTILAN/PD/ONESTACK. Please ensure the #if introduced here is closed at the same place the for loop / surrounding if are closed (near the later } //for (...)), and remove/relocate the premature #endif.

Copilot uses AI. Check for mistakes.
Comment on lines 263 to 282
@@ -275,7 +278,7 @@ STATIC uint64_t helper_ntoh64(const uint64_t *inputval)
return returnval;
}

STATIC uint64_t helper_hton64(const uint64_t *inputval)
STATIC uint64_t helper_hton64(const long long unsigned int *inputval)
{
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

helper_ntoh64() / helper_hton64() now take a const long long unsigned int *, but they are called with &tmp_prefix where tmp_prefix is a uint64_t. This introduces an incompatible pointer type conversion (can be a warning or error depending on flags/toolchain). Keeping the parameter as const uint64_t * (as before) avoids the mismatch and still supports the bit-shift logic.

Copilot uses AI. Check for mistakes.
Comment on lines +2133 to +2137
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
{
sysevent_get(si6->sefd, si6->setok, "ipv6_prefix_delegation", si6->mso_prefix, sizeof(si6->mso_prefix));
sysevent_set(si6->sefd, si6->setok, "ipv6_prefix-divided", "", 0);
}
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

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

ONESTACK adds new behavior here (reading ipv6_prefix_delegation and resetting ipv6_prefix-divided only when FEATURE_IPV6_DELEGATION is supported). The existing service_ipv6_gtest suite doesn’t exercise this ONESTACK-specific path, so regressions in business vs residential mode selection would be easy to miss. Please consider adding unit coverage that builds with _ONESTACK_PRODUCT_REQ_ and stubs isFeatureSupportedInCurrentMode() to validate both branches (delegation vs non-delegation) set the expected sysevent keys / control flow.

Suggested change
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
{
sysevent_get(si6->sefd, si6->setok, "ipv6_prefix_delegation", si6->mso_prefix, sizeof(si6->mso_prefix));
sysevent_set(si6->sefd, si6->setok, "ipv6_prefix-divided", "", 0);
}
if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION))
{
sysevent_get(si6->sefd, si6->setok, "ipv6_prefix_delegation", si6->mso_prefix, sizeof(si6->mso_prefix));
sysevent_set(si6->sefd, si6->setok, "ipv6_prefix-divided", "", 0);
}
else
{
/* Fall back to legacy behavior when prefix delegation is not supported. */
sysevent_get(si6->sefd, si6->setok, "ipv6_prefix", si6->mso_prefix, sizeof(si6->mso_prefix));
sysevent_set(si6->sefd, si6->setok, "ipv6_prefix-divided", "", 0);
}

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.

4 participants