-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/cnl tests2 #11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Reason for change: Testing Corenet Lib APIs Test Procedure: As mentioned in the ticket Risks: None Signed-off-by: [email protected]
Reason for change: Testing Corenet Lib APIs Test Procedure: As mentioned in the ticket Risks: None Signed-off-by: [email protected]
…ibrary into feature/CNL_Tests2
Reason for change: Testing Corenet Lib APIs Test Procedure: As mentioned in the ticket Risks: None Signed-off-by: [email protected]
…ibrary into feature/CNL_Tests2
Reason for change: Testing Corenet Lib APIs Test Procedure: As mentioned in the ticket Risks: None Signed-off-by: [email protected]
…ibrary into feature/CNL_Tests2
Reason for change: Testing Corenet Lib APIs Test Procedure: As mentioned in the ticket Risks: None Signed-off-by: [email protected]
Reason for change: Testing Corenet Lib APIs Test Procedure: As mentioned in the ticket Risks: None Signed-off-by: [email protected]
…ibrary into feature/CNL_Tests2
Reason for change: Testing Corenet Lib APIs Test Procedure: As mentioned in the ticket Risks: None Signed-off-by: [email protected]
…ibrary into feature/CNL_Tests2
…ibrary into feature/CNL_Tests2
…ibrary into feature/CNL_Tests2
Reason for change: Testing Corenet Lib APIs Test Procedure: As mentioned in the ticket Risks: None Signed-off-by: [email protected]
Reason for change: Testing Corenet Lib APIs Test Procedure: As mentioned in the ticket Risks: None Signed-off-by: [email protected]
…ibrary into feature/CNL_Tests2
Reason for change: Testing Corenet Lib APIs Test Procedure: As mentioned in the ticket Risks: None Signed-off-by: [email protected]
Reason for change: Testing Corenet Lib APIs Test Procedure: As mentioned in the ticket Risks: None Signed-off-by: [email protected]
Reason for change: Testing Corenet Lib APIs Test Procedure: As mentioned in the ticket Risks: None Signed-off-by: [email protected]
Reason for change: Testing Corenet Lib APIs Test Procedure: As mentioned in the ticket Risks: None Signed-off-by: [email protected]
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this 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 adds comprehensive test infrastructure for the corenetlib library, including:
- A new test API executable (
corenetlib_api) that provides test handlers for various network operations - XML and JSON test case definitions for address, VLAN, route, rule, bridge, and interface operations
- A function declaration for parsing preferred source route parameters
- Bug fix removing an invalid pointer assignment in the IPv6 address retrieval function
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| source/libnet_util.h | Adds function declaration for libnet_route_parse_pref_src to support parsing preferred source addresses in routing |
| source/libnet.c | Fixes bug by removing invalid assignment *ipv6_addr = NULL in error handling path |
| source/corenetlib_tests.xml | Adds 1716 lines of XML test case definitions covering all library functionality |
| source/corenetlib_api.c | Adds complete test API implementation with 2207 lines including test handlers, validation, and JSON parsing |
| source/Makefile.am | Adds build configuration for the new corenetlib_api test executable with json-c dependency |
Comments suppressed due to low confidence (1)
source/corenetlib_api.c:1
- Using compound literal for argv array is non-portable and may cause undefined behavior. The compound literal creates a temporary array that may not persist beyond the expression. Consider creating a proper
char *argv[]array variable instead.
#include <stdio.h>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| char *dev_name = argv[3]; | ||
| char *local_ip6 = argv[4]; | ||
| char *remote_ip6 = argv[5]; | ||
| char *encaplimit = argv[6]; |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Array index out of bounds: checking for argc != 6 but then accessing argv[6]. When argc is 6, valid indices are 0-5, but the code accesses argv[6] which is out of bounds. The check should be argc != 7 or the variable assignments should use indices 2-5 only with argc checking for 6.
| char *encaplimit = argv[6]; | |
| char *encaplimit = argv[5]; |
| char *args_copy = strdup(args); | ||
| char *token = strtok(args_copy, " "); |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Memory leak: args_copy is allocated with strdup() but not freed on all code paths. While it is freed at line 456 in the success path, if the function returns early at line 462-464 due to validation failure, the allocated memory is never freed.
| snprintf(validation_command, sizeof(validation_command), "ip link show %s | grep -w 'mtu %s'", if_name, mtu_value_str); | ||
|
|
||
| char output[256]; | ||
| if (execute_command(validation_command, output, sizeof(output)) == 0 && strstr(output, "mtu")) { |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weak validation: checking for substring "mtu" in output could match unrelated text. Should use a more specific pattern like strstr(output, mtu_value_str) or construct a pattern that confirms the MTU value matches what was set.
| if (execute_command(validation_command, output, sizeof(output)) == 0 && strstr(output, "mtu")) { | |
| char mtu_pattern[32]; | |
| snprintf(mtu_pattern, sizeof(mtu_pattern), "mtu %s", mtu_value_str); | |
| if (execute_command(validation_command, output, sizeof(output)) == 0 && strstr(output, mtu_pattern)) { |
| // Validation: get MAC from ifconfig and compare | ||
| char validation_command[256]; | ||
| snprintf(validation_command, sizeof(validation_command), | ||
| "ifconfig %s | grep -i 'HWaddr' | awk '{print $5}'", if_name); |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code relies on the deprecated ifconfig command which may not be available on modern Linux systems. Consider using ip link show instead, which is the modern replacement and more universally available. Example: ip link show %s | grep 'link/ether' | awk '{print $2}'
| "ifconfig %s | grep -i 'HWaddr' | awk '{print $5}'", if_name); | |
| "ip link show %s | grep 'link/ether' | awk '{print $2}'", if_name); |
| sleep(3); | ||
|
|
||
| // Simplified validation: just check for common active flags combo | ||
| char validation_command[256]; | ||
| snprintf(validation_command, sizeof(validation_command), "ifconfig %s", if_name); | ||
|
|
||
| char output[1024] = {0}; | ||
| if (execute_command(validation_command, output, sizeof(output)) != 0) { | ||
| printf("FAIL: Validation failed to get ifconfig output for %s.\n", if_name); | ||
| return -1; | ||
| } | ||
|
|
||
| // Check for each flag substring individually | ||
| int up = strstr(output, "UP") != NULL; | ||
| // int broadcast = strstr(output, "BROADCAST") != NULL; | ||
| int running = strstr(output, "RUNNING") != NULL; | ||
| // int multicast = strstr(output, "MULTICAST") != NULL; | ||
|
|
||
|
|
||
| if (up && running) { | ||
| printf("PASS: ifconfig output contains UP and RUNNING for %s.\n", if_name); | ||
| return 0; | ||
| } else { | ||
| printf("FAIL: Expected flags not found in ifconfig output for %s.\n", if_name); |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded 3-second sleep in validation logic at line 1801 is excessive and slows down test execution. Consider reducing to a minimal delay (e.g., 100ms with usleep) or implementing a proper polling mechanism with timeout to check if flags are set.
| sleep(3); | |
| // Simplified validation: just check for common active flags combo | |
| char validation_command[256]; | |
| snprintf(validation_command, sizeof(validation_command), "ifconfig %s", if_name); | |
| char output[1024] = {0}; | |
| if (execute_command(validation_command, output, sizeof(output)) != 0) { | |
| printf("FAIL: Validation failed to get ifconfig output for %s.\n", if_name); | |
| return -1; | |
| } | |
| // Check for each flag substring individually | |
| int up = strstr(output, "UP") != NULL; | |
| // int broadcast = strstr(output, "BROADCAST") != NULL; | |
| int running = strstr(output, "RUNNING") != NULL; | |
| // int multicast = strstr(output, "MULTICAST") != NULL; | |
| if (up && running) { | |
| printf("PASS: ifconfig output contains UP and RUNNING for %s.\n", if_name); | |
| return 0; | |
| } else { | |
| printf("FAIL: Expected flags not found in ifconfig output for %s.\n", if_name); | |
| // Poll for up to 3 seconds, checking every 100ms for flags to be set | |
| const int max_attempts = 30; // 3 seconds / 100ms | |
| int attempt; | |
| int up = 0, running = 0; | |
| char validation_command[256]; | |
| char output[1024] = {0}; | |
| for (attempt = 0; attempt < max_attempts; ++attempt) { | |
| snprintf(validation_command, sizeof(validation_command), "ifconfig %s", if_name); | |
| memset(output, 0, sizeof(output)); | |
| if (execute_command(validation_command, output, sizeof(output)) != 0) { | |
| // If command fails, wait and retry | |
| usleep(100000); // 100ms | |
| continue; | |
| } | |
| up = strstr(output, "UP") != NULL; | |
| running = strstr(output, "RUNNING") != NULL; | |
| if (up && running) { | |
| break; | |
| } | |
| usleep(100000); // 100ms | |
| } | |
| if (up && running) { | |
| printf("PASS: ifconfig output contains UP and RUNNING for %s.\n", if_name); | |
| return 0; | |
| } else { | |
| printf("FAIL: Expected flags not found in ifconfig output for %s after polling.\n", if_name); |
|
|
||
| char output[1024] = {0}; | ||
| if (execute_command(validation_command, output, sizeof(output)) != 0) { | ||
| printf("FAIL: Validation failed to get ifconfig output for %s.\n", if_name); |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error message states 'Validation failed to get ifconfig output' but doesn't provide the actual reason for failure. Consider including errno or the command's stderr output to help diagnose validation failures.
| printf("FAIL: Validation failed to get ifconfig output for %s.\n", if_name); | |
| printf("FAIL: Validation failed to get ifconfig output for %s. errno=%d (%s)\n", if_name, errno, strerror(errno)); | |
| // Optionally, print the output buffer if it may contain stderr (depends on execute_command implementation) | |
| if (strlen(output) > 0) { | |
| printf("Command output: %s\n", output); | |
| } |
| // NOTE: execute_command only reads the first line into output! | ||
| // So your output only contains the first rule, not all rules. | ||
| // That's why your validation fails for rules not on the first line. | ||
|
|
||
| // To fix: use popen/fread or similar to read all output lines. |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment acknowledges a limitation in the previous code but the fix has been implemented. The comment should be updated or removed to reflect that the issue is now resolved by using popen/fread to read all output.
| // NOTE: execute_command only reads the first line into output! | |
| // So your output only contains the first rule, not all rules. | |
| // That's why your validation fails for rules not on the first line. | |
| // To fix: use popen/fread or similar to read all output lines. |
| if (execute_command(validation_command, output, sizeof(output)) != 0) { | ||
| snprintf(log_msg, sizeof(log_msg), "FAIL: Linux command validation failed for interface %s.", if_name); |
Copilot
AI
Nov 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Duplicated error handling pattern: The pattern of calling execute_command(), checking return value, constructing log messages, and calling log_to_file() appears many times throughout the file. Consider extracting this into a helper function to reduce code duplication and improve maintainability.
|
|
||
| // Function to log messages to a file | ||
| void log_to_file(const char *message) { | ||
| FILE *log_file = fopen("/rdklogs/logs/corenetlib_api.log", "a"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to use rdk-logger APIs, when writing to /rdklogs folder. If we forcefully write a file without rdk-logger, then the file might not get rotated and might fill the rdklog partition as well.
source/corenetlib_api.c
Outdated
|
|
||
| // Parse description | ||
| if (json_object_object_get_ex(testcase_obj, "description", &temp_obj)) { | ||
| test->description = strdup(json_object_get_string(temp_obj)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where the description is freed ?
| typedef struct { | ||
| const char *name; | ||
| api_handler_t handler; | ||
| } HandlerEntry; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have a separate structure HandlerEntry ? can't we reuse api_entry_t in get_handler_by_name()
source/corenetlib_api.c
Outdated
| if (json_object_get_type(arg_obj) == json_type_null) { | ||
| test->argv[i] = NULL; | ||
| } else { | ||
| test->argv[i] = strdup(json_object_get_string(arg_obj)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where the argv[i] is freed ?
| return; | ||
| } | ||
|
|
||
| fseek(fp, 0, SEEK_END); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't use fstat() instead of fseek() to get the file size?
There was a problem hiding this 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 5 out of 5 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Step 2: Remove the temporary bridge interface after tests | ||
| printf("\nRemoving temporary interface: %s...\n", test_interface); | ||
| result = handle_bridge_delete(3, (char *[]){"corenetlib_api", "bridge_delete", (char *)test_interface}); |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same compound literal issue. Use a proper array variable instead of (char *[]){"corenetlib_api", "bridge_delete", (char *)test_interface} to ensure proper lifetime.
| result = handle_bridge_delete(3, (char *[]){"corenetlib_api", "bridge_delete", (char *)test_interface}); | |
| char *bridge_delete_args[3] = {"corenetlib_api", "bridge_delete", (char *)test_interface}; | |
| result = handle_bridge_delete(3, bridge_delete_args); |
| printf("Setting up gretap0.018 and adding it to %s...\n", test_interface); | ||
| system("ip link add gretap0.018 type gretap local 192.168.1.1 remote 192.168.1.2 dev brlan18 2>/dev/null || true"); | ||
| system("ip link set gretap0.018 up"); | ||
| system("brctl addif brlan18 gretap0.018"); |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using system() calls with hardcoded commands is a security risk if any of the variables (like interface names) could potentially be controlled by user input. While brlan18 is a constant here, consider using the libnet APIs directly instead of shelling out, or at least add input validation/sanitization if any variables are incorporated into these commands in the future.
| system("brctl addif brlan18 gretap0.018"); | |
| // If interface names ever become variable, validate/sanitize them before use! | |
| char brctl_cmd[128]; | |
| snprintf(brctl_cmd, sizeof(brctl_cmd), "brctl addif %s %s", test_interface, "gretap0.018"); | |
| system(brctl_cmd); |
| if (!match) { | ||
| printf("PASS: Validation successful. Rule is no longer present.\n"); | ||
| return 0; | ||
| } else { |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number: The value 8 on line 1506 appears to be a flag value for LOOPBACK, but it's not clearly defined. Consider defining a constant like #define IFF_LOOPBACK_FLAG 8 or using the system's IFF_LOOPBACK constant from <net/if.h> if available, to make the code more readable and maintainable.
| return -1; | ||
| } | ||
|
|
||
| printf("Flags for interface %s set to %u successfully.\n", if_name, flags); |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number: The sleep(3) on line 1801 delays for 3 seconds but there's no explanation why this delay is needed. Add a comment explaining the reason for the delay (e.g., "Wait for interface flags to propagate") or consider if this is actually necessary. Hardcoded sleep values can make tests unnecessarily slow.
| printf("Flags for interface %s set to %u successfully.\n", if_name, flags); | |
| printf("Flags for interface %s set to %u successfully.\n", if_name, flags); | |
| // Wait for interface flags to propagate before validating with ifconfig |
| // Try to create the bridge if it doesn't exist (only for brlan* names) | ||
| if (strncmp(if_name, "brlan", 5) == 0) { | ||
| printf("Interface %s does not exist, attempting to create it for validation...\n", if_name); | ||
| int create_result = handle_bridge_create(3, (char *[]){"corenetlib_api", "bridge_create", if_name}); |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compound literal (char *[]){"corenetlib_api", "bridge_create", if_name} creates a temporary array that may not have a lifetime extending beyond the function call. While this works in most compilers, it's not guaranteed by the C standard. Consider creating a proper array variable instead:
char *create_argv[] = {"corenetlib_api", "bridge_create", if_name};
int create_result = handle_bridge_create(3, create_argv);| int create_result = handle_bridge_create(3, (char *[]){"corenetlib_api", "bridge_create", if_name}); | |
| char *create_argv[] = {"corenetlib_api", "bridge_create", if_name}; | |
| int create_result = handle_bridge_create(3, create_argv); |
| test->description = strdup(cJSON_GetStringValue(temp_obj)); | ||
| } |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential memory leak: test->description is allocated via strdup() on line 1894 but is never freed. Similar to the argv memory leak, this should be cleaned up when tests are done or the program exits. Add a cleanup function to free both description and argv strings for all test cases.
| int execute_command(const char *command, char *output, size_t output_size) { | ||
| FILE *fp = popen(command, "r"); | ||
| if (fp == NULL) { | ||
| perror("popen failed"); | ||
| return -1; | ||
| } | ||
|
|
||
| if (fgets(output, output_size, fp) == NULL) { | ||
| pclose(fp); | ||
| return -1; // Command execution failed or no output | ||
| } | ||
|
|
||
| if (pclose(fp) == -1) { | ||
| perror("pclose failed"); | ||
| return -1; | ||
| } | ||
|
|
||
| return 0; // Success | ||
| } |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The execute_command function has a potential issue: when output is NULL (line 730), the function will still attempt to call fgets(output, output_size, fp) on line 191, which will cause undefined behavior. The function should check if output is NULL before calling fgets, and if it is, should skip reading and only check the command's exit status.
| } else { | ||
| printf("Interface %s does not exist.\n", if_name); | ||
| if (ifconfig_status != 0) { | ||
| printf("PASS: Validation successful. Interface %s is not present in ifconfig output.\n", if_name); | ||
| return -1; | ||
| } else { | ||
| printf("FAIL: Validation failed. Interface %s unexpectedly found in ifconfig output.\n", if_name); | ||
| return -1; | ||
| } | ||
| } |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect return value logic: When the interface doesn't exist (line 1774), the function correctly prints "Interface ... does not exist" but should return 0 (success) since this is the expected behavior for a non-existent interface check. However, line 1777 returns -1, which contradicts line 1776's PASS message. The logic should be:
- If interface exists and found in ifconfig: return 0
- If interface doesn't exist and not found in ifconfig: return -1 (for negative test compatibility)
The current code has the wrong return value at line 1777.
| for (int i = 0; i < array_len && i < MAX_ARGS; i++) { | ||
| cJSON *arg_obj = cJSON_GetArrayItem(temp_obj, i); | ||
| if (cJSON_IsNull(arg_obj)) { | ||
| test->argv[i] = NULL; | ||
| } else if (cJSON_IsString(arg_obj)) { | ||
| test->argv[i] = strdup(cJSON_GetStringValue(arg_obj)); | ||
| } else { | ||
| test->argv[i] = NULL; | ||
| } | ||
| } | ||
| } |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential memory leak: In the loop starting at line 1887, test->argv[i] may be assigned memory from strdup() on line 1918. However, there's no cleanup code to free this allocated memory when tests complete or when the program exits. Consider adding a cleanup function that frees all allocated argv strings in the test cases.
|
|
||
| const char *test_interface = "brlan18"; | ||
| printf("\nCreating temporary interface: %s...\n", test_interface); | ||
| int result = handle_bridge_create(3, (char *[]){"corenetlib_api", "bridge_create", (char *)test_interface}); |
Copilot
AI
Nov 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same compound literal issue exists here. Use a proper array variable instead of (char *[]){"corenetlib_api", "bridge_create", (char *)test_interface} to ensure proper lifetime and avoid potential undefined behavior.
| int result = handle_bridge_create(3, (char *[]){"corenetlib_api", "bridge_create", (char *)test_interface}); | |
| char *bridge_create_args[] = {"corenetlib_api", "bridge_create", (char *)test_interface}; | |
| int result = handle_bridge_create(3, bridge_create_args); |
RDKB-59386 : corenetlib_api binary interface for test code
Reason for change: Testing Corenet Lib APIs using xml conf file
Test Procedure: As mentioned in the ticket
Risks: None
Signed-off-by: [email protected]