util: extend oc_endpoint_addresses_t by custom on change callback #618
Conversation
|
🎉 Thank you for your code contribution! To guarantee the change/addition is conformant to the OCF Specification, we would like to ask you to execute OCF Conformance Testing of your change ☝️ when your work is ready to be reviewed. ℹ️ To verify your latest change (160511e), label this PR with |
WalkthroughThe recent updates focus on enhancing cloud connectivity and management within the IoTivity-lite framework. Key improvements include introducing conditional logic for cloud context settings, refining device initialization and deinitialization processes, and optimizing cloud endpoint management. Additionally, the updates bring more structured error handling, debug logging, and efficient token expiration calculations. These changes aim to streamline cloud interactions, improve reliability, and facilitate easier debugging and maintenance. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (14)
- api/cloud/oc_cloud.c (7 hunks)
- api/cloud/oc_cloud_context.c (1 hunks)
- api/cloud/oc_cloud_internal.h (1 hunks)
- api/cloud/oc_cloud_manager.c (3 hunks)
- api/cloud/oc_cloud_resource.c (1 hunks)
- api/cloud/oc_cloud_store.c (2 hunks)
- api/cloud/unittest/cloud_resource_test.cpp (1 hunks)
- api/oc_rep.c (2 hunks)
- api/oc_rep_internal.h (1 hunks)
- include/oc_cloud.h (1 hunks)
- include/oc_rep.h (1 hunks)
- swig/swig_interfaces/oc_rep.i (1 hunks)
- util/oc_endpoint_address.c (3 hunks)
- util/oc_endpoint_address_internal.h (2 hunks)
Files skipped from review due to trivial changes (1)
- api/oc_rep_internal.h
Additional comments: 22
api/cloud/oc_cloud_internal.h (1)
- 66-68: The addition of
oc_cloud_reset_endpointfunction is a good enhancement for managing cloud endpoints. It's consistent with the design and naming conventions of the existing API.However, it would be beneficial to include documentation comments above the function declaration to explain its purpose, expected behavior, and any side effects or preconditions. This will help future developers understand and use this function correctly.
api/cloud/oc_cloud_resource.c (1)
- 129-129: The replacement of
oc_rep_getwithoc_rep_get_by_type_and_keyfor retrieving string properties is a positive change that aligns with the PR's objectives to enhance property retrieval accuracy and reliability.Ensure that all instances where
oc_rep_getwas previously used for string properties across the codebase have been updated to useoc_rep_get_by_type_and_keyto maintain consistency and leverage the improved functionality.api/cloud/oc_cloud_context.c (1)
- 237-237: The replacement of specific operations on
ctx->cloud_epwith a call tooc_cloud_reset_endpoint(ctx)centralizes endpoint reset logic, improving maintainability and readability.Ensure that the
oc_cloud_reset_endpointfunction correctly handles all necessary operations previously performed directly onctx->cloud_epto maintain the intended functionality.util/oc_endpoint_address_internal.h (2)
- 102-108: The introduction of
oc_endpoint_addresses_on_selected_change_tstruct and associated functions for handling selected address change callbacks is a valuable enhancement.However, there's a typographical error in the function name
oc_endpoint_adddresses_set_on_selected_change(extra 'd' in "addresses"). Consider correcting this to maintain consistency and clarity in the API.Additionally, adding documentation comments for the new struct and functions would be beneficial to explain their purpose, expected behavior, and usage.
- 145-154: Ensure that documentation comments are added for the new functions
oc_endpoint_adddresses_set_on_selected_changeandoc_endpoint_addresses_get_on_selected_changeto explain their purpose, expected behavior, and usage. This will help future developers understand and use these functions correctly.api/oc_rep.c (1)
- 150-151: The renaming of
oc_rep_gettooc_rep_get_by_type_and_keyand the modification to include separate parameters for type and key is a positive change that enhances property retrieval accuracy and reliability.Consider adding documentation comments for the
oc_rep_get_by_type_and_keyfunction to explain its purpose, expected behavior, and any special considerations regarding its parameters. This will help future developers understand and use this function correctly.api/cloud/oc_cloud_store.c (2)
- 268-279: The replacement of
oc_rep_getwithoc_rep_get_by_type_and_keyforCLOUD_ENDPOINT_URIandCLOUD_ENDPOINT_IDis a positive change. It enhances the accuracy and reliability of property retrieval by specifying both the type and key of the properties being accessed.- 392-393: The update to the
oc_cloud_store_reinitializefunction, specifically the change in arguments passed tooc_cloud_store_initialize, aligns with the introduction of the new callback functionality for cloud endpoint addresses. This ensures that the custom on-change callback is correctly set during reinitialization, enhancing the dynamic handling of endpoint address changes.api/cloud/unittest/cloud_resource_test.cpp (1)
- 60-68: The replacement of
oc_rep_getwithoc_rep_get_by_type_and_keyin the unit test for retrieving URI and ID values from server objects is consistent with the changes made in the main codebase. This ensures that the tests accurately reflect the updated logic and continue to validate the functionality effectively.api/cloud/oc_cloud.c (4)
- 81-85: The introduction of conditional logic to set
ctx->store.cpsbased on the status ofctx->store.statusis a logical enhancement. It ensures that the cloud provisioning status (cps) is accurately reflected based on whether the cloud is registered, improving the clarity and correctness of the cloud context state.- 153-159: The introduction of the
oc_cloud_reset_endpointfunction centralizes the logic for resetting cloud endpoints and their states. This is a significant improvement in code maintainability and reusability, as it replaces multiple instances of similar logic scattered throughout the codebase with a single, well-defined function.- 248-248: Utilizing
oc_cloud_reset_endpointwithinoc_cloud_provision_conf_resourceto reset the cloud endpoint before reinitializing the cloud store and restarting the cloud manager is a good practice. It ensures that the cloud context is in a clean state before applying new configurations, enhancing the reliability of cloud provisioning operations.- 298-298: The use of
oc_cloud_reset_endpointinoc_cloud_update_by_resourcefor handling cloud configuration updates via resource provisioning is appropriate. It ensures that any existing cloud connection is properly closed and the endpoint is reset before applying new configurations, which is crucial for the correct application of updates.util/oc_endpoint_address.c (4)
- 142-143: The callback function
eas->on_selected_change.cbis correctly invoked with its associated dataeas->on_selected_change.cb_data. This change aligns with the PR objectives of enhancing flexibility and responsiveness in cloud endpoint address management. However, it's important to ensure that the callback function and its data are properly set before this invocation. Consider adding a check or documentation to ensure that users of this API are aware of the need to set the callback and its data beforehand.- 212-213: The initialization of the callback function and its data within
oc_endpoint_addresses_initfunction is correctly implemented. This setup is crucial for the dynamic handling of changes to endpoint addresses. It's recommended to validate the inputs to this function, especially the callback function pointer, to ensure it's not NULL unless intentionally allowed. This can prevent potential null-pointer dereferences when the callback is invoked.- 253-256: The reinitialization logic in
oc_endpoint_addresses_reinitcorrectly preserves the callback function and its data while reinitializing the endpoint addresses. This ensures that the dynamic handling of endpoint address changes is maintained across reinitializations. It's a good practice to document the expected behavior and state of the system after reinitialization to help users understand how it affects the registered callbacks and the list of endpoint addresses.- 258-265: The functions
oc_endpoint_adddresses_set_on_selected_changeandoc_endpoint_addresses_get_on_selected_changeprovide a clear API for managing the callback for selected endpoint address changes. It's important to ensure that the setter function properly handles replacing an existing callback or setting it to NULL if removing the callback is intended. Additionally, consider adding error handling or return values to these functions to indicate success or failure in setting or getting the callback, especially in constrained environments where operations may fail due to resource limitations.api/cloud/oc_cloud_manager.c (2)
- 305-305: The replacement of
oc_rep_getwithoc_rep_get_by_type_and_keyenhances the precision of property retrieval by specifying both the type and key. This is a positive change for code clarity and error prevention. Ensure that the rest of the function and its callers correctly handle the returned value, especially considering memory management and pointer safety.- 405-405: The use of
oc_cloud_reset_endpointcentralizes the resetting of cloud endpoints, which is a positive change towards more efficient and maintainable code. Ensure thatoc_cloud_reset_endpointcorrectly handles all necessary cleanup and reinitialization tasks for the cloud endpoint.include/oc_rep.h (1)
- 1571-1586: The function
oc_rep_get_by_type_and_keyintroduces a way to retrieve properties by type and key, which is a valuable addition. However, it would be beneficial to include in the documentation how the function handles cases where the key is not found or the type does not match the expected value. Additionally, ensure that the implementation includes checks for NULL pointers and validates the type against the providedoc_rep_tobject.swig/swig_interfaces/oc_rep.i (2)
- 1743-1743: Renaming
oc_mmemtoOCArrayand exposing it as a class in Java bindings is a significant change. Ensure that the necessary helper functions are also exposed and documented for effective use in Java. Additionally, verify that this change aligns with the PR objectives related to enhancing cloud endpoint management and property retrieval.- 1743-1743: Ensure that all inline implementations for Java bindings follow Java naming conventions and are consistent with the existing codebase. This includes verifying the use of Java types, method names, and documentation comments. Additionally, consider adding unit tests for these bindings to ensure their correctness and reliability.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- api/cloud/oc_cloud.c (10 hunks)
- api/cloud/oc_cloud_context.c (4 hunks)
- api/cloud/oc_cloud_context_internal.h (3 hunks)
- api/cloud/oc_cloud_manager.c (7 hunks)
- api/cloud/oc_cloud_manager_internal.h (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- api/cloud/oc_cloud.c
- api/cloud/oc_cloud_context.c
- api/cloud/oc_cloud_manager.c
Additional comments: 4
api/cloud/oc_cloud_manager_internal.h (1)
- 94-101: The function
cloud_manager_register_try_retryis well-defined and follows the coding standards of the file. However, it would be beneficial to enhance the documentation to specify under what conditions a retry should be attempted. This additional detail can help developers understand the function's behavior more clearly.api/cloud/oc_cloud_context_internal.h (3)
- 65-73: The
oc_cloud_registration_context_tstruct is well-designed for managing registration context. However, it would be helpful to provide more details on howremaining_server_changesis used in the retry logic, specifically how it's decremented or influences the retry process.- 174-177: The function
oc_cloud_registration_context_initis clearly defined. It would be beneficial to clarify in the documentation how theserversparameter is used during initialization, particularly how it relates to theinitial_serverfield of theoc_cloud_registration_context_tstruct.- 179-181: The function
oc_cloud_registration_context_deinitis well-defined and follows the coding standards of the file. LGTM!
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- api/cloud/oc_cloud_context.c (4 hunks)
- api/cloud/oc_cloud_manager.c (7 hunks)
Files skipped from review as they are similar to previous changes (2)
- api/cloud/oc_cloud_context.c
- api/cloud/oc_cloud_manager.c
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- api/cloud/oc_cloud_context.c (4 hunks)
- swig/swig_interfaces/oc_cloud.i (1 hunks)
- swig/swig_interfaces/oc_endpoint_address.i (3 hunks)
- util/oc_endpoint_address.c (3 hunks)
- util/oc_endpoint_address_internal.h (2 hunks)
- util/unittest/endpoint_address_test.cpp (4 hunks)
Files skipped from review as they are similar to previous changes (3)
- api/cloud/oc_cloud_context.c
- util/oc_endpoint_address.c
- util/oc_endpoint_address_internal.h
Additional comments: 12
swig/swig_interfaces/oc_endpoint_address.i (4)
- 35-35: The
%ignoredirective is used to prevent SWIG from generating bindings foroc_endpoint_address_view. Ensure that this entity is not required in the Java bindings, as ignoring it will make it inaccessible from Java code.- 91-95: Ignoring several types and functions related to endpoint address metadata and on-change callbacks. Confirm that these are intentionally not exposed to Java, as this could limit the ability to interact with endpoint address metadata and change notifications from Java.
- 113-113: Ignoring
oc_endpoint_addresses_selectedmight impact the ability to retrieve the currently selected endpoint address from Java. If this functionality is required, consider providing a Java-accessible method to access this information.- 118-119: The ignoring of
oc_endpoint_addresses_set_on_selected_changeandoc_endpoint_addresses_get_on_selected_changesuggests that the mechanism to set and get the on-selected-change callback is not exposed to Java. This could limit the ability to react to changes in the selected endpoint address from Java code. If this functionality is needed, consider exposing appropriate methods.swig/swig_interfaces/oc_cloud.i (2)
- 716-719: Ignoring declarations related to
oc_cloud_registration_context_tand its initialization and deinitialization functions. This decision to ignore these entities should be carefully considered, as it affects the ability to manage cloud registration contexts from Java. If these contexts are essential for cloud operations, consider exposing them through Java bindings.- 719-719: Ignoring the
registration_ctxfield withinoc_cloud_context_tmay limit the ability to access registration context details from Java. If operations on the cloud registration context are required from Java, consider exposing this field.util/unittest/endpoint_address_test.cpp (6)
- 381-387: The test case
TestEndpointAddresses, Selectinitializes a callback for selected changes but does not verify if the callback is correctly invoked upon selection changes. Consider adding assertions to ensure that the callback is invoked as expected when the selected endpoint address changes.- 390-390: This line attempts to select the next endpoint address when the list is empty. The test correctly expects no change in the selected address. However, it would be beneficial to add a comment explaining the expected behavior in this scenario for clarity.
- 407-407: After adding an endpoint address to an empty list, the test verifies that the
selected_changedflag is set totrue. This implicitly tests the behavior that adding the first endpoint address automatically selects it. It might be helpful to explicitly state this behavior in a comment for better understanding.- 420-423: The test case checks that attempting to select a non-existing URI does not change the selected endpoint address. It's good practice, but consider adding a brief comment explaining the purpose of this test case for clarity.
- 468-476: The test case uses conditional compilation to handle dynamic allocation scenarios differently. It's important to ensure that both paths (with and without
OC_DYNAMIC_ALLOCATION) are thoroughly tested. Consider adding a brief comment explaining the rationale behind the conditional compilation here.- 476-476: The test verifies that selecting the next endpoint address correctly updates the selected address. Adding a comment to explain the expected behavior when the list contains only one address or when it wraps around would enhance readability.
Danielius1922
force-pushed
the
adam/feature/extend-endpoint-list
branch
from
343e01a to
5ba1d0e
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (6)
- api/cloud/oc_cloud_context.c (4 hunks)
- swig/swig_interfaces/oc_cloud.i (1 hunks)
- swig/swig_interfaces/oc_endpoint_address.i (3 hunks)
- util/oc_endpoint_address.c (3 hunks)
- util/oc_endpoint_address_internal.h (2 hunks)
- util/unittest/endpoint_address_test.cpp (4 hunks)
Files skipped from review as they are similar to previous changes (6)
- api/cloud/oc_cloud_context.c
- swig/swig_interfaces/oc_cloud.i
- swig/swig_interfaces/oc_endpoint_address.i
- util/oc_endpoint_address.c
- util/oc_endpoint_address_internal.h
- util/unittest/endpoint_address_test.cpp
Danielius1922
force-pushed
the
adam/feature/extend-endpoint-list
branch
from
5ba1d0e to
0107fca
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (15)
- api/cloud/oc_cloud.c (13 hunks)
- api/cloud/oc_cloud_context.c (4 hunks)
- api/cloud/oc_cloud_endpoints.c (2 hunks)
- api/cloud/oc_cloud_endpoints_internal.h (2 hunks)
- api/cloud/oc_cloud_internal.h (4 hunks)
- api/cloud/oc_cloud_manager.c (7 hunks)
- api/cloud/oc_cloud_manager_internal.h (1 hunks)
- api/cloud/oc_cloud_resource.c (2 hunks)
- api/cloud/oc_cloud_store.c (2 hunks)
- api/cloud/unittest/cloud_test.cpp (3 hunks)
- swig/swig_interfaces/oc_cloud.i (1 hunks)
- swig/swig_interfaces/oc_endpoint_address.i (3 hunks)
- util/oc_endpoint_address.c (5 hunks)
- util/oc_endpoint_address_internal.h (3 hunks)
- util/unittest/endpoint_address_test.cpp (4 hunks)
Files skipped from review as they are similar to previous changes (12)
- api/cloud/oc_cloud.c
- api/cloud/oc_cloud_context.c
- api/cloud/oc_cloud_internal.h
- api/cloud/oc_cloud_manager.c
- api/cloud/oc_cloud_manager_internal.h
- api/cloud/oc_cloud_resource.c
- api/cloud/oc_cloud_store.c
- swig/swig_interfaces/oc_cloud.i
- swig/swig_interfaces/oc_endpoint_address.i
- util/oc_endpoint_address.c
- util/oc_endpoint_address_internal.h
- util/unittest/endpoint_address_test.cpp
Additional comments: 6
api/cloud/oc_cloud_endpoints_internal.h (3)
- 23-23: The inclusion of
"oc_rep.h"is appropriate for the usage ofoc_rep_tin the newly introduced struct. Good practice to include what's necessary.- 47-52: The
oc_endpoint_addresses_rep_tstruct is well-defined and serves its purpose of representing endpoint addresses with URI, UUID keys, and a pointer to servers.- 54-58: The function
oc_cloud_endpoint_addresses_setis clearly defined and matches its intended functionality of setting cloud endpoint addresses.api/cloud/oc_cloud_endpoints.c (2)
- 25-26: The inclusion of
"api/cloud/oc_cloud_log_internal.h"and"api/cloud/oc_cloud_resource_internal.h"is appropriate for logging and resource handling within the new function. Good practice to include what's necessary.- 50-122: The implementation of
oc_cloud_endpoint_addresses_setis clear and well-structured, with appropriate error handling and debug logging. However, consider the implications of usingasserton line 81 in release builds where assertions might be disabled. Ensure that critical checks are handled appropriately even when assertions are not active.api/cloud/unittest/cloud_test.cpp (1)
- 110-152: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [104-149]
The test case
oc_cloud_update_by_resourceis well-structured and effectively covers various scenarios for cloud updates, including handling of missing uri, missing id, invalid id, and duplicate servers. The use ofRepPoolfor parsing and extracting server information is appropriate and ensures that valid server data is processed correctly.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (14)
- .github/workflows/ctt-test.yml (1 hunks)
- .github/workflows/plgd-device-test-with-cfg.yml (1 hunks)
- .github/workflows/plgd-hub-test-with-cfg.yml (1 hunks)
- .github/workflows/sonar-cloud-analysis.yml (1 hunks)
- .github/workflows/unit-test-with-cfg.yml (1 hunks)
- api/cloud/oc_cloud_context.c (5 hunks)
- api/cloud/oc_cloud_context_internal.h (3 hunks)
- api/cloud/oc_cloud_manager.c (11 hunks)
- api/cloud/oc_cloud_manager_internal.h (2 hunks)
- api/cloud/oc_cloud_rd.c (1 hunks)
- api/cloud/oc_cloud_resource.c (2 hunks)
- api/cloud/oc_cloud_store.c (5 hunks)
- api/cloud/rd_client.c (2 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (4 hunks)
Files skipped from review as they are similar to previous changes (5)
- api/cloud/oc_cloud_context.c
- api/cloud/oc_cloud_context_internal.h
- api/cloud/oc_cloud_manager.c
- api/cloud/oc_cloud_resource.c
- api/cloud/oc_cloud_store.c
Additional comments: 17
.github/workflows/ctt-test.yml (1)
- 31-31: The update from
actions/upload-artifact@v3toactions/upload-artifact@v4is correctly applied and follows best practices for maintaining up-to-date dependencies in GitHub Actions workflows.api/cloud/oc_cloud_manager_internal.h (2)
- 67-79: The addition of
cloud_manager_calculate_refresh_token_expirationfunction is a logical enhancement for managing refresh token expiration times. The function signature and documentation are clear and concise.- 108-116: The addition of
cloud_manager_register_check_retry_with_changed_serverfunction enhances the robustness of cloud registration processes by handling server changes. The function's documentation and signature are appropriately detailed..github/workflows/unit-test-with-cfg.yml (1)
- 138-138: Updating the
actions/upload-artifactaction tov4is correctly applied, ensuring the workflow uses the latest version for improved features and security..github/workflows/plgd-hub-test-with-cfg.yml (1)
- 125-125: The update to
actions/upload-artifact@v4in this workflow file is appropriate, aligning with best practices for using the most current versions of GitHub Actions..github/workflows/plgd-device-test-with-cfg.yml (1)
- 132-132: Correctly updating the
actions/upload-artifactaction to versionv4ensures the workflow benefits from the latest improvements and security features..github/workflows/sonar-cloud-analysis.yml (1)
- 116-116: Updating the
actions/download-artifactaction tov4is a prudent maintenance step, ensuring the workflow uses the latest version for enhanced functionality and security.api/cloud/oc_cloud_rd.c (1)
- 372-372: Updating the logging function to
OC_CLOUD_ERRfor cloud-related errors enhances the clarity and specificity of log messages. This is a good practice for categorizing and managing log outputs, especially in a module focused on cloud functionality.api/cloud/rd_client.c (2)
- 161-161: The update to use
OC_CLOUD_ERRfor logging errors related to cloud operations is appropriate and improves the specificity of log messages in the cloud module.- 215-215: The change to
OC_CLOUD_DBGfor debug messages related to cloud functionality is a good practice, enhancing the clarity and relevance of debug logs within the cloud module.api/cloud/unittest/cloud_manager_test.cpp (7)
- 48-48: The constant
kTestServeris introduced to specify a server address. Ensure that this address is appropriate for testing purposes and does not point to a live or production server to avoid unintended interactions.- 70-71: Initialization of
m_context.storenow includes a callback functioncloud_context_on_server_change. Verify that this callback is properly defined and handles server changes as expected. It's crucial for maintaining the robustness of cloud connectivity.- 74-79: The use of
kTestServerin assertions related to server addresses is a good practice for consistency. Ensure that the server address defined bykTestServeris correctly used throughout the tests and matches the expected format for cloud endpoints.- 88-89: Adjusting the logging level in
SetUpandTearDownis a good practice for controlling the verbosity of test outputs. However, ensure that the logging level is set back to a default or previous state after tests to avoid affecting other tests or components.Consider storing the previous logging level in
SetUpand restoring it inTearDownto ensure that the global logging configuration is not unintentionally altered.
- 359-360: The comment indicates that there are three servers to try, including the default cloud server,
kTestServer, anduri. This is a good practice for testing the cloud manager's ability to handle multiple servers. Ensure that the test covers scenarios where each server is tried and appropriately handles failures or redirects.- 362-363: Asserting that
kTestServeris selected as the cloud server is a good practice for verifying the cloud manager's server selection logic. Ensure that the selection criteria are well-documented and align with the system's requirements for cloud connectivity.- 462-490: The tests for calculating refresh token expiration times are crucial for ensuring that the system refreshes tokens before they expire, maintaining continuous cloud connectivity. Verify that the logic for calculating expiration times accounts for possible time discrepancies and network delays.
Danielius1922
force-pushed
the
adam/feature/extend-endpoint-list
branch
from
0fbbc25 to
20a7c97
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (14)
- .github/workflows/ctt-test.yml (1 hunks)
- .github/workflows/plgd-device-test-with-cfg.yml (2 hunks)
- .github/workflows/plgd-hub-test-with-cfg.yml (2 hunks)
- .github/workflows/sonar-cloud-analysis.yml (1 hunks)
- .github/workflows/unit-test-with-cfg.yml (2 hunks)
- api/cloud/oc_cloud_context.c (5 hunks)
- api/cloud/oc_cloud_context_internal.h (3 hunks)
- api/cloud/oc_cloud_manager.c (11 hunks)
- api/cloud/oc_cloud_manager_internal.h (2 hunks)
- api/cloud/oc_cloud_rd.c (1 hunks)
- api/cloud/oc_cloud_resource.c (2 hunks)
- api/cloud/oc_cloud_store.c (5 hunks)
- api/cloud/rd_client.c (2 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (4 hunks)
Files skipped from review as they are similar to previous changes (13)
- .github/workflows/ctt-test.yml
- .github/workflows/plgd-device-test-with-cfg.yml
- .github/workflows/sonar-cloud-analysis.yml
- .github/workflows/unit-test-with-cfg.yml
- api/cloud/oc_cloud_context.c
- api/cloud/oc_cloud_context_internal.h
- api/cloud/oc_cloud_manager.c
- api/cloud/oc_cloud_manager_internal.h
- api/cloud/oc_cloud_rd.c
- api/cloud/oc_cloud_resource.c
- api/cloud/oc_cloud_store.c
- api/cloud/rd_client.c
- api/cloud/unittest/cloud_manager_test.cpp
Additional comments: 3
.github/workflows/plgd-hub-test-with-cfg.yml (3)
- 106-112: The dynamic generation of the file name and artifact name using a unique suffix is a good practice for ensuring traceability of coverage artifacts. However, it might be helpful to add a comment explaining the rationale behind the choice of inputs for the suffix generation to aid future maintainability.
- 126-126: Updating the
actions/upload-artifactaction to v4 is a good practice to leverage improvements and security patches. Ensure to verify the compatibility and any new features or breaking changes introduced by v4 to avoid potential issues in the workflow.- 128-128: Using dynamically generated names for artifacts, as done here, is a best practice for ensuring traceability and avoiding conflicts. This approach is well-implemented and enhances the workflow's robustness.
Danielius1922
force-pushed
the
adam/feature/extend-endpoint-list
branch
from
20a7c97 to
179de31
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (27)
- .github/workflows/ctt-test.yml (1 hunks)
- .github/workflows/plgd-device-test-with-cfg.yml (2 hunks)
- .github/workflows/plgd-hub-test-with-cfg.yml (2 hunks)
- .github/workflows/sonar-cloud-analysis.yml (2 hunks)
- .github/workflows/unit-test-with-cfg.yml (2 hunks)
- api/cloud/oc_cloud_context.c (5 hunks)
- api/cloud/oc_cloud_context_internal.h (3 hunks)
- api/cloud/oc_cloud_endpoints.c (2 hunks)
- api/cloud/oc_cloud_manager.c (11 hunks)
- api/cloud/oc_cloud_manager_internal.h (2 hunks)
- api/cloud/oc_cloud_rd.c (2 hunks)
- api/cloud/oc_cloud_resource.c (2 hunks)
- api/cloud/oc_cloud_store.c (5 hunks)
- api/cloud/rd_client.c (2 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (10 hunks)
- api/oc_storage.c (1 hunks)
- api/oc_swupdate.c (1 hunks)
- api/plgd/plgd_time.c (4 hunks)
- messaging/coap/coap.c (2 hunks)
- messaging/coap/observe.c (1 hunks)
- messaging/coap/transactions.c (1 hunks)
- port/linux/ipadapter.c (2 hunks)
- port/windows/network_addresses.c (1 hunks)
- security/oc_acl.c (4 hunks)
- security/oc_certs.c (1 hunks)
- security/oc_pstat.c (2 hunks)
- security/oc_tls.c (9 hunks)
Files not reviewed due to errors (1)
- (no review received)
Files skipped from review as they are similar to previous changes (15)
- .github/workflows/ctt-test.yml
- .github/workflows/plgd-device-test-with-cfg.yml
- .github/workflows/plgd-hub-test-with-cfg.yml
- .github/workflows/sonar-cloud-analysis.yml
- .github/workflows/unit-test-with-cfg.yml
- api/cloud/oc_cloud_context.c
- api/cloud/oc_cloud_context_internal.h
- api/cloud/oc_cloud_endpoints.c
- api/cloud/oc_cloud_manager.c
- api/cloud/oc_cloud_manager_internal.h
- api/cloud/oc_cloud_rd.c
- api/cloud/oc_cloud_resource.c
- api/cloud/oc_cloud_store.c
- api/cloud/rd_client.c
- api/cloud/unittest/cloud_manager_test.cpp
Additional comments: 22
port/windows/network_addresses.c (1)
- 147-154: The addition of a debug log message to skip certain addresses based on conditions related to address type and DNS eligibility is a good practice for improving observability and aiding in debugging. It's important to ensure that the log message provides enough context to be useful. This change is guarded by a debug flag, which is appropriate to avoid performance impacts in production builds.
api/oc_storage.c (1)
- 216-219: The addition of a debug log message and data printing within a conditional block, controlled by the
OC_DBG_IS_ENABLEDmacro, is a good practice for improving debugging capabilities. It's important that these additions are wrapped in a conditional compilation block to ensure they do not affect production performance. The structured printing of data can be very useful for understanding the flow of data during debugging.messaging/coap/transactions.c (1)
- 279-283: The addition of a debug message to print the endpoint string within preprocessor directives (
// GCOVR_EXCL_STARTand// GCOVR_EXCL_STOP) is a good practice for enhancing debugging capabilities. It's important to ensure that debug messages are informative and do not leak sensitive information. This change is appropriately managed with preprocessor directives to indicate its purpose for debugging.security/oc_pstat.c (2)
- 344-349: The addition of debug log statements in the
oc_pstat_handle_statefunction provides valuable insights during the debugging process, especially when dealing with state transitions in security configurations. It's important that these log statements are wrapped in a debug flag to ensure they are only included in debug builds. This change enhances the ability to diagnose issues related to state transitions.- 571-575: The addition of debug log statements in the
oc_sec_decode_pstatfunction aids in debugging, particularly when decoding security configurations. Ensuring that these log statements are conditionally compiled is crucial for maintaining performance in production builds. This change enhances the ability to diagnose issues related to decoding processes.api/plgd/plgd_time.c (2)
- 209-215: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [212-232]
Similar to the previous comment, the debug log statements for calculating and comparing the plgd-time with the system time are useful for debugging purposes. Ensure to wrap these debug log statements with a check for
OC_DBG_IS_ENABLEDto optimize the code for production builds where debug logging might be disabled.+ #if OC_DBG_IS_ENABLED // GCOVR_EXCL_START #define RFC3339_BUFFER_SIZE 64 double to_micros = (10000000 / (double)OC_CLOCK_SECOND); char lst_ts[RFC3339_BUFFER_SIZE] = { 0 }; oc_clock_encode_time_rfc3339(pt.store.last_synced_time, lst_ts, sizeof(lst_ts)); OC_DBG("calculating plgd-time: last_synced_time=%s, update_time=%ldus, " "current_time=%ldus, elapsed_time=%ldus", lst_ts, (long)(pt.update_time * to_micros), (long)(cur * to_micros), (long)(elapsed * to_micros)); char pt_ts[RFC3339_BUFFER_SIZE] = { 0 }; oc_clock_encode_time_rfc3339(ptime, pt_ts, sizeof(pt_ts)); oc_clock_time_t time = oc_clock_time(); char ts[RFC3339_BUFFER_SIZE] = { 0 }; oc_clock_encode_time_rfc3339(time, ts, sizeof(ts)); long diff = (long)((double)(time - ptime) / (double)OC_CLOCK_SECOND); OC_DBG("calculated plgd-time: %s, system time: %s, diff: %lds", pt_ts, ts, diff); // GCOVR_EXCL_STOP + #endif /* OC_DBG_IS_ENABLED */
- 253-256: The debug log statement for setting the plgd-time status is wrapped correctly with the
OC_DBG_IS_ENABLEDcheck. This is a good practice for optimizing the code for production builds where debug logging might be disabled.api/oc_swupdate.c (1)
- 276-281: The addition of a debug log message to print the scheduled update time in RFC3339 format is a useful enhancement for debugging purposes. It's correctly wrapped in a conditional compilation block to ensure it's only included in debug builds, which is good practice.
Please ensure the
oc_clock_encode_time_rfc3339function is thoroughly tested in various scenarios to guarantee the timestamp is always correctly formatted.port/linux/ipadapter.c (5)
- 878-884: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [881-896]
The addition of
GCOVR_EXCL_STARTandGCOVR_EXCL_STOPmarkers around the debug logging block is a good practice for excluding debug code from coverage metrics. This ensures that the coverage report accurately reflects the test coverage of functional code, excluding debug or logging statements that don't necessarily need to be covered by tests.
- 878-884: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
It's important to ensure that the license header is up-to-date and accurately reflects the licensing terms for the file. If the project has updated its licensing terms or if this file borrows code from different sources with different licensing, make sure to reflect that accurately in the header.
- 878-884: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
Given the complexity and critical nature of the connectivity functionality, consider adding more comprehensive comments throughout the file to explain the purpose and behavior of functions, especially those that involve network operations, error handling, and conditional compilation. This will improve maintainability and make it easier for new contributors to understand the codebase.
- 878-884: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
For functions that perform network operations, ensure thorough error handling and logging. This includes checking return values of system calls and handling different error conditions appropriately. It's crucial for debugging and maintaining a stable and robust connectivity layer.
- 878-884: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
Consider the performance implications of the current implementation, especially in areas where the code interacts with the network or performs potentially blocking operations. Evaluate the use of non-blocking I/O, efficient data structures, and algorithms to ensure the scalability and responsiveness of the connectivity layer.
security/oc_acl.c (2)
- 780-786: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [783-795]
The addition of debug statements within
oc_sec_add_new_aceis well-implemented. They are correctly wrapped in#if OC_DBG_IS_ENABLEDto ensure they are only included in debug builds, and the use ofOC_DBGfor logging is consistent with the rest of the file. This enhances the maintainability and debuggability of the code without impacting release build performance.
- 827-833: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [830-844]
The addition of debug statements within
oc_sec_add_new_ace_resis consistent with best practices and the rest of the file. They are wrapped in#if OC_DBG_IS_ENABLEDand useOC_DBGfor logging, which ensures that these logs are only included in debug builds. This approach maintains the code's performance in release builds while improving its debuggability.messaging/coap/coap.c (2)
- 502-508: Adding debug messages using
COAP_DBGwithin#if OC_DBG_IS_ENABLEDblocks is a good practice for conditional compilation of debug code. This ensures that the debug messages are only included when debugging is enabled, reducing the footprint in production builds.- 726-732: The addition of debug messages for displaying the ETag value in hexadecimal format is useful for debugging purposes. Using
oc_conv_byte_array_to_hex_stringto convert the ETag byte array to a hex string before logging ensures readability and consistency in debug outputs.security/oc_tls.c (5)
- 468-474: The implementation of
oc_tls_peer_allocatecorrectly allocates and initializes a new TLS peer structure, ensuring all fields are set to appropriate default values. The direct use ofmemcpyfor copying the endpoint structure is based on the assumption thatoc_endpoint_tis trivially copyable, which holds true in the current context.- 2166-2178: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2169-3032]
The
oc_tls_init_contextfunction correctly initializes the necessary mbedTLS structures for the TLS context, including entropy, DRBG, and cookie contexts. It properly handles initialization failures and ensures that an entropy source is added, which is crucial for the randomness of cryptographic operations.
- 2166-2178: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2169-3032]
The
oc_tls_shutdownfunction properly cleans up the TLS context by freeing allocated resources for TLS peers, identity certificates, CA certificates, and mbedTLS contexts, ensuring comprehensive cleanup and preventing memory leaks.
- 2166-2178: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2169-3032]
The
oc_tls_send_messagefunction correctly handles TLS connection states by checking for an existing connection and attempting to initialize one if necessary before sending a message. This ensures that messages are only sent over an established TLS connection, which is crucial for maintaining secure communication.
- 2166-2178: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [2169-3032]
The
oc_tls_recv_messagefunction appropriately processes incoming TLS messages by retrieving or creating a TLS peer for the message's endpoint and adding the message to the peer's receive queue. This ensures that incoming messages are processed in the context of the correct TLS peer, maintaining the integrity of the TLS communication.
Danielius1922
force-pushed
the
adam/feature/extend-endpoint-list
branch
from
179de31 to
2960e27
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (29)
- .github/workflows/ctt-test.yml (1 hunks)
- .github/workflows/plgd-device-test-with-cfg.yml (2 hunks)
- .github/workflows/plgd-hub-test-with-cfg.yml (2 hunks)
- .github/workflows/sonar-cloud-analysis.yml (2 hunks)
- .github/workflows/unit-test-with-cfg.yml (2 hunks)
- api/cloud/oc_cloud_context.c (5 hunks)
- api/cloud/oc_cloud_context_internal.h (3 hunks)
- api/cloud/oc_cloud_endpoints.c (2 hunks)
- api/cloud/oc_cloud_manager.c (11 hunks)
- api/cloud/oc_cloud_manager_internal.h (2 hunks)
- api/cloud/oc_cloud_rd.c (2 hunks)
- api/cloud/oc_cloud_resource.c (2 hunks)
- api/cloud/oc_cloud_store.c (5 hunks)
- api/cloud/rd_client.c (2 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (10 hunks)
- api/cloud/unittest/cloud_resource_test.cpp (2 hunks)
- api/cloud/unittest/cloud_test.cpp (39 hunks)
- api/oc_storage.c (1 hunks)
- api/oc_swupdate.c (1 hunks)
- api/plgd/plgd_time.c (4 hunks)
- messaging/coap/coap.c (2 hunks)
- messaging/coap/observe.c (1 hunks)
- messaging/coap/transactions.c (1 hunks)
- port/linux/ipadapter.c (2 hunks)
- port/windows/network_addresses.c (1 hunks)
- security/oc_acl.c (4 hunks)
- security/oc_certs.c (1 hunks)
- security/oc_pstat.c (2 hunks)
- security/oc_tls.c (9 hunks)
Files skipped from review as they are similar to previous changes (29)
- .github/workflows/ctt-test.yml
- .github/workflows/plgd-device-test-with-cfg.yml
- .github/workflows/plgd-hub-test-with-cfg.yml
- .github/workflows/sonar-cloud-analysis.yml
- .github/workflows/unit-test-with-cfg.yml
- api/cloud/oc_cloud_context.c
- api/cloud/oc_cloud_context_internal.h
- api/cloud/oc_cloud_endpoints.c
- api/cloud/oc_cloud_manager.c
- api/cloud/oc_cloud_manager_internal.h
- api/cloud/oc_cloud_rd.c
- api/cloud/oc_cloud_resource.c
- api/cloud/oc_cloud_store.c
- api/cloud/rd_client.c
- api/cloud/unittest/cloud_manager_test.cpp
- api/cloud/unittest/cloud_resource_test.cpp
- api/cloud/unittest/cloud_test.cpp
- api/oc_storage.c
- api/oc_swupdate.c
- api/plgd/plgd_time.c
- messaging/coap/coap.c
- messaging/coap/observe.c
- messaging/coap/transactions.c
- port/linux/ipadapter.c
- port/windows/network_addresses.c
- security/oc_acl.c
- security/oc_certs.c
- security/oc_pstat.c
- security/oc_tls.c
Danielius1922
force-pushed
the
adam/feature/extend-endpoint-list
branch
from
2960e27 to
b8197ff
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (29)
- .github/workflows/ctt-test.yml (1 hunks)
- .github/workflows/plgd-device-test-with-cfg.yml (2 hunks)
- .github/workflows/plgd-hub-test-with-cfg.yml (2 hunks)
- .github/workflows/sonar-cloud-analysis.yml (2 hunks)
- .github/workflows/unit-test-with-cfg.yml (2 hunks)
- api/cloud/oc_cloud_context.c (5 hunks)
- api/cloud/oc_cloud_context_internal.h (3 hunks)
- api/cloud/oc_cloud_endpoints.c (2 hunks)
- api/cloud/oc_cloud_manager.c (11 hunks)
- api/cloud/oc_cloud_manager_internal.h (2 hunks)
- api/cloud/oc_cloud_rd.c (2 hunks)
- api/cloud/oc_cloud_resource.c (2 hunks)
- api/cloud/oc_cloud_store.c (5 hunks)
- api/cloud/rd_client.c (2 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (10 hunks)
- api/cloud/unittest/cloud_resource_test.cpp (2 hunks)
- api/cloud/unittest/cloud_test.cpp (39 hunks)
- api/oc_storage.c (1 hunks)
- api/oc_swupdate.c (1 hunks)
- api/plgd/plgd_time.c (4 hunks)
- messaging/coap/coap.c (2 hunks)
- messaging/coap/observe.c (1 hunks)
- messaging/coap/transactions.c (1 hunks)
- port/linux/ipadapter.c (2 hunks)
- port/windows/network_addresses.c (1 hunks)
- security/oc_acl.c (4 hunks)
- security/oc_certs.c (1 hunks)
- security/oc_pstat.c (2 hunks)
- security/oc_tls.c (9 hunks)
Files skipped from review as they are similar to previous changes (29)
- .github/workflows/ctt-test.yml
- .github/workflows/plgd-device-test-with-cfg.yml
- .github/workflows/plgd-hub-test-with-cfg.yml
- .github/workflows/sonar-cloud-analysis.yml
- .github/workflows/unit-test-with-cfg.yml
- api/cloud/oc_cloud_context.c
- api/cloud/oc_cloud_context_internal.h
- api/cloud/oc_cloud_endpoints.c
- api/cloud/oc_cloud_manager.c
- api/cloud/oc_cloud_manager_internal.h
- api/cloud/oc_cloud_rd.c
- api/cloud/oc_cloud_resource.c
- api/cloud/oc_cloud_store.c
- api/cloud/rd_client.c
- api/cloud/unittest/cloud_manager_test.cpp
- api/cloud/unittest/cloud_resource_test.cpp
- api/cloud/unittest/cloud_test.cpp
- api/oc_storage.c
- api/oc_swupdate.c
- api/plgd/plgd_time.c
- messaging/coap/coap.c
- messaging/coap/observe.c
- messaging/coap/transactions.c
- port/linux/ipadapter.c
- port/windows/network_addresses.c
- security/oc_acl.c
- security/oc_certs.c
- security/oc_pstat.c
- security/oc_tls.c
Danielius1922
force-pushed
the
adam/feature/extend-endpoint-list
branch
from
b8197ff to
f00522d
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (29)
- .github/workflows/ctt-test.yml (1 hunks)
- .github/workflows/plgd-device-test-with-cfg.yml (2 hunks)
- .github/workflows/plgd-hub-test-with-cfg.yml (2 hunks)
- .github/workflows/sonar-cloud-analysis.yml (2 hunks)
- .github/workflows/unit-test-with-cfg.yml (2 hunks)
- api/cloud/oc_cloud_context.c (5 hunks)
- api/cloud/oc_cloud_context_internal.h (3 hunks)
- api/cloud/oc_cloud_endpoints.c (2 hunks)
- api/cloud/oc_cloud_manager.c (11 hunks)
- api/cloud/oc_cloud_manager_internal.h (2 hunks)
- api/cloud/oc_cloud_rd.c (2 hunks)
- api/cloud/oc_cloud_resource.c (2 hunks)
- api/cloud/oc_cloud_store.c (5 hunks)
- api/cloud/rd_client.c (2 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (11 hunks)
- api/cloud/unittest/cloud_resource_test.cpp (2 hunks)
- api/cloud/unittest/cloud_test.cpp (39 hunks)
- api/oc_storage.c (1 hunks)
- api/oc_swupdate.c (1 hunks)
- api/plgd/plgd_time.c (4 hunks)
- messaging/coap/coap.c (2 hunks)
- messaging/coap/observe.c (1 hunks)
- messaging/coap/transactions.c (1 hunks)
- port/linux/ipadapter.c (2 hunks)
- port/windows/network_addresses.c (1 hunks)
- security/oc_acl.c (4 hunks)
- security/oc_certs.c (1 hunks)
- security/oc_pstat.c (2 hunks)
- security/oc_tls.c (9 hunks)
Files skipped from review as they are similar to previous changes (29)
- .github/workflows/ctt-test.yml
- .github/workflows/plgd-device-test-with-cfg.yml
- .github/workflows/plgd-hub-test-with-cfg.yml
- .github/workflows/sonar-cloud-analysis.yml
- .github/workflows/unit-test-with-cfg.yml
- api/cloud/oc_cloud_context.c
- api/cloud/oc_cloud_context_internal.h
- api/cloud/oc_cloud_endpoints.c
- api/cloud/oc_cloud_manager.c
- api/cloud/oc_cloud_manager_internal.h
- api/cloud/oc_cloud_rd.c
- api/cloud/oc_cloud_resource.c
- api/cloud/oc_cloud_store.c
- api/cloud/rd_client.c
- api/cloud/unittest/cloud_manager_test.cpp
- api/cloud/unittest/cloud_resource_test.cpp
- api/cloud/unittest/cloud_test.cpp
- api/oc_storage.c
- api/oc_swupdate.c
- api/plgd/plgd_time.c
- messaging/coap/coap.c
- messaging/coap/observe.c
- messaging/coap/transactions.c
- port/linux/ipadapter.c
- port/windows/network_addresses.c
- security/oc_acl.c
- security/oc_certs.c
- security/oc_pstat.c
- security/oc_tls.c
Danielius1922
force-pushed
the
adam/feature/extend-endpoint-list
branch
from
f00522d to
5e19f09
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (29)
- .github/workflows/ctt-test.yml (1 hunks)
- .github/workflows/plgd-device-test-with-cfg.yml (2 hunks)
- .github/workflows/plgd-hub-test-with-cfg.yml (2 hunks)
- .github/workflows/sonar-cloud-analysis.yml (2 hunks)
- .github/workflows/unit-test-with-cfg.yml (2 hunks)
- api/cloud/oc_cloud_context.c (5 hunks)
- api/cloud/oc_cloud_context_internal.h (3 hunks)
- api/cloud/oc_cloud_endpoints.c (2 hunks)
- api/cloud/oc_cloud_manager.c (11 hunks)
- api/cloud/oc_cloud_manager_internal.h (2 hunks)
- api/cloud/oc_cloud_rd.c (2 hunks)
- api/cloud/oc_cloud_resource.c (2 hunks)
- api/cloud/oc_cloud_store.c (5 hunks)
- api/cloud/rd_client.c (2 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (11 hunks)
- api/cloud/unittest/cloud_resource_test.cpp (2 hunks)
- api/cloud/unittest/cloud_test.cpp (39 hunks)
- api/oc_storage.c (1 hunks)
- api/oc_swupdate.c (1 hunks)
- api/plgd/plgd_time.c (4 hunks)
- messaging/coap/coap.c (2 hunks)
- messaging/coap/observe.c (1 hunks)
- messaging/coap/transactions.c (1 hunks)
- port/linux/ipadapter.c (2 hunks)
- port/windows/network_addresses.c (1 hunks)
- security/oc_acl.c (4 hunks)
- security/oc_certs.c (1 hunks)
- security/oc_pstat.c (2 hunks)
- security/oc_tls.c (9 hunks)
Files skipped from review as they are similar to previous changes (29)
- .github/workflows/ctt-test.yml
- .github/workflows/plgd-device-test-with-cfg.yml
- .github/workflows/plgd-hub-test-with-cfg.yml
- .github/workflows/sonar-cloud-analysis.yml
- .github/workflows/unit-test-with-cfg.yml
- api/cloud/oc_cloud_context.c
- api/cloud/oc_cloud_context_internal.h
- api/cloud/oc_cloud_endpoints.c
- api/cloud/oc_cloud_manager.c
- api/cloud/oc_cloud_manager_internal.h
- api/cloud/oc_cloud_rd.c
- api/cloud/oc_cloud_resource.c
- api/cloud/oc_cloud_store.c
- api/cloud/rd_client.c
- api/cloud/unittest/cloud_manager_test.cpp
- api/cloud/unittest/cloud_resource_test.cpp
- api/cloud/unittest/cloud_test.cpp
- api/oc_storage.c
- api/oc_swupdate.c
- api/plgd/plgd_time.c
- messaging/coap/coap.c
- messaging/coap/observe.c
- messaging/coap/transactions.c
- port/linux/ipadapter.c
- port/windows/network_addresses.c
- security/oc_acl.c
- security/oc_certs.c
- security/oc_pstat.c
- security/oc_tls.c
Danielius1922
force-pushed
the
adam/feature/extend-endpoint-list
branch
from
5e19f09 to
36a649e
Compare
ocf-conformance-test-tool
bot
removed
the
OCF Conformance Testing
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (42)
- .github/workflows/ctt-test.yml (1 hunks)
- .github/workflows/plgd-device-test-with-cfg.yml (2 hunks)
- .github/workflows/plgd-hub-test-with-cfg.yml (2 hunks)
- .github/workflows/sonar-cloud-analysis.yml (2 hunks)
- .github/workflows/unit-test-with-cfg.yml (2 hunks)
- api/cloud/oc_cloud.c (14 hunks)
- api/cloud/oc_cloud_context.c (5 hunks)
- api/cloud/oc_cloud_context_internal.h (3 hunks)
- api/cloud/oc_cloud_endpoints.c (2 hunks)
- api/cloud/oc_cloud_endpoints_internal.h (2 hunks)
- api/cloud/oc_cloud_internal.h (4 hunks)
- api/cloud/oc_cloud_manager.c (11 hunks)
- api/cloud/oc_cloud_manager_internal.h (2 hunks)
- api/cloud/oc_cloud_rd.c (2 hunks)
- api/cloud/oc_cloud_resource.c (2 hunks)
- api/cloud/oc_cloud_store.c (5 hunks)
- api/cloud/rd_client.c (2 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (11 hunks)
- api/cloud/unittest/cloud_resource_test.cpp (2 hunks)
- api/cloud/unittest/cloud_test.cpp (39 hunks)
- api/oc_rep.c (2 hunks)
- api/oc_rep_internal.h (1 hunks)
- api/oc_storage.c (1 hunks)
- api/oc_swupdate.c (1 hunks)
- api/plgd/plgd_time.c (4 hunks)
- include/oc_cloud.h (1 hunks)
- include/oc_rep.h (1 hunks)
- messaging/coap/coap.c (2 hunks)
- messaging/coap/observe.c (1 hunks)
- messaging/coap/transactions.c (1 hunks)
- port/linux/ipadapter.c (2 hunks)
- port/windows/network_addresses.c (1 hunks)
- security/oc_acl.c (4 hunks)
- security/oc_certs.c (1 hunks)
- security/oc_pstat.c (2 hunks)
- security/oc_tls.c (9 hunks)
- swig/swig_interfaces/oc_cloud.i (1 hunks)
- swig/swig_interfaces/oc_endpoint_address.i (3 hunks)
- swig/swig_interfaces/oc_rep.i (1 hunks)
- util/oc_endpoint_address.c (5 hunks)
- util/oc_endpoint_address_internal.h (3 hunks)
- util/unittest/endpoint_address_test.cpp (4 hunks)
Files skipped from review as they are similar to previous changes (42)
- .github/workflows/ctt-test.yml
- .github/workflows/plgd-device-test-with-cfg.yml
- .github/workflows/plgd-hub-test-with-cfg.yml
- .github/workflows/sonar-cloud-analysis.yml
- .github/workflows/unit-test-with-cfg.yml
- api/cloud/oc_cloud.c
- api/cloud/oc_cloud_context.c
- api/cloud/oc_cloud_context_internal.h
- api/cloud/oc_cloud_endpoints.c
- api/cloud/oc_cloud_endpoints_internal.h
- api/cloud/oc_cloud_internal.h
- api/cloud/oc_cloud_manager.c
- api/cloud/oc_cloud_manager_internal.h
- api/cloud/oc_cloud_rd.c
- api/cloud/oc_cloud_resource.c
- api/cloud/oc_cloud_store.c
- api/cloud/rd_client.c
- api/cloud/unittest/cloud_manager_test.cpp
- api/cloud/unittest/cloud_resource_test.cpp
- api/cloud/unittest/cloud_test.cpp
- api/oc_rep.c
- api/oc_rep_internal.h
- api/oc_storage.c
- api/oc_swupdate.c
- api/plgd/plgd_time.c
- include/oc_cloud.h
- include/oc_rep.h
- messaging/coap/coap.c
- messaging/coap/observe.c
- messaging/coap/transactions.c
- port/linux/ipadapter.c
- port/windows/network_addresses.c
- security/oc_acl.c
- security/oc_certs.c
- security/oc_pstat.c
- security/oc_tls.c
- swig/swig_interfaces/oc_cloud.i
- swig/swig_interfaces/oc_endpoint_address.i
- swig/swig_interfaces/oc_rep.i
- util/oc_endpoint_address.c
- util/oc_endpoint_address_internal.h
- util/unittest/endpoint_address_test.cpp
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (9)
- api/cloud/oc_cloud_context.c (6 hunks)
- api/cloud/oc_cloud_manager.c (14 hunks)
- api/cloud/oc_cloud_manager_internal.h (3 hunks)
- api/cloud/oc_cloud_schedule.c (1 hunks)
- api/cloud/oc_cloud_schedule_internal.h (1 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (12 hunks)
- port/esp32/main/CMakeLists.txt (1 hunks)
- port/windows/vs2015/IoTivity-lite.vcxproj (2 hunks)
- port/windows/vs2015/IoTivity-lite.vcxproj.filters (4 hunks)
Files skipped from review as they are similar to previous changes (3)
- api/cloud/oc_cloud_context.c
- api/cloud/oc_cloud_manager.c
- api/cloud/oc_cloud_manager_internal.h
Additional comments: 21
api/cloud/oc_cloud_schedule_internal.h (4)
- 37-37: The function
cloud_retry_is_overlacks documentation. It's important to have comments explaining the purpose and usage of functions, especially for internal APIs that might not be as self-explanatory as public APIs.Consider adding a brief comment above the function declaration to explain its purpose and any important details about its parameters or return value.
- 40-40: The function
oc_cloud_set_retry_timeoutsis publicly exposed but lacks documentation. For maintainability and ease of use, it's beneficial to document the purpose, parameters, and expected behavior of this function.Please add a comment describing the function's purpose, the expected format and meaning of the
timeoutsarray, and thesizeparameter.
- 42-42: The function
oc_cloud_get_retry_timeoutsis missing documentation. Documenting functions, especially those that are part of an internal API, is crucial for maintainability and clarity.It would be helpful to add a comment explaining the purpose of this function, the expected usage of the
timeoutsandsizeparameters, and the return value.
- 44-44: The function
cloud_schedule_actionlacks documentation. Given its role in scheduling cloud actions, providing clear documentation is essential for future maintenance and understanding of the code.Please add a comment detailing the function's purpose, describing each parameter, and explaining the return value.
api/cloud/oc_cloud_schedule.c (5)
- 84-89: The function
cloud_retry_is_overuses a logical OR to check if the retry count has exceeded or if the specific retry timeout is set to 0. This logic is sound, but it's important to ensure thatg_retry_timeout_msis always correctly initialized to prevent unexpected behavior.Ensure that
g_retry_timeout_msis properly initialized and that there are no paths where uninitialized values could lead to incorrect behavior.
- 91-108: The function
default_schedule_actioncalculates the delay and timeout for retry actions. The use of random delay to prevent simultaneous requests from multiple devices is a good strategy. However, there's a potential issue with how the delay is calculated.Consider the edge case where
oc_random_value() % *delaymight result in a 0 delay, especially when*delayis small. It might be beneficial to ensure a minimum non-zero delay to avoid immediate retries in certain scenarios.
- 111-134: The function
on_action_response_set_retryhandles the retry logic for cloud actions. It correctly delegates to either a custom or default scheduling action. However, the logging within this function could potentially leak sensitive information.Ensure that logging does not expose sensitive information such as tokens or user identifiers. Consider sanitizing or omitting sensitive details from logs.
- 137-161: The function
cloud_schedule_actionschedules cloud actions with retry logic. It properly increments retry counts and handles different actions. However, there's a lack of error handling for the case whereoc_reset_delayed_callback_msfails.Verify that
oc_reset_delayed_callback_mshas adequate internal error handling and consider what action should be taken if it fails to schedule the callback.
- 164-171: The function
oc_cloud_set_schedule_actionallows setting a custom schedule action callback. This is a flexible design choice, but it's important to ensure that the callback anduser_dataare managed correctly to avoid memory leaks or dangling pointers.Ensure that there's a clear ownership model for
user_dataand that any memory allocated for callbacks is properly freed when no longer needed.port/esp32/main/CMakeLists.txt (1)
- 139-139: The addition of
oc_cloud_schedule.cto the list of source files in theCMakeLists.txtfor the ESP32 port is correctly done under theCONFIG_CLOUDcondition. This ensures that the file is only included in builds where cloud functionality is enabled.This change is correctly scoped and follows the existing pattern for conditional compilation based on configuration options.
api/cloud/unittest/cloud_manager_test.cpp (7)
- 48-48: The constant
kTestServeris introduced to specify a test server address. This is a good practice for unit tests, as it centralizes the server address used across multiple tests, making it easier to update if needed.The use of a constant for the test server address improves maintainability and readability of the test code.
- 65-65: The call to
oc_cloud_set_retry_timeoutswithin theSetUpmethod ofTestCloudManageris a good practice, as it ensures that each test starts with a known retry timeout configuration. This contributes to the consistency and reliability of the tests.Initializing test preconditions within the
SetUpmethod is a best practice for unit testing.
- 70-79: The initialization of
m_context.storewithoc_cloud_store_initializeand the subsequent addition of a server endpoint usingoc_endpoint_addresses_addare correctly done. However, it's important to ensure that theoc_endpoint_addresses_addfunction properly handles the addition of endpoints to prevent potential memory leaks or corruption.Verify that
oc_endpoint_addresses_addand related functions properly manage memory and handle edge cases to prevent leaks or corruption.
- 91-92: Resetting retry timeouts and schedule actions in the
TearDownmethod is a good practice to ensure that each test case starts with a clean state. This helps prevent tests from interfering with each other.Properly cleaning up test state in the
TearDownmethod is crucial for reliable unit testing.
- 344-344: The call to
oc_cloud_set_retry_timeoutswith a single timeout value incloud_manager_select_next_server_on_retrytest case is a good way to control the retry behavior for the purpose of the test. It simplifies the test setup and makes the test behavior more predictable.Adjusting retry timeouts for specific test cases is a good practice for controlling test flow and isolating test conditions.
- 512-540: The test
cloud_manager_calculate_refresh_token_expirationcorrectly verifies the behavior of the token expiration calculation under different conditions. This is important for ensuring that the cloud manager refreshes tokens at appropriate times.Testing various scenarios for token expiration calculation is essential for ensuring robust and reliable cloud connectivity management.
- 512-540: The test cases for parsing register data and handling redirect responses are comprehensive and cover various scenarios, including invalid payloads. This thorough testing is crucial for ensuring that the cloud manager correctly handles responses from the cloud service.
Comprehensive testing of response parsing and handling ensures that the cloud manager behaves correctly under various conditions.
port/windows/vs2015/IoTivity-lite.vcxproj (2)
- 216-225: The addition of new internal headers related to cloud functionality is noted. Ensure these headers are correctly referenced in the corresponding source files and that they are included in the project's include path to avoid compilation issues.
- 350-350: The addition of the
oc_cloud_schedule.csource file is observed. It's important to verify that this and other newly added source files are correctly integrated into the build process and that they do not introduce any build errors or warnings.port/windows/vs2015/IoTivity-lite.vcxproj.filters (2)
- 443-445: The addition of
oc_cloud_schedule.cto theCore\cloudfilter is appropriate and aligns with the PR's objectives of enhancing cloud connectivity and management components.- 775-802: The addition of internal cloud header files to the
Core\cloudfilter is appropriate and supports the PR's objectives of improving cloud connectivity and management components. Organizing these files under a specific filter aids in project maintainability.
Danielius1922
force-pushed
the
adam/feature/extend-endpoint-list
branch
from
49febce to
69263c2
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (9)
- api/cloud/oc_cloud_context.c (6 hunks)
- api/cloud/oc_cloud_manager.c (14 hunks)
- api/cloud/oc_cloud_manager_internal.h (3 hunks)
- api/cloud/oc_cloud_schedule.c (1 hunks)
- api/cloud/oc_cloud_schedule_internal.h (1 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (13 hunks)
- port/esp32/main/CMakeLists.txt (1 hunks)
- port/windows/vs2015/IoTivity-lite.vcxproj (2 hunks)
- port/windows/vs2015/IoTivity-lite.vcxproj.filters (4 hunks)
Files skipped from review as they are similar to previous changes (9)
- api/cloud/oc_cloud_context.c
- api/cloud/oc_cloud_manager.c
- api/cloud/oc_cloud_manager_internal.h
- api/cloud/oc_cloud_schedule.c
- api/cloud/oc_cloud_schedule_internal.h
- api/cloud/unittest/cloud_manager_test.cpp
- port/esp32/main/CMakeLists.txt
- port/windows/vs2015/IoTivity-lite.vcxproj
- port/windows/vs2015/IoTivity-lite.vcxproj.filters
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (8)
- api/cloud/oc_cloud.c (13 hunks)
- api/cloud/oc_cloud_context.c (6 hunks)
- api/cloud/oc_cloud_context_internal.h (4 hunks)
- api/cloud/oc_cloud_internal.h (3 hunks)
- api/cloud/oc_cloud_schedule.c (1 hunks)
- api/cloud/oc_cloud_schedule_internal.h (1 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (13 hunks)
- api/cloud/unittest/cloud_test.cpp (42 hunks)
Files skipped from review as they are similar to previous changes (7)
- api/cloud/oc_cloud.c
- api/cloud/oc_cloud_context.c
- api/cloud/oc_cloud_context_internal.h
- api/cloud/oc_cloud_internal.h
- api/cloud/oc_cloud_schedule.c
- api/cloud/oc_cloud_schedule_internal.h
- api/cloud/unittest/cloud_test.cpp
Additional comments not posted (6)
api/cloud/unittest/cloud_manager_test.cpp (6)
49-49: Ensure the server address format inkTestServeris correct and consistent with the project's standards.
71-78: Verify that the callback functioncloud_context_on_server_changeproperly handles server changes, especially in the context of usingkTestServer.
79-80: Confirm that the updated assertions withkTestServeraccurately test the intended functionality without reducing test coverage.
113-120: Ensure that the refactored function calls tooc_cloud_manager_*maintain the same functionality and side effects as the originalcloud_manager_*calls.
353-430: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [345-374]
Carefully review the timing calculation adjustments in
cloud_manager_select_next_server_on_retryto ensure they enhance scheduling efficiency without introducing timing-related bugs.
513-541: Ensure the new tests for calculating refresh token expiration times accurately test the functionality and cover edge cases.
Danielius1922
force-pushed
the
adam/feature/extend-endpoint-list
branch
from
84c3736 to
50deb3e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (8)
- api/cloud/oc_cloud.c (13 hunks)
- api/cloud/oc_cloud_context.c (6 hunks)
- api/cloud/oc_cloud_context_internal.h (4 hunks)
- api/cloud/oc_cloud_internal.h (3 hunks)
- api/cloud/oc_cloud_schedule.c (1 hunks)
- api/cloud/oc_cloud_schedule_internal.h (1 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (13 hunks)
- api/cloud/unittest/cloud_test.cpp (42 hunks)
Files skipped from review as they are similar to previous changes (8)
- api/cloud/oc_cloud.c
- api/cloud/oc_cloud_context.c
- api/cloud/oc_cloud_context_internal.h
- api/cloud/oc_cloud_internal.h
- api/cloud/oc_cloud_schedule.c
- api/cloud/oc_cloud_schedule_internal.h
- api/cloud/unittest/cloud_manager_test.cpp
- api/cloud/unittest/cloud_test.cpp
Danielius1922
force-pushed
the
adam/feature/extend-endpoint-list
branch
from
50deb3e to
292c17f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (9)
- api/cloud/oc_cloud.c (13 hunks)
- api/cloud/oc_cloud_context.c (6 hunks)
- api/cloud/oc_cloud_context_internal.h (4 hunks)
- api/cloud/oc_cloud_internal.h (3 hunks)
- api/cloud/oc_cloud_schedule.c (1 hunks)
- api/cloud/oc_cloud_schedule_internal.h (1 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (13 hunks)
- api/cloud/unittest/cloud_test.cpp (42 hunks)
- security/oc_pstat.c (4 hunks)
Files skipped from review as they are similar to previous changes (9)
- api/cloud/oc_cloud.c
- api/cloud/oc_cloud_context.c
- api/cloud/oc_cloud_context_internal.h
- api/cloud/oc_cloud_internal.h
- api/cloud/oc_cloud_schedule.c
- api/cloud/oc_cloud_schedule_internal.h
- api/cloud/unittest/cloud_manager_test.cpp
- api/cloud/unittest/cloud_test.cpp
- security/oc_pstat.c
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (2)
- .github/workflows/sonar-cloud-analysis.yml (3 hunks)
- api/cloud/oc_cloud_schedule.c (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- .github/workflows/sonar-cloud-analysis.yml
- api/cloud/oc_cloud_schedule.c
Danielius1922
force-pushed
the
adam/feature/extend-endpoint-list
branch
from
3ab5fb3 to
5bef75b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (2)
- .github/workflows/sonar-cloud-analysis.yml (3 hunks)
- api/cloud/oc_cloud_schedule.c (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- .github/workflows/sonar-cloud-analysis.yml
- api/cloud/oc_cloud_schedule.c
Danielius1922
force-pushed
the
adam/feature/extend-endpoint-list
branch
from
5bef75b to
e84e0fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (5)
- .github/workflows/sonar-cloud-analysis.yml (3 hunks)
- api/cloud/oc_cloud.c (13 hunks)
- api/cloud/oc_cloud_schedule.c (1 hunks)
- api/cloud/unittest/cloud_test.cpp (42 hunks)
- api/oc_ri_server.c (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- .github/workflows/sonar-cloud-analysis.yml
- api/cloud/oc_cloud.c
- api/cloud/oc_cloud_schedule.c
- api/cloud/unittest/cloud_test.cpp
Additional comments not posted (1)
api/oc_ri_server.c (1)
37-37: Consider adding a comment explaining why the presence of a duplicate delete resource callback is treated as an error, to provide context for future maintainers.
Danielius1922
force-pushed
the
adam/feature/extend-endpoint-list
branch
from
e84e0fc to
9ed7fa9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (5)
- .github/workflows/sonar-cloud-analysis.yml (3 hunks)
- api/cloud/oc_cloud.c (13 hunks)
- api/cloud/oc_cloud_schedule.c (1 hunks)
- api/cloud/unittest/cloud_test.cpp (42 hunks)
- api/oc_ri_server.c (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- .github/workflows/sonar-cloud-analysis.yml
- api/cloud/oc_cloud_schedule.c
- api/cloud/unittest/cloud_test.cpp
- api/oc_ri_server.c
Additional comments not posted (8)
api/cloud/oc_cloud.c (8)
154-159: The functionoc_cloud_reset_endpointproperly resets the cloud endpoint and its state. Ensure that all necessary cleanup operations are performed before resetting the endpoint to avoid resource leaks or inconsistent states.
193-209: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [185-206]
Ensure that the
oc_cloud_endpoint_addresses_setfunction properly handles all possible error conditions and that its return value is always checked where it's called.This verification is necessary to ensure robust error handling in the cloud configuration update process.
Verification successful
The
oc_cloud_endpoint_addresses_setfunction, as defined inapi/cloud/oc_cloud_endpoints.c, does handle error conditions. It checks for initialization failures, asserts the type of servers, and validates the presence of necessary fields within the server representations. If any of these checks fail, the function logs an error and, in some cases, returnsfalseto indicate failure. This demonstrates that the function is designed to handle various error conditions.Moreover, the usage of
oc_cloud_endpoint_addresses_setinapi/cloud/oc_cloud.c, as indicated by the review comment, shows that its return value is checked. The function's return value is used to determine the success or failure of setting cloud endpoint addresses, and if it fails, the calling function can react accordingly.Based on the provided implementation details and usage context, it can be concluded that the
oc_cloud_endpoint_addresses_setfunction properly handles error conditions and its return value is checked where it's called. This aligns with the requirements mentioned in the review comment.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that oc_cloud_endpoint_addresses_set handles errors correctly ast-grep --lang c --pattern $'oc_cloud_endpoint_addresses_set($_, $_, $_, $_)'Length of output: 79
Script:
#!/bin/bash # Verify the existence of oc_cloud_endpoint_addresses_set function rg "oc_cloud_endpoint_addresses_set" --files-with-matchesLength of output: 176
Script:
#!/bin/bash # Inspect the implementation of oc_cloud_endpoint_addresses_set cat api/cloud/oc_cloud_endpoints.c # Check if the return value of oc_cloud_endpoint_addresses_set is checked in oc_cloud.c rg "oc_cloud_endpoint_addresses_set" api/cloud/oc_cloud.cLength of output: 4674
255-261: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [245-269]
The sequence of operations in
oc_cloud_provision_conf_resourcefor resetting the endpoint, reinitializing the store, and updating the cloud configuration is logically structured. However, ensure that each step is atomic and can be rolled back in case of failure to maintain system stability.
301-316: Similar to the previous comment, the sequence of operations inoc_cloud_update_by_resourcefor resetting the endpoint, reinitializing the store, and updating the cloud configuration is well-structured. Again, ensure atomicity and rollback capabilities for each step.
454-455: Initialization ofoc_cloud_registration_contextwithinoc_cloud_manager_startis crucial for managing cloud registration contexts. Verify that this initialization is paired with appropriate deinitialization logic to prevent memory leaks.This verification ensures that resources are properly managed and released.
Verification successful
The search results indicate that the
oc_cloud_registration_context_deinitfunction is indeed present and used within the codebase, specifically in theapi/cloud/oc_cloud_context.cfile. This file contains multiple calls tooc_cloud_registration_context_deinit, ensuring that theoc_cloud_registration_contextis deinitialized appropriately in various scenarios, such as during the deinitialization of the cloud context, clearing of the cloud context, and within the initialization function itself to reset the context before reinitializing it.Given this evidence, it can be concluded that the initialization of
oc_cloud_registration_contextwithinoc_cloud_manager_startis paired with appropriate deinitialization logic to prevent memory leaks, as per the original review comment's concern.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify deinitialization of oc_cloud_registration_context ast-grep --lang c --pattern $'oc_cloud_registration_context_deinit($_)'Length of output: 72
Script:
#!/bin/bash # Broaden the search to find any potential deinitialization logic related to oc_cloud_registration_context # This includes looking for destructors or cleanup functions that might be related to cloud context management. rg "oc_cloud_registration_context" --context 5Length of output: 10375
490-491: The cleanup logic inoc_cloud_manager_stopis comprehensive, including resetting the endpoint and deinitializing the registration context. Ensure that all resources allocated during the cloud manager's operation are released here to avoid memory leaks.
498-531: The functionscloud_deinit_devices,cloud_init_device, andcloud_init_devicesprovide structured mechanisms for initializing and deinitializing cloud-related resources across devices. Ensure that these functions are called appropriately during system startup and shutdown to manage resources efficiently.
539-541: The initialization logic inoc_cloud_initincludes a fallback mechanism to remove the delete resource callback if device initialization fails. This is a good practice for maintaining system stability in case of initialization errors.
Danielius1922
force-pushed
the
adam/feature/extend-endpoint-list
branch
from
9ed7fa9 to
5a909ea
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (8)
- .github/workflows/sonar-cloud-analysis.yml (3 hunks)
- api/cloud/oc_cloud.c (13 hunks)
- api/cloud/oc_cloud_schedule.c (1 hunks)
- api/cloud/unittest/cloud_test.cpp (42 hunks)
- api/oc_ri_server.c (1 hunks)
- util/oc_endpoint_address.c (6 hunks)
- util/oc_endpoint_address_internal.h (4 hunks)
- util/unittest/endpoint_address_test.cpp (4 hunks)
Files skipped from review as they are similar to previous changes (8)
- .github/workflows/sonar-cloud-analysis.yml
- api/cloud/oc_cloud.c
- api/cloud/oc_cloud_schedule.c
- api/cloud/unittest/cloud_test.cpp
- api/oc_ri_server.c
- util/oc_endpoint_address.c
- util/oc_endpoint_address_internal.h
- util/unittest/endpoint_address_test.cpp
Danielius1922
force-pushed
the
adam/feature/extend-endpoint-list
branch
from
5a909ea to
24dd7fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (13)
- .github/workflows/sonar-cloud-analysis.yml (3 hunks)
- api/cloud/oc_cloud.c (13 hunks)
- api/cloud/oc_cloud_context.c (6 hunks)
- api/cloud/oc_cloud_context_internal.h (4 hunks)
- api/cloud/oc_cloud_manager.c (14 hunks)
- api/cloud/oc_cloud_schedule.c (1 hunks)
- api/cloud/oc_cloud_schedule_internal.h (1 hunks)
- api/cloud/unittest/cloud_context_test.cpp (2 hunks)
- api/cloud/unittest/cloud_test.cpp (42 hunks)
- api/oc_ri_server.c (1 hunks)
- util/oc_endpoint_address.c (6 hunks)
- util/oc_endpoint_address_internal.h (4 hunks)
- util/unittest/endpoint_address_test.cpp (4 hunks)
Files skipped from review as they are similar to previous changes (11)
- .github/workflows/sonar-cloud-analysis.yml
- api/cloud/oc_cloud_context.c
- api/cloud/oc_cloud_context_internal.h
- api/cloud/oc_cloud_manager.c
- api/cloud/oc_cloud_schedule.c
- api/cloud/oc_cloud_schedule_internal.h
- api/cloud/unittest/cloud_test.cpp
- api/oc_ri_server.c
- util/oc_endpoint_address.c
- util/oc_endpoint_address_internal.h
- util/unittest/endpoint_address_test.cpp
Additional comments not posted (8)
api/cloud/unittest/cloud_context_test.cpp (3)
39-68: The test caseInitfor cloud context initialization and deinitialization in insecure builds is well-implemented and correctly handles both dynamic and static allocation scenarios.
70-73: The test caseDeinitcorrectly tests the deinitialization of a null cloud context, ensuring graceful handling of null inputs.
36-77: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [74-529]
The remaining test cases covering status change callbacks, device retrieval, and token management are well-implemented and provide good coverage for cloud context functionality.
api/cloud/oc_cloud.c (5)
81-83: The simplification of the conditional logic for settingctx->store.cpsusing a ternary operation improves readability and conciseness.
151-157: The introduction ofoc_cloud_reset_endpointcentralizes the cloud endpoint reset logic, improving code clarity and reusability.
191-207: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [183-204]
The updates to
cloud_set_cloudconfto handle multiple cloud server addresses enhance cloud connectivity flexibility.
235-235: The validation of the server ID format inoc_cloud_provision_conf_resourceimproves the robustness of the cloud configuration process.
495-529: The introduction ofcloud_init_device,cloud_deinit_devices, andcloud_init_devicesfor structured device initialization and deinitialization enhances code maintainability and scalability.
Danielius1922
force-pushed
the
adam/feature/extend-endpoint-list
branch
from
24dd7fb to
803e3f0
Compare
ocf-conformance-test-tool
bot
removed
the
OCF Conformance Testing
There was a problem hiding this comment.
Actionable comments posted: 2
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (49)
- .github/workflows/ctt-test.yml (1 hunks)
- .github/workflows/plgd-device-test-with-cfg.yml (2 hunks)
- .github/workflows/plgd-hub-test-with-cfg.yml (2 hunks)
- .github/workflows/sonar-cloud-analysis.yml (3 hunks)
- .github/workflows/unit-test-with-cfg.yml (2 hunks)
- api/cloud/oc_cloud.c (13 hunks)
- api/cloud/oc_cloud_context.c (6 hunks)
- api/cloud/oc_cloud_context_internal.h (4 hunks)
- api/cloud/oc_cloud_endpoints.c (2 hunks)
- api/cloud/oc_cloud_endpoints_internal.h (2 hunks)
- api/cloud/oc_cloud_internal.h (3 hunks)
- api/cloud/oc_cloud_manager.c (14 hunks)
- api/cloud/oc_cloud_manager_internal.h (3 hunks)
- api/cloud/oc_cloud_rd.c (2 hunks)
- api/cloud/oc_cloud_resource.c (2 hunks)
- api/cloud/oc_cloud_schedule.c (1 hunks)
- api/cloud/oc_cloud_schedule_internal.h (1 hunks)
- api/cloud/oc_cloud_store.c (5 hunks)
- api/cloud/rd_client.c (2 hunks)
- api/cloud/unittest/cloud_context_test.cpp (2 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (13 hunks)
- api/cloud/unittest/cloud_resource_test.cpp (2 hunks)
- api/cloud/unittest/cloud_test.cpp (42 hunks)
- api/oc_rep.c (2 hunks)
- api/oc_rep_internal.h (1 hunks)
- api/oc_ri_server.c (1 hunks)
- api/oc_storage.c (1 hunks)
- api/oc_swupdate.c (1 hunks)
- api/plgd/plgd_time.c (4 hunks)
- include/oc_cloud.h (1 hunks)
- include/oc_rep.h (1 hunks)
- messaging/coap/coap.c (2 hunks)
- messaging/coap/observe.c (1 hunks)
- messaging/coap/transactions.c (1 hunks)
- port/esp32/main/CMakeLists.txt (1 hunks)
- port/linux/ipadapter.c (2 hunks)
- port/windows/network_addresses.c (1 hunks)
- port/windows/vs2015/IoTivity-lite.vcxproj (2 hunks)
- port/windows/vs2015/IoTivity-lite.vcxproj.filters (4 hunks)
- security/oc_acl.c (4 hunks)
- security/oc_certs.c (1 hunks)
- security/oc_pstat.c (4 hunks)
- security/oc_tls.c (9 hunks)
- swig/swig_interfaces/oc_cloud.i (1 hunks)
- swig/swig_interfaces/oc_endpoint_address.i (3 hunks)
- swig/swig_interfaces/oc_rep.i (1 hunks)
- util/oc_endpoint_address.c (6 hunks)
- util/oc_endpoint_address_internal.h (4 hunks)
- util/unittest/endpoint_address_test.cpp (4 hunks)
Files skipped from review as they are similar to previous changes (47)
- .github/workflows/ctt-test.yml
- .github/workflows/plgd-device-test-with-cfg.yml
- .github/workflows/plgd-hub-test-with-cfg.yml
- .github/workflows/sonar-cloud-analysis.yml
- .github/workflows/unit-test-with-cfg.yml
- api/cloud/oc_cloud_context.c
- api/cloud/oc_cloud_context_internal.h
- api/cloud/oc_cloud_endpoints.c
- api/cloud/oc_cloud_endpoints_internal.h
- api/cloud/oc_cloud_internal.h
- api/cloud/oc_cloud_manager.c
- api/cloud/oc_cloud_manager_internal.h
- api/cloud/oc_cloud_rd.c
- api/cloud/oc_cloud_resource.c
- api/cloud/oc_cloud_schedule.c
- api/cloud/oc_cloud_schedule_internal.h
- api/cloud/oc_cloud_store.c
- api/cloud/rd_client.c
- api/cloud/unittest/cloud_context_test.cpp
- api/cloud/unittest/cloud_manager_test.cpp
- api/cloud/unittest/cloud_resource_test.cpp
- api/cloud/unittest/cloud_test.cpp
- api/oc_rep.c
- api/oc_rep_internal.h
- api/oc_ri_server.c
- api/oc_storage.c
- api/oc_swupdate.c
- api/plgd/plgd_time.c
- include/oc_cloud.h
- include/oc_rep.h
- messaging/coap/coap.c
- messaging/coap/observe.c
- messaging/coap/transactions.c
- port/esp32/main/CMakeLists.txt
- port/linux/ipadapter.c
- port/windows/network_addresses.c
- port/windows/vs2015/IoTivity-lite.vcxproj
- port/windows/vs2015/IoTivity-lite.vcxproj.filters
- security/oc_acl.c
- security/oc_certs.c
- security/oc_pstat.c
- security/oc_tls.c
- swig/swig_interfaces/oc_cloud.i
- swig/swig_interfaces/oc_endpoint_address.i
- swig/swig_interfaces/oc_rep.i
- util/oc_endpoint_address_internal.h
- util/unittest/endpoint_address_test.cpp
Additional comments not posted (10)
util/oc_endpoint_address.c (3)
213-214: Ensure that the callback and its data are valid before setting them inoc_endpoint_addresses_init. Adding a null check or validation step could improve robustness.
259-266: The functionoc_endpoint_addresses_set_on_selected_changecorrectly updates the callback and its data. However, consider validating the callback function pointer for null before setting it to prevent potential issues with invoking a null pointer.
440-447: The functionoc_endpoint_addresses_select_nextcorrectly selects the next endpoint address. However, ensure that the list of addresses is not empty before attempting to select the next address to avoid unexpected behavior.api/cloud/oc_cloud.c (7)
81-83: The conditional assignment for settingctx->store.cpsbased onctx->store.statusis clear and concise. This change improves readability and simplifies the logic.
152-157: The functionoc_cloud_reset_endpointcorrectly resets the cloud endpoint and its state. This refactoring improves code clarity and reusability. Ensure thatctx->cloud_epis not null before dereferencing it to avoid potential null pointer dereferences.
194-202: When setting the cloud endpoint addresses incloud_set_cloudconf, ensure that eitherdata->ci_serveris not null ordata->ci_serversis not empty before proceeding. This validation can prevent potential issues with empty or invalid cloud endpoint configurations.
235-235: Validating the server ID format before proceeding with the cloud configuration update is crucial to prevent potential issues with invalid IDs. This validation step enhances the robustness of the cloud configuration process.
243-245: Resetting the cloud endpoint and reinitializing the cloud store before updating the cloud configuration is a good practice. It ensures that the cloud context is in a clean state before applying new configurations.
299-301: Similar to previous comments, resetting the cloud endpoint and reinitializing the cloud store before updating the cloud configuration via resource update is a prudent approach. It ensures consistency and reliability in the cloud configuration process.
495-529: The introduction ofcloud_deinit_devices,cloud_init_device, andcloud_init_devicesfunctions for handling device initialization and deinitialization in a structured manner is a significant improvement. It enhances the modularity and maintainability of the code related to cloud device management.
Danielius1922
force-pushed
the
adam/feature/extend-endpoint-list
branch
from
803e3f0 to
4772822
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (49)
- .github/workflows/ctt-test.yml (1 hunks)
- .github/workflows/plgd-device-test-with-cfg.yml (2 hunks)
- .github/workflows/plgd-hub-test-with-cfg.yml (2 hunks)
- .github/workflows/sonar-cloud-analysis.yml (3 hunks)
- .github/workflows/unit-test-with-cfg.yml (2 hunks)
- api/cloud/oc_cloud.c (13 hunks)
- api/cloud/oc_cloud_context.c (6 hunks)
- api/cloud/oc_cloud_context_internal.h (4 hunks)
- api/cloud/oc_cloud_endpoints.c (2 hunks)
- api/cloud/oc_cloud_endpoints_internal.h (2 hunks)
- api/cloud/oc_cloud_internal.h (3 hunks)
- api/cloud/oc_cloud_manager.c (14 hunks)
- api/cloud/oc_cloud_manager_internal.h (3 hunks)
- api/cloud/oc_cloud_rd.c (2 hunks)
- api/cloud/oc_cloud_resource.c (2 hunks)
- api/cloud/oc_cloud_schedule.c (1 hunks)
- api/cloud/oc_cloud_schedule_internal.h (1 hunks)
- api/cloud/oc_cloud_store.c (5 hunks)
- api/cloud/rd_client.c (2 hunks)
- api/cloud/unittest/cloud_context_test.cpp (2 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (13 hunks)
- api/cloud/unittest/cloud_resource_test.cpp (2 hunks)
- api/cloud/unittest/cloud_test.cpp (42 hunks)
- api/oc_rep.c (2 hunks)
- api/oc_rep_internal.h (1 hunks)
- api/oc_ri_server.c (1 hunks)
- api/oc_storage.c (1 hunks)
- api/oc_swupdate.c (1 hunks)
- api/plgd/plgd_time.c (4 hunks)
- include/oc_cloud.h (1 hunks)
- include/oc_rep.h (1 hunks)
- messaging/coap/coap.c (2 hunks)
- messaging/coap/observe.c (1 hunks)
- messaging/coap/transactions.c (1 hunks)
- port/esp32/main/CMakeLists.txt (1 hunks)
- port/linux/ipadapter.c (2 hunks)
- port/windows/network_addresses.c (1 hunks)
- port/windows/vs2015/IoTivity-lite.vcxproj (2 hunks)
- port/windows/vs2015/IoTivity-lite.vcxproj.filters (4 hunks)
- security/oc_acl.c (4 hunks)
- security/oc_certs.c (1 hunks)
- security/oc_pstat.c (4 hunks)
- security/oc_tls.c (9 hunks)
- swig/swig_interfaces/oc_cloud.i (1 hunks)
- swig/swig_interfaces/oc_endpoint_address.i (3 hunks)
- swig/swig_interfaces/oc_rep.i (1 hunks)
- util/oc_endpoint_address.c (12 hunks)
- util/oc_endpoint_address_internal.h (4 hunks)
- util/unittest/endpoint_address_test.cpp (4 hunks)
Files skipped from review as they are similar to previous changes (47)
- .github/workflows/ctt-test.yml
- .github/workflows/plgd-device-test-with-cfg.yml
- .github/workflows/plgd-hub-test-with-cfg.yml
- .github/workflows/sonar-cloud-analysis.yml
- .github/workflows/unit-test-with-cfg.yml
- api/cloud/oc_cloud_context.c
- api/cloud/oc_cloud_context_internal.h
- api/cloud/oc_cloud_endpoints.c
- api/cloud/oc_cloud_endpoints_internal.h
- api/cloud/oc_cloud_internal.h
- api/cloud/oc_cloud_manager.c
- api/cloud/oc_cloud_manager_internal.h
- api/cloud/oc_cloud_rd.c
- api/cloud/oc_cloud_resource.c
- api/cloud/oc_cloud_schedule.c
- api/cloud/oc_cloud_schedule_internal.h
- api/cloud/oc_cloud_store.c
- api/cloud/rd_client.c
- api/cloud/unittest/cloud_context_test.cpp
- api/cloud/unittest/cloud_manager_test.cpp
- api/cloud/unittest/cloud_resource_test.cpp
- api/cloud/unittest/cloud_test.cpp
- api/oc_rep.c
- api/oc_rep_internal.h
- api/oc_ri_server.c
- api/oc_storage.c
- api/oc_swupdate.c
- api/plgd/plgd_time.c
- include/oc_rep.h
- messaging/coap/coap.c
- messaging/coap/observe.c
- messaging/coap/transactions.c
- port/esp32/main/CMakeLists.txt
- port/linux/ipadapter.c
- port/windows/network_addresses.c
- port/windows/vs2015/IoTivity-lite.vcxproj
- port/windows/vs2015/IoTivity-lite.vcxproj.filters
- security/oc_acl.c
- security/oc_certs.c
- security/oc_pstat.c
- security/oc_tls.c
- swig/swig_interfaces/oc_cloud.i
- swig/swig_interfaces/oc_endpoint_address.i
- swig/swig_interfaces/oc_rep.i
- util/oc_endpoint_address.c
- util/oc_endpoint_address_internal.h
- util/unittest/endpoint_address_test.cpp
Additional comments not posted (10)
api/cloud/oc_cloud.c (9)
81-83: Consider simplifying the conditional assignment for settingctx->store.cpsto enhance readability.- ctx->store.cps = (ctx->store.status & OC_CLOUD_REGISTERED) != 0 - ? OC_CPS_REGISTERED - : OC_CPS_READYTOREGISTER;This change aligns with previous feedback and maintains consistency in handling conditional assignments.
152-157: Introducingoc_cloud_reset_endpointfunction enhances modularity and reusability by encapsulating the logic for resetting the cloud endpoint. This is a positive change that contributes to cleaner and more maintainable code.
191-207: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [183-202]
Ensure the server ID (
data->sid) is validated before proceeding with cloud configuration updates. This validation is crucial for maintaining the integrity of cloud configurations and preventing potential issues with invalid IDs.
253-259: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [243-267]
The sequence of operations in
oc_cloud_provision_conf_resourcefunction, including resetting the endpoint, reinitializing the store, and updating the cloud configuration, is logically structured. However, consider adding error handling for the operations that can fail, such asoc_cloud_reset_endpointandoc_cloud_store_reinitialize, to ensure robustness.
299-314: Similar to the previous comment, the sequence of operations inoc_cloud_update_by_resourcefunction is well-organized. Again, adding error handling for operations that can potentially fail would enhance the robustness of the code.
452-453: Initializing the registration context withinoc_cloud_manager_startfunction is a good practice as it ensures that the context is properly set up before starting the cloud manager. This contributes to the reliability of the cloud connectivity features.
488-489: The cleanup process inoc_cloud_manager_stopfunction, including resetting the endpoint and deinitializing the registration context, is thorough and ensures that resources are properly released. This is a good practice for resource management.
496-529: The functionscloud_deinit_devices,cloud_init_device, andcloud_init_devicesprovide a structured approach to initializing and deinitializing devices. This structured approach enhances the maintainability and scalability of the code related to device management.
537-539: The conditional logic inoc_cloud_initfor initializing devices and handling failure cases is clear and concise. This approach ensures that the cloud functionality is properly initialized and that resources are cleaned up in case of initialization failure.include/oc_cloud.h (1)
141-141: The reformatting of the comment for theoc_cloud_get_contextfunction improves the readability of the documentation. This is a positive change that contributes to better code documentation practices.
|
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores