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_ |
No description provided.