RDKB-63098: Handle IPv6 delegation for business vs residential Partner ID as part of single build#25
RDKB-63098: Handle IPv6 delegation for business vs residential Partner ID as part of single build#25
Conversation
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>
There was a problem hiding this comment.
Pull request overview
Enables a single build to handle IPv6 prefix delegation differences (e.g., business vs. residential) by broadening compilation guards to include _ONESTACK_PRODUCT_REQ_ and adding runtime feature checks before applying PD-related TLVs and sysevents.
Changes:
- Expand DHCPv6 PD compilation guards to include
_ONESTACK_PRODUCT_REQ_across the provisioning state machine and callback abstraction. - Add runtime gating via
isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION)to ignore PD TLVs and avoid starting/stopping the DHCPv6 client when PD is disabled. - Conditionally register the topology-mode TLV callback based on the runtime PD feature state.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| source/CMAgentSsp/gw_prov_sm.c | Adds OneStack compile guards and runtime feature gating for PD TLVs, DHCPv6 client sysevents, and callback registration. |
| source/CMAgentSsp/gw_prov_abstraction.h | Extends the callback typedef/member guarding to include _ONESTACK_PRODUCT_REQ_. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #ifdef _ONESTACK_PRODUCT_REQ_ | ||
| if (!isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION)) | ||
| { | ||
| /* PD feature disabled — ignore TLV */ | ||
| return TLV_PARSE_CALLBACK_OK_EXTIF; | ||
| } |
There was a problem hiding this comment.
When _ONESTACK_PRODUCT_REQ_ is enabled, this code introduces references to isFeatureSupportedInCurrentMode() and FEATURE_IPV6_DELEGATION, but there is no declaration/definition visible in this repository or included headers in this file. Please include the proper header (or add an extern prototype / enum definition) under _ONESTACK_PRODUCT_REQ_ so OneStack builds don’t fail with an implicit declaration / undefined identifier.
| #if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_) | ||
| #if defined(_ONESTACK_PRODUCT_REQ_) | ||
| if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION)) | ||
| #endif | ||
| { | ||
| v_secure_system("sysevent set dhcpv6_client-stop"); | ||
| } | ||
| #endif | ||
| } |
There was a problem hiding this comment.
Preprocessor directives in this block are indented inside the function body, which is inconsistent with most other #if/#endif usage in this file and makes the conditional structure harder to read/maintain. Consider left-aligning the #if/#endif to column 0 (and keeping the runtime if indented) to match surrounding style.
| #if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_) | |
| #if defined(_ONESTACK_PRODUCT_REQ_) | |
| if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION)) | |
| #endif | |
| { | |
| v_secure_system("sysevent set dhcpv6_client-stop"); | |
| } | |
| #endif | |
| } | |
| #if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_) | |
| #if defined(_ONESTACK_PRODUCT_REQ_) | |
| if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION)) | |
| #endif | |
| { | |
| v_secure_system("sysevent set dhcpv6_client-stop"); | |
| } | |
| #endif | |
| } |
| #if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_) | ||
| #if defined(_ONESTACK_PRODUCT_REQ_) | ||
| if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION)) | ||
| #endif | ||
| { | ||
| void* pGW_setTopologyMode = GW_setTopologyMode; | ||
| obj->pGW_SetTopologyMode = (fpGW_SetTopologyMode)pGW_setTopologyMode; | ||
| } |
There was a problem hiding this comment.
RegisterDocsisCallback() allocates appCallBack with malloc() and does not zero-initialize it. With the new runtime gating, when IPv6 delegation is not supported pGW_SetTopologyMode is left uninitialized (garbage) instead of being set to NULL, which can lead to crashes if the state machine reads/calls it. Please ensure the struct is zeroed (e.g., calloc/memset) or explicitly set obj->pGW_SetTopologyMode = NULL in the non-supported path.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -227,7 +227,7 @@ typedef struct __appCallBack | |||
| fpDocsisRATransInterval pDocsis_GetRATransInterval; | |||
| #endif | |||
| fpGW_Tr069PaSubTLVParse pGW_Tr069PaSubTLVParse; | |||
| #if defined (CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) | |||
| #if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_) | |||
| fpGW_SetTopologyMode pGW_SetTopologyMode; | |||
| #endif | |||
There was a problem hiding this comment.
This header now conditionally changes the appCallBack struct layout when _ONESTACK_PRODUCT_REQ_ is defined. Since CcspCMAgentSsp links against an external -lgwprovappabs library (where SME_CreateEventHandler likely consumes appCallBack), any mismatch in whether _ONESTACK_PRODUCT_REQ_ is defined between the executable and that library can cause ABI/layout corruption at runtime. Ensure the same macro is defined consistently for all compilation units that include this header (or avoid changing the struct layout via conditional compilation).
| @@ -143,5 +143,6 @@ AC_CONFIG_FILES( | |||
|
|
|||
| AM_CONDITIONAL([PLATFORM_PUMA7_ENABLED], [test $PLATFORM_PUMA7_ENABLED = yes]) | |||
| AM_CONDITIONAL([FEATURE_RDKB_WAN_MANAGER], [test $FEATURE_RDKB_WAN_MANAGER = yes]) | |||
There was a problem hiding this comment.
This adds an Automake conditional for ONESTACK_PRODUCT_REQ, but it is not wired to any AC_ARG_ENABLE/AC_ARG_WITH or AC_DEFINE that would propagate a corresponding C macro (e.g., _ONESTACK_PRODUCT_REQ_) into config.h/compile flags. If this repo is expected to control the feature via configure, add a proper AC_ARG_ENABLE (or similar) and an AC_DEFINE so the C sources and all dependent components can reliably compile the intended code paths.
| AM_CONDITIONAL([FEATURE_RDKB_WAN_MANAGER], [test $FEATURE_RDKB_WAN_MANAGER = yes]) | |
| AM_CONDITIONAL([FEATURE_RDKB_WAN_MANAGER], [test $FEATURE_RDKB_WAN_MANAGER = yes]) | |
| ONESTACK_PRODUCT_REQ=false | |
| AC_ARG_ENABLE([onestack_product_req], | |
| AS_HELP_STRING([--enable-onestack_product_req],[enable OneStack product requirements support]), | |
| [ | |
| case "${enableval}" in | |
| yes) | |
| ONESTACK_PRODUCT_REQ=true | |
| AC_DEFINE([_ONESTACK_PRODUCT_REQ_],[1], | |
| [Enable OneStack product requirements support]) | |
| ;; | |
| no) | |
| ONESTACK_PRODUCT_REQ=false | |
| ;; | |
| *) | |
| AC_MSG_ERROR([bad value ${enableval} for --enable-onestack_product_req]) | |
| ;; | |
| esac | |
| ], | |
| [ONESTACK_PRODUCT_REQ=false]) |
| #if defined(CISCO_CONFIG_DHCPV6_PREFIX_DELEGATION) || defined(_ONESTACK_PRODUCT_REQ_) | ||
| #if defined(_ONESTACK_PRODUCT_REQ_) | ||
| if (isFeatureSupportedInCurrentMode(FEATURE_IPV6_DELEGATION)) | ||
| #endif | ||
| { | ||
| void* pGW_setTopologyMode = GW_setTopologyMode; | ||
| obj->pGW_SetTopologyMode = (fpGW_SetTopologyMode)pGW_setTopologyMode; | ||
| } |
There was a problem hiding this comment.
When _ONESTACK_PRODUCT_REQ_ is defined but FEATURE_IPV6_DELEGATION is not supported, this block skips assigning obj->pGW_SetTopologyMode. Since obj is allocated with malloc (not zeroed), pGW_SetTopologyMode may contain an indeterminate value, which can lead to a crash/UB if the callback is invoked. Initialize the struct (e.g., zero-initialize or explicitly set pGW_SetTopologyMode = NULL in the non-supported path).
| CcspCMAgentSsp_LDFLAGS += -lnet | ||
| endif | ||
| if ONESTACK_PRODUCT_REQ | ||
| CcspCMAgentSsp_LDFLAGS += -ldevicemode -lonestack_syscfg -lonestack_log -lrdkb_feature_mode_gate |
There was a problem hiding this comment.
_ONESTACK_PRODUCT_REQ_ is used as the C preprocessor feature flag throughout the code, but the build system change introduces an Automake conditional named ONESTACK_PRODUCT_REQ and only adds link libraries here. As-is, enabling the conditional won’t define _ONESTACK_PRODUCT_REQ_, so the new code paths (and header changes guarded by _ONESTACK_PRODUCT_REQ_) won’t compile in as intended. Add a consistent compile-time definition (e.g., CcspCMAgentSsp_CPPFLAGS += -D_ONESTACK_PRODUCT_REQ_ under this conditional, or switch the C guards to match the defined macro).
| CcspCMAgentSsp_LDFLAGS += -ldevicemode -lonestack_syscfg -lonestack_log -lrdkb_feature_mode_gate | |
| CcspCMAgentSsp_LDFLAGS += -ldevicemode -lonestack_syscfg -lonestack_log -lrdkb_feature_mode_gate | |
| CcspCMAgentSsp_CPPFLAGS += -D_ONESTACK_PRODUCT_REQ_ |
snayak002c
left a comment
There was a problem hiding this comment.
validate without this changes, prefix delegation is enabled by default in business gateways
Is this a User Story (US)?
Have all dependent PRs from other components been listed ?
RDKB-63098-changes.xlsx
Does the commit message include both the User Story ticket and the Subtask ticket?
Will be all changes related to the User Story squashed and merged in a single commit?
Has the PR been raised only after completing all changes for the User Story (no partial changes)?
Has code development for the User Story been completed?
If yes, has the Gerrit topic or list of all dependent PRs across components (including meta-layer changes) been shared?
Is there a validation log available in the Jira ticket for verifying builds with the updated generic-srcrev.inc across all platforms?
RDKB-63098-logs.txt