RDKB-65730: Integrate Dynamic Table Support - MsgPack support and L2 tests - #404
Open
yogeswaransky wants to merge 5 commits into
Open
RDKB-65730: Integrate Dynamic Table Support - MsgPack support and L2 tests#404yogeswaransky wants to merge 5 commits into
yogeswaransky wants to merge 5 commits into
Conversation
…tests Signed-off-by: Yogeswaran K <yogeswaransky@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the Telemetry 2.0 profile parser to support dynamic table (“dataModelTable”) configuration in the MsgPack profile flow, aligning MsgPack behavior more closely with the existing JSON dynamic-table parsing path.
Changes:
- Added MsgPack dynamic-table parser helper (
parseDataModelTableParamsMsgpack) underENABLE_DYNAMIC_TABLE_SUPPORT. - Implemented MsgPack handling for
type == "dataModelTable"inaddParameterMsgpack_marker_config, including optional index expansion and dynamic-table structure parsing. - Introduced a
msgpack_add_paramlabel to allow the no-index dynamic-table case to fall through into the existing parameter-add path.
Suppressed comments (1)
source/t2parser/t2parser.c:2351
referenceStrownership differs between root vs nested calls (root stores it incurrentTable->reference). On these failure paths it is currently freed only whenparentTable == NULL, which can leavecurrentTable->referencedangling (and later double-freed infreeDataModelTable) and also leaksreferenceStrfor nested calls. FreereferenceStronly for nested calls on error paths.
if (buildFullPath(currentPath, parentPath, referenceStr) != 0)
{
T2Error("Failed to build current path\n");
if (!parentTable) free(referenceStr);
return T2ERROR_FAILURE;
Comment on lines
+2339
to
+2343
| { | ||
| currentTable = parentTable; | ||
| free(referenceStr); | ||
| referenceStr = msgpack_strdup(mpReference); | ||
| } |
Comment on lines
+2669
to
+2676
| char basePath[256] = ""; | ||
| char *baseRefStr = msgpack_strdup(mpBaseRef); | ||
| if (baseRefStr) | ||
| { | ||
| strncpy(basePath, baseRefStr, sizeof(basePath) - 1); | ||
| basePath[sizeof(basePath) - 1] = '\0'; | ||
| free(baseRefStr); | ||
| } |
Comment on lines
+2780
to
+2782
| free(paramtype); | ||
| paramtype = strdup("dataModel"); | ||
| // Parse sub-parameters for dynamic table structure (no-index case) |
…tests Signed-off-by: Yogeswaran K <yogeswaransky@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
source/t2parser/t2parser.c:2764
- Same as above: snprintf truncation/failure when building basePathWithIndex is logged but addParameter() is still called with a potentially truncated path. Skip the entry if snprintf fails/truncates.
int written = snprintf(basePathWithIndex, sizeof(basePathWithIndex), "%s%d.", basePath, val);
if (written < 0 || (size_t)written >= sizeof(basePathWithIndex))
{
T2Error("%s: snprintf truncated or failed while building path: '%s'\n", __FUNCTION__, basePathWithIndex);
}
source/t2parser/t2parser.c:2737
- When building basePathWithIndex, snprintf truncation/failure is logged but execution still calls addParameter() with a potentially truncated path. That can add incorrect parameter names into the profile. Treat truncation as an error and skip the index entry.
This issue also appears on line 2760 of the same file.
int written = snprintf(basePathWithIndex, sizeof(basePathWithIndex), "%s%d.", basePath, k);
if (written < 0 || (size_t)written >= sizeof(basePathWithIndex))
{
T2Error("%s: snprintf truncated or failed while building path: '%s'\n", __FUNCTION__, basePathWithIndex);
}
source/t2parser/t2parser.c:2678
- This PR adds a new MsgPack-specific dataModelTable parsing flow, but there don’t appear to be any unit tests covering MsgPack profiles with dataModelTable (existing dynamic table tests are JSON-based). Please add/extend gtest coverage to include (at minimum) dataModelTable with and without index in MsgPack, plus a nested table case, to prevent regressions in addParameterMsgpack_marker_config()/parseDataModelTableParamsMsgpack().
T2Debug("Processing dataModelTable in MsgPack profile\n");
msgpack_object *mpBaseRef = msgpack_get_map_value(Parameter_array_map, "reference");
if (mpBaseRef)
{
char basePath[256] = "";
Comment on lines
+2346
to
+2366
| char currentPath[MAX_PATH_LENGTH]; | ||
| if (buildFullPath(currentPath, parentPath, referenceStr) != 0) | ||
| { | ||
| T2Error("Failed to build current path\n"); | ||
| if (!parentTable) | ||
| { | ||
| free(referenceStr); | ||
| } | ||
| return T2ERROR_FAILURE; | ||
| } | ||
|
|
||
| char pathWithWildcard[MAX_PATH_LENGTH]; | ||
| if ((size_t)snprintf(pathWithWildcard, sizeof(pathWithWildcard), "%s*.", currentPath) >= sizeof(pathWithWildcard)) | ||
| { | ||
| T2Error("Path with wildcard exceeded buffer size\n"); | ||
| if (!parentTable) | ||
| { | ||
| free(referenceStr); | ||
| } | ||
| return T2ERROR_FAILURE; | ||
| } |
…tests Signed-off-by: Yogeswaran K <yogeswaransky@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
source/t2parser/t2parser.c:2351
- On
buildFullPath/ wildcard-path failures, the code freesreferenceStrwhen parsing the root table. But for the root tablecurrentTable->referencepoints toreferenceStr, so freeing it leaves a dangling pointer inprofile->dataModelTableListand can crash later (e.g., report generation / cleanup). Clean up the table entry from the vector on failure instead of freeing the owned string.
if (buildFullPath(currentPath, parentPath, referenceStr) != 0)
{
T2Error("Failed to build current path\n");
if (!parentTable)
{
test/run_l2.sh:58
- mock_table_provider is started in the background but cleanup is only a best-effort
killat the end of the test block. If the script exits early (e.g., gcc/pytest failure, SIGINT), the provider can be left running and interfere with subsequent test runs. Add a trap-based cleanup and fail fast if compilation/startup fails.
# Compile mock table provider for dataModelTable L2 tests
gcc -o test/functional-tests/tests/mock_table_provider test/functional-tests/tests/mock_table_provider.c \
-I/usr/local/include -I/usr/local/include/rbus \
-L/usr/local/lib -lrbus -lrbuscore -lrtMessage -lmsgpackc
# Start mock table provider in background (provides Device.X_T2TEST_Table.AccessPoint.{1,2,3}.*)
test/functional-tests/tests/mock_table_provider &
MOCK_TABLE_PROVIDER_PID=$!
sleep 2
final_result=0
# removing --exitfirst flag as it is causing the test to exit after first failure
pytest -v --json-report --json-report-summary --json-report-file $RESULT_DIR/runs_as_daemon.json test/functional-tests/tests/test_runs_as_daemon.py || final_result=1
pytest -v --json-report --json-report-summary --json-report-file $RESULT_DIR/bootup_sequence.json test/functional-tests/tests/test_bootup_sequence.py || final_result=1
pytest -v --json-report --json-report-summary --json-report-file $RESULT_DIR/xconf_communications.json test/functional-tests/tests/test_xconf_communications.py || final_result=1
pytest -v --json-report --json-report-summary --json-report-file $RESULT_DIR/msg_packet.json test/functional-tests/tests/test_multiprofile_msgpacket.py || final_result=1
pytest -v --json-report --json-report-summary --json-report-file $RESULT_DIR/datamodeltable.json test/functional-tests/tests/test_datamodeltable.py || final_result=1
# Stop mock table provider
kill $MOCK_TABLE_PROVIDER_PID 2>/dev/null
source/t2parser/t2parser.c:2408
param->referenceis used unconditionally inbuildFullPath(...), butmsgpack_strdupcan fail and return NULL (OOM). That would turn into a NULL dereference inbuildFullPath(which logs an error but still dereferencesreferenceto compute length/indexing in some code paths). Add a NULL check and freeparamearly.
param->reference = msgpack_strdup(mpParamRef);
char fullPath[MAX_PATH_LENGTH];
if (buildFullPath(fullPath, pathWithWildcard, param->reference) != 0)
…tests Signed-off-by: Yogeswaran K <yogeswaransky@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (6)
test/functional-tests/tests/mock_table_provider.c:112
- In the wildcard query path,
currentis set tonextafterrbusProperty_Release(next), so the next loop iteration appends to a released property (use-after-free / corrupted property chain). Avoid usingnextafter releasing it; typically the appended property should remain owned by the list until the RBUS framework releases the head property.
rbusProperty_t next;
rbusProperty_Init(&next, paramNames[i], val);
rbusProperty_Append(current, next);
rbusProperty_Release(next);
current = next;
source/t2parser/t2parser.c:2343
referenceStris reallocated for nested tables without a NULL-check, and the error path freesreferenceStrwhen!parentTableeven though ownership was already transferred tocurrentTable->reference. This can lead to NULL dereference inbuildFullPathand a danglingcurrentTable->reference(double-free later).
currentTable = parentTable;
free(referenceStr);
referenceStr = msgpack_strdup(mpReference);
}
source/t2parser/t2parser.c:2360
- On the wildcard-path snprintf failure, the error path frees
referenceStrwhen!parentTable, but in the root-table casereferenceStris stored incurrentTable->reference. Freeing it here leaves a dangling pointer in the table list and can cause a double-free duringfreeDataModelTable.
if ((size_t)snprintf(pathWithWildcard, sizeof(pathWithWildcard), "%s*.", currentPath) >= sizeof(pathWithWildcard))
{
T2Error("Path with wildcard exceeded buffer size\n");
test/run_l2.sh:47
- The mock table provider is started in the background without any guarantee it will be stopped if the script exits early (e.g., gcc failure, interrupted CI job). Also, the
gccinvocation isn’t checked, so the script may continue with a missing/old binary. Add an explicit build check and an EXIT trap that always terminates the background provider.
# Compile mock table provider for dataModelTable L2 tests
gcc -o test/functional-tests/tests/mock_table_provider test/functional-tests/tests/mock_table_provider.c \
-I/usr/local/include -I/usr/local/include/rbus \
-L/usr/local/lib -lrbus -lrbuscore -lrtMessage -lmsgpackc
test/run_l2.sh:58
- After introducing
cleanup_mock_table_provider, use it here instead of a rawkillso the process is waited on and the logic stays in one place (and matches the EXIT trap).
# Stop mock table provider
kill $MOCK_TABLE_PROVIDER_PID 2>/dev/null
source/t2parser/t2parser.c:2675
- MsgPack
dataModelTableparsing is now implemented, but there are no unit tests covering this MsgPack-only path (existing dynamic table tests exercise the JSON flow). Add gtests undersource/test/t2parser/that build a MsgPack profile containingdataModelTable(wildcard, explicit index, and nested) and assertprofile->dataModelTableListandparamListcontents.
else if(0 == msgpack_strcmp(Parameter_type_str, "dataModelTable"))
{
#ifdef ENABLE_DYNAMIC_TABLE_SUPPORT
T2Debug("Processing dataModelTable in MsgPack profile\n");
msgpack_object *mpBaseRef = msgpack_get_map_value(Parameter_array_map, "reference");
…tests Signed-off-by: Yogeswaran K <yogeswaransky@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (8)
test/functional-tests/tests/mock_table_provider.c:112
nextis appended to the RBUS property list and then immediately released, butcurrentis set tonextafterwards. This breaks the property chain and can become a use-after-free when RBUS later traverses the list (the main codebase releases only the head property after iteration; see source/ccspinterface/rbusInterface.c:391-394).
rbusProperty_t next;
rbusProperty_Init(&next, paramNames[i], val);
rbusProperty_Append(current, next);
rbusProperty_Release(next);
current = next;
source/t2parser/t2parser.c:2351
- On the buildFullPath error path,
referenceStris freed whenparentTableis NULL, but in that case the pointer is owned bycurrentTable->reference(stored earlier). This can leavecurrentTable->referencedangling insideprofile->dataModelTableListand cause a later free/crash.
if (buildFullPath(currentPath, parentPath, referenceStr) != 0)
{
T2Error("Failed to build current path\n");
if (!parentTable)
{
source/t2parser/t2parser.c:2362
- On the wildcard snprintf overflow path,
referenceStris freed only whenparentTableis NULL. For nested table parsing (parentTable != NULL),referenceStris newly allocated and will leak on this error path; for root parsing, freeing it can again leavecurrentTable->referencedangling.
if ((size_t)snprintf(pathWithWildcard, sizeof(pathWithWildcard), "%s*.", currentPath) >= sizeof(pathWithWildcard))
{
T2Error("Path with wildcard exceeded buffer size\n");
if (!parentTable)
{
test/functional-tests/tests/test_datamodeltable.py:38
subprocessanddtare imported but never used, which adds noise and can mask real unused-import issues.
import subprocess
from time import sleep
from datetime import datetime as dt
import pytest
test/run_l2.sh:58
- The mock table provider is started in the background but only killed at the end without a trap or wait. If this script exits early (pytest error, SIGINT/TERM, etc.), the provider can keep running and interfere with later runs;
killwithoutwaitcan also leave a zombie process. Also, the mock provider compile step is not checked for failure, so tests can run against a missing binary.
# Start mock table provider in background (provides Device.X_T2TEST_Table.AccessPoint.{1,2,3}.*)
test/functional-tests/tests/mock_table_provider &
MOCK_TABLE_PROVIDER_PID=$!
sleep 2
build_inside_container.sh:34
- This script now always enables dynamic table support, which makes the feature effectively non-optional for the container build and prevents running CI scenarios against a feature-disabled build (the L2 workflow uses this script). Consider gating this behind an environment variable so the default build remains representative.
./configure --prefix=${INSTALL_DIR} --enable-rdkcertselector=yes --enable-dynamic-table-support=yes && make && make install
test/functional-tests/tests/test_datamodeltable.py:26
- The scenario list claims the tests validate report JSON structure / row contents, but the assertions currently only check logs and that the process didn’t crash. Updating the scenario descriptions will avoid misleading future maintainers.
This issue also appears on line 35 of the same file.
1. Push a profile with dataModelTable (explicit index) -> verify report JSON structure
2. Push a profile with dataModelTable (wildcard) -> verify all rows appear in report
3. Push a profile with dataModelTable while a reporting cycle is active -> verify no crash
source/t2parser/t2parser.c:2681
- MsgPack dataModelTable support is newly added here, but there don’t appear to be any unit tests exercising the MsgPack profile parsing path (existing parser tests cover JSON/dynamic-table parsing only). Adding a focused unit test would help prevent regressions in the MsgPack flow.
T2Debug("Processing dataModelTable in MsgPack profile\n");
msgpack_object *mpBaseRef = msgpack_get_map_value(Parameter_array_map, "reference");
if (mpBaseRef)
{
char basePath[256] = "";
char *baseRefStr = msgpack_strdup(mpBaseRef);
if (baseRefStr)
{
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.