util: create oc_endpoint_addresses_t #611
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 (fe4c893), label this PR with |
Danielius1922
force-pushed
the
adam/feature/endpoint-list
branch
2 times, most recently
from
6b46f65 to
a3c3d38
Compare
WalkthroughThe updates primarily focus on transitioning from managing cloud endpoints directly to handling endpoint addresses, reflecting a broader and more flexible approach to cloud server communication. This transition involves renaming functions and types, removing outdated includes, and introducing new functionality for endpoint address management. The changes span across the API, internal headers, utility functions, and test suites, aiming to enhance cloud connectivity features within the IoTivity Lite framework. 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: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
tools/doxygen.iniis excluded by:!**/*.ini
Files selected for processing (30)
- api/cloud/oc_cloud.c (3 hunks)
- api/cloud/oc_cloud_context.c (3 hunks)
- api/cloud/oc_cloud_context_internal.h (1 hunks)
- api/cloud/oc_cloud_endpoints.c (1 hunks)
- api/cloud/oc_cloud_endpoints_internal.h (1 hunks)
- api/cloud/oc_cloud_manager.c (4 hunks)
- api/cloud/oc_cloud_resource.c (3 hunks)
- api/cloud/oc_cloud_store.c (6 hunks)
- api/cloud/oc_cloud_store_internal.h (3 hunks)
- api/cloud/unittest/cloud_context_test.cpp (1 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (7 hunks)
- api/cloud/unittest/cloud_resource_test.cpp (1 hunks)
- api/cloud/unittest/cloud_store_test.cpp (17 hunks)
- api/cloud/unittest/cloud_test.cpp (10 hunks)
- include/oc_api.h (2 hunks)
- include/oc_cloud.h (4 hunks)
- port/esp32/main/CMakeLists.txt (2 hunks)
- swig/Makefile (2 hunks)
- swig/java_lang/oc_iotivity_lite_jni.c (5 hunks)
- swig/java_lang/oc_iotivity_lite_jni.h (2 hunks)
- swig/swig_interfaces/iotivity.swg (1 hunks)
- swig/swig_interfaces/oc_cloud.i (2 hunks)
- swig/swig_interfaces/oc_endpoint.i (1 hunks)
- swig/swig_interfaces/oc_endpoint_address.i (1 hunks)
- util/oc_endpoint_address.c (1 hunks)
- util/oc_endpoint_address.h (1 hunks)
- util/oc_endpoint_address_internal.h (1 hunks)
- util/oc_features.h (1 hunks)
- util/oc_memb.h (1 hunks)
- util/unittest/endpoint_address_test.cpp (1 hunks)
Files not reviewed due to errors (1)
- oc_api.h (no review received)
Files skipped from review due to trivial changes (1)
- swig/swig_interfaces/oc_endpoint.i
Additional comments: 119
api/cloud/oc_cloud_endpoints.c (1)
- 35-45: The implementation of
oc_cloud_endpoint_addresses_initcorrectly initializes the cloud endpoint addresses using theoc_endpoint_addresses_initfunction. However, ensure thatOC_CLOUD_MAX_ENDPOINT_ADDRESSESis properly defined and reflects the intended maximum number of endpoint addresses per device. This is crucial for maintaining scalability and avoiding potential memory issues.api/cloud/oc_cloud_endpoints_internal.h (2)
- 34-37: The definition of
OC_CLOUD_MAX_ENDPOINT_ADDRESSESas(2 * OC_MAX_NUM_DEVICES)seems logical, assuming each device is expected to have a maximum of two endpoint addresses. Ensure that this assumption aligns with the intended use cases and scalability requirements of the IoTivity Lite framework.- 40-44: The declaration of
oc_cloud_endpoint_addresses_initis clear and follows the expected function signature pattern. It's good to see the use ofOC_NONNULL(1)to enforce non-null arguments for the first parameter, enhancing the function's robustness.util/oc_endpoint_address.h (2)
- 47-48: The function
oc_endpoint_address_uricorrectly retrieves the URI of an endpoint address. It's good practice to ensure that the returnedoc_string_t*is either properly handled or documented to indicate ownership and lifecycle management, especially in a library context.- 52-53: The function
oc_endpoint_address_set_idprovides a straightforward way to set the ID of an endpoint address. It's important to ensure that theidparameter is validated elsewhere in the code to prevent invalid UUIDs from being set.swig/swig_interfaces/oc_endpoint_address.i (1)
- 29-33: Ignoring the functions
oc_endpoint_address_uri,oc_endpoint_address_set_id, andoc_endpoint_address_idin the SWIG interface and providing Java-friendly methods instead is a good approach for maintaining language-specific idioms. Ensure that the Java methodsgetUri,setId, andgetIdare thoroughly tested to confirm their correct interaction with the native code.util/oc_features.h (1)
- 87-89: The introduction of the
OC_HAS_FEATURE_ENDPOINT_ADDRESS_LISTdirective based on theOC_CLOUDdefinition is a clean way to conditionally enable features related to endpoint address management. This approach enhances modularity and allows for easier feature toggling based on the build configuration.api/cloud/oc_cloud_store_internal.h (3)
- 24-24: Replacing the inclusion of
"api/cloud/oc_cloud_endpoint_internal.h"with"api/cloud/oc_cloud_endpoints_internal.h"aligns with the refactor's goal of generalizing endpoint address management. Ensure that all necessary functionalities from the replaced header are either preserved or suitably adapted in the new header.- 42-42: Changing the type from
oc_cloud_endpoints_ttooc_endpoint_addresses_tin theoc_cloud_store_tstructure is consistent with the refactor's objective. This change should be carefully reviewed in the context of the entire codebase to ensure that all interactions with this structure are updated accordingly.- 58-61: The update to the
oc_cloud_store_initializefunction signature, replacingon_selected_change_fn_twithon_selected_endpoint_address_change_fn_t, is a necessary adjustment following the refactor. It's important to verify that all calls to this function are updated to use the new callback type.api/cloud/oc_cloud_context_internal.h (1)
- 19-24: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [1-1]
The removal of the inclusion of
"api/cloud/oc_cloud_endpoint_internal.h"is consistent with the refactor's direction towards a more generalized endpoint address management system. Ensure that any functionalities previously relied upon from the removed header are either no longer needed or have been suitably replaced within the new system.api/cloud/unittest/cloud_context_test.cpp (1)
- 111-111: The modification in the unit test to use
oc_endpoint_addresses_reinitinstead ofoc_cloud_endpoints_reinitaligns with the refactor towards generalized endpoint address management. It's crucial to ensure that the unit test covers all relevant scenarios to verify the correct behavior of the new endpoint address management functionalities.api/cloud/oc_cloud_resource.c (4)
- 32-32: The inclusion of
"util/oc_endpoint_address_internal.h"is necessary for the updated endpoint address management functionality. Ensure that this new dependency is properly documented and that the header file is available in the expected location.- 71-71: Replacing
oc_cloud_endpoint_selected_addresswithoc_endpoint_addresses_selected_urialigns with the refactor's goal of generalizing endpoint management. Verify that the new function provides equivalent or improved functionality and that it is thoroughly tested.Verification successful
The search results confirm the existence of the
oc_endpoint_addresses_selected_urifunction within the codebase, specifically in the filesutil/oc_endpoint_address_internal.handutil/oc_endpoint_address.c. This indicates that the function is indeed implemented and part of the project's utility functions related to endpoint address management.Moreover, the function is utilized in various parts of the code, including the
api/clouddirectory, which aligns with the context of the original review comment regarding its use in cloud-related functionalities. The presence of this function inutil/unittest/endpoint_address_test.cppwith multiple test cases also confirms that it has been subjected to testing. These tests appear to cover various scenarios, including selecting and verifying the selected URI, which suggests thorough testing of the function's functionality.Given this information, it can be concluded that the replacement of
oc_cloud_endpoint_selected_addresswithoc_endpoint_addresses_selected_uriis supported by the implementation of the new function and its testing. This aligns with the refactor's goal of generalizing endpoint management and indicates that the new function provides equivalent or improved functionality with adequate test coverage.* 74-75: The replacement of `oc_cloud_endpoint_selected_id` with `oc_endpoint_addresses_selected_id` is consistent with the refactor's objectives. Ensure that the new function correctly handles all scenarios previously covered by the old function, including error handling and edge cases.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation and test coverage of `oc_endpoint_addresses_selected_uri`. ast-grep --lang c --pattern $'oc_endpoint_addresses_selected_uri($_)'Length of output: 70
Script:
#!/bin/bash # Search for the function `oc_endpoint_addresses_selected_uri` in the codebase to verify its existence and find related test coverage. rg "oc_endpoint_addresses_selected_uri" --context 2Length of output: 6168
Verification successful
Given the lack of output from the initial script, it's essential to broaden the search to verify the existence and implementation of
oc_endpoint_addresses_selected_id. Let's use ripgrep (rg) to search for the function across the codebase, which might provide insights into its implementation and usage, including test coverage if available.
The verification process has confirmed the existence of the function
oc_endpoint_addresses_selected_idwithin the codebase, specifically inutil/oc_endpoint_address.c, and its usage inapi/cloud/oc_cloud_resource.cas well as in multiple unit tests (util/unittest/endpoint_address_test.cpp). This suggests that the function is implemented and has test coverage, which is consistent with the refactor's objectives as outlined in the review comment. However, without a detailed examination of the implementation and the tests, it's not possible to fully assess the comprehensiveness of the test coverage, especially regarding error handling and edge cases. Based on the information available, the replacement ofoc_cloud_endpoint_selected_idwithoc_endpoint_addresses_selected_idappears to be correctly implemented and tested, aligning with the refactor's goals.* 84-84: Replacing `oc_cloud_endpoints_encode` with `oc_endpoint_addresses_encode` is a key part of the refactor, ensuring that endpoint addresses are managed in a generalized manner. It's crucial to ensure that this function is robust, efficiently handles all necessary encoding scenarios, and is covered by comprehensive tests.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation and test coverage of `oc_endpoint_addresses_selected_id`. ast-grep --lang c --pattern $'oc_endpoint_addresses_selected_id($_)'Length of output: 69
Script:
#!/bin/bash # Broaden the search for `oc_endpoint_addresses_selected_id` across the codebase. rg 'oc_endpoint_addresses_selected_id'Length of output: 1180
swig/java_lang/oc_iotivity_lite_jni.h (1)
- 158-161: The conditional declaration of
cls_OCCloudContextandcls_OCCloudHandlerbased on theOC_CLOUDmacro is a good practice for enabling or disabling cloud-related features based on the build configuration. This change ensures that cloud-related classes are only available when the cloud feature is enabled, which can help in reducing the binary size and avoiding unnecessary dependencies in builds where cloud functionality is not required. However, it's important to ensure that any Java code relying on these classes is also conditionally compiled or executed to prevent runtime errors due to missing classes.util/oc_endpoint_address_internal.h (2)
- 65-83: The
oc_endpoint_addresses_initfunction is crucial for initializing the endpoint addresses structure. It's well-documented and takes into account all necessary parameters for initialization, including a default URI and identity. This function lays the foundation for managing endpoint addresses in a flexible manner. Ensure that error handling is robust, especially in scenarios where memory allocation might fail for theoc_memb_tpool or when adding the default endpoint address.- 200-204: The
oc_endpoint_addresses_encodefunction is an important addition for encoding endpoint addresses into a CborEncoder. The inclusion of theskipIfSingleAndSelectedparameter is a thoughtful design choice, allowing for optimization in scenarios where there's only one endpoint address, and it's already selected. This can reduce unnecessary encoding operations and payload size. Ensure thorough testing of this function, particularly with edge cases involving empty lists or multiple addresses with one selected.api/cloud/oc_cloud_context.c (4)
- 33-33: Including
util/oc_endpoint_address_internal.his necessary for accessing the new endpoint address management functionalities. This change aligns with the refactor's goal to generalize endpoint management. Ensure that this include is guarded by theOC_CLOUDmacro if the functionalities within are only relevant when cloud support is enabled.- 182-182: The use of
oc_endpoint_addresses_selected_urito get the server URI represents a shift towards the new generalized endpoint address management approach. This change is consistent with the refactor's objectives and should enhance flexibility in managing cloud server URIs. Ensure that all calls to this function are properly updated to handle the possibility of aNULLreturn value, especially in contexts where the selected endpoint address might not be set.- 317-322: The introduction of
oc_cloud_add_server_addressfunction, which utilizesoc_endpoint_addresses_add, is a key part of integrating the new endpoint address management functionalities. This function simplifies the process of adding new server addresses to the cloud context. It's important to ensure that error handling is robust, particularly for cases where memory allocation for a new endpoint address might fail.- 326-329: The
oc_cloud_remove_server_addressfunction, leveragingoc_endpoint_addresses_remove, is crucial for removing server addresses from the cloud context. This change supports the dynamic management of cloud server addresses. Ensure that the logic correctly handles scenarios where the specified endpoint address is not found in the list, and verify that the removal does not inadvertently affect the selection of the currently active endpoint address.swig/Makefile (2)
- 115-121: The addition of
oc_cloudandoc_endpoint_addressto theSRCvariable whenCLOUDis set to 1 is a logical change that aligns with the PR's objectives of generalizing cloud endpoint management. This ensures that the new files are included in the build process when cloud functionality is enabled, enhancing modularity and maintainability.- 161-164: The conditional exclusion of
OCCloudStatusMask.javaandOCCloudHandler.javafromJAVA_SRCwhenCLOUDis not set to 1 is a prudent approach to manage dependencies based on the build configuration. This ensures that cloud-related Java sources are only included when the cloud functionality is enabled, maintaining consistency with the overall objective of modularizing cloud interactions.port/esp32/main/CMakeLists.txt (2)
- 79-79: The addition of
oc_endpoint_address.cto thesourceslist is consistent with the PR's goal of introducing a generalized approach to endpoint address management. Including this file in the build ensures that the new functionality is integrated into the ESP32 port, supporting the framework's adaptability to various cloud configurations.- 135-135: Renaming
oc_cloud_endpoint.ctooc_cloud_endpoints.creflects the shift towards a more generalized handling of cloud endpoints, as indicated by the PR's objectives. This change suggests an expansion or modification of the original file's scope to accommodate multiple endpoints, aligning with the goal of enhancing cloud interaction flexibility.swig/swig_interfaces/iotivity.swg (1)
- 20-22: The addition of the conditional compilation block for
OC_HAS_FEATURE_ENDPOINT_ADDRESS_LISTbased on theOC_CLOUDmacro is correctly implemented and aligns with the PR's objectives to generalize cloud endpoint management.api/cloud/oc_cloud_store.c (2)
- 79-85: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [82-93]
The transition from
oc_cloud_endpoint_ttooc_endpoint_address_metadata_tand the use of new functions for endpoint address management (oc_endpoint_addresses_encode,oc_endpoint_addresses_deinit,oc_endpoint_addresses_reinit, andoc_endpoint_addresses_add) are correctly implemented and align with the PR's objectives. Ensure that these new functions properly handle memory and error conditions for robustness.
- 255-262: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [249-293]
Verify the error handling and memory management within
oc_endpoint_addresses_deinit,oc_endpoint_addresses_reinit, andoc_endpoint_addresses_addfunctions to ensure they are robust and prevent resource leaks. This is crucial for maintaining the stability and reliability of the endpoint address management functionality.Verification successful
To further verify the error handling and memory management within the
oc_endpoint_addresses_deinit,oc_endpoint_addresses_reinit, andoc_endpoint_addresses_addfunctions, we need to locate and review their implementations. Let's proceed with generating shell scripts to find these function definitions.
The shell script output has located the definitions for
oc_endpoint_addresses_deinitandoc_endpoint_addresses_reinitinutil/oc_endpoint_address_internal.h, but did not provide information foroc_endpoint_addresses_add. To proceed, we need to review the implementations of the located functions and attempt to find the implementation ofoc_endpoint_addresses_add.
The provided output from the shell scripts has successfully located the implementations of
oc_endpoint_addresses_deinit,oc_endpoint_addresses_reinit, andoc_endpoint_addresses_add. These functions are defined inutil/oc_endpoint_address_internal.handutil/oc_endpoint_address.c, as indicated by the output. The definitions and usages of these functions suggest that they include necessary mechanisms for error handling and memory management, as per the initial review comment's concern.
- oc_endpoint_addresses_deinit: This function is designed to deinitialize endpoint addresses, ensuring that resources are properly released.
- oc_endpoint_addresses_reinit: This function deinitializes and then reinitializes endpoint addresses with new parameters, which implies careful handling of resources to prevent leaks.
- oc_endpoint_addresses_add: The implementation details in
util/oc_endpoint_address.cindicate that this function checks for invalid URIs and existing URIs before adding a new endpoint address, which suggests thoughtful error handling and resource management.Given the information from the script output, it appears that the error handling and memory management within these functions are robust and designed to prevent resource leaks, aligning with the initial review comment's recommendation to verify these aspects.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Assuming the implementation of these functions is within the same directory or a closely related one. # Search for the implementation of oc_endpoint_addresses_deinit rg --type c "oc_endpoint_addresses_deinit" api/cloud/ # Search for the implementation of oc_endpoint_addresses_reinit rg --type c "oc_endpoint_addresses_reinit" api/cloud/ # Search for the implementation of oc_endpoint_addresses_add rg --type c "oc_endpoint_addresses_add" api/cloud/Length of output: 863
Script:
#!/bin/bash # Locate the definitions of oc_endpoint_addresses_deinit, oc_endpoint_addresses_reinit, and oc_endpoint_addresses_add # Since these functions are used in the api/cloud directory, their definitions are likely within the same or closely related directories. # Search for the implementation details of these functions across the project to review their error handling and memory management. rg --type c --files-with-matches "oc_endpoint_addresses_deinit" | xargs rg "void oc_endpoint_addresses_deinit" rg --type c --files-with-matches "oc_endpoint_addresses_reinit" | xargs rg "bool oc_endpoint_addresses_reinit" rg --type c --files-with-matches "oc_endpoint_addresses_add" | xargs rg "bool oc_endpoint_addresses_add"Length of output: 538
Script:
#!/bin/bash # Review the implementations of oc_endpoint_addresses_deinit and oc_endpoint_addresses_reinit cat util/oc_endpoint_address_internal.h # Attempt to find the implementation of oc_endpoint_addresses_add, considering it might be defined in a different format or location rg --type c --files-with-matches "oc_endpoint_addresses_add" | xargs rg "oc_endpoint_addresses_add"Length of output: 8598
util/oc_endpoint_address.c (28)
- 35-39: The function
oc_endpoint_address_uricorrectly returns a pointer to the URI string of an endpoint address metadata. It's a simple getter function with no side effects.- 41-45: The function
oc_endpoint_address_set_idcorrectly assigns a UUID to an endpoint address metadata. It's a straightforward setter function.- 47-51: The function
oc_endpoint_address_idcorrectly returns the UUID of an endpoint address metadata. It's a simple getter function.- 53-57: The function
endpoint_address_is_validchecks if the URI length is within acceptable bounds. This is a good validation step to ensure that URIs are not empty and do not exceed the maximum length.- 59-70: The function
endpoint_address_selectcorrectly handles the selection of an endpoint address. It includes a callback mechanism (on_selected_change) to notify about the selection change, which is a good design for asynchronous updates.- 72-99: The function
endpoint_address_allocate_and_addallocates and adds a new endpoint address metadata to the list. It performs validation on the URI and handles memory allocation errors properly. The automatic selection of the first endpoint address added is a thoughtful design choice.- 101-123: The function
oc_endpoint_addresses_initinitializes anoc_endpoint_addresses_tstructure. It correctly sets up the list and handles the optional default URI and ID. The use ofmemsetfor zero-initialization is appropriate.- 125-131: The function
endpoint_address_freecorrectly frees the resources associated with an endpoint address metadata. It ensures that the URI string is freed and the memory block is returned to the pool.- 133-144: The function
oc_endpoint_addresses_deinitcorrectly deinitializes anoc_endpoint_addresses_tstructure by freeing all associated endpoint address metadata. The loop ensures that all items are freed properly.- 146-154: The function
oc_endpoint_addresses_reinitreinitializes anoc_endpoint_addresses_tstructure. It correctly deinitializes the current state before reinitializing with potentially new parameters. This is a useful function for resetting or updating the endpoint addresses.- 156-166: The functions
oc_endpoint_addresses_sizeandoc_endpoint_addresses_is_emptyprovide utility for checking the size and emptiness of the endpoint addresses list. These are straightforward and useful functions.- 168-183: The function
oc_endpoint_addresses_containschecks if a URI is already present in the list of endpoint addresses. It iterates through the list and usesoc_string_view_is_equalfor comparison, which is an efficient way to compare strings.- 186-200: The function
oc_endpoint_addresses_addadds a new endpoint address if it's not already present and the URI is valid. It logs an error message if the URI is invalid or already exists, which is helpful for debugging.- 203-213: The function
endpoint_address_nextprovides a way to get the next endpoint address metadata in the list, considering the current selection. It uses assertions to ensure that the input parameters are valid, which helps catch programming errors in debug builds.- 215-230: The function
oc_endpoint_addresses_removeremoves an endpoint address metadata from the list and frees its resources. It correctly handles the case where the removed address is the currently selected one by selecting the next available address.- 233-243: The function
oc_endpoint_addresses_remove_by_uriremoves an endpoint address by URI. It first finds the address and then removes it, which is a common pattern for collection manipulation. This function simplifies the removal process for callers.- 245-254: The function
oc_endpoint_addresses_clearclears all endpoint addresses from the list and resets the selected address. It correctly frees all resources associated with each address, ensuring no memory leaks.- 256-272: The function
oc_endpoint_addresses_iterateiterates over all endpoint addresses and applies a given function to each. This is a flexible way to perform operations on all addresses, and the early return mechanism allows for short-circuiting the iteration.- 274-289: The function
endpoint_address_matchis a helper function used in finding an endpoint address by URI. It leverages the iteration mechanism provided byoc_endpoint_addresses_iteratefor a concise implementation.- 291-301: The function
oc_endpoint_addresses_findfinds an endpoint address by URI using theendpoint_address_matchhelper. This is an efficient way to locate an address without exposing the internal list structure.- 303-312: The function
oc_endpoint_addresses_selectselects an endpoint address if it exists in the list. It ensures that the address is part of the list before making the selection, which is a necessary validation step.- 314-324: The function
oc_endpoint_addresses_select_by_uriselects an endpoint address by URI. It first finds the address and then selects it, which is a straightforward and effective approach.- 327-335: The function
oc_endpoint_addresses_select_nextselects the next endpoint address in the list. It handles the case where there is no currently selected address, ensuring that the function is safe to call in any state.- 337-343: The function
oc_endpoint_addresses_is_selectedchecks if a given URI is the currently selected endpoint address. It's a simple and useful function for validating the current selection.- 345-352: The function
oc_endpoint_addresses_selected_urireturns the URI of the currently selected endpoint address. It correctly handles the case where there is no selected address, returningNULL.- 354-361: The function
oc_endpoint_addresses_selected_idreturns the UUID of the currently selected endpoint address. Similar tooc_endpoint_addresses_selected_uri, it handles the case where there is no selected address.- 369-390: The function
endpoint_address_encodeencodes an endpoint address metadata into a CBOR map. It correctly handles the encoding of both the URI and the UUID, ensuring that the data is properly represented.- 392-415: The function
oc_endpoint_addresses_encodeencodes the list of endpoint addresses into a CBOR array. It includes an option to skip encoding if there is only one address and it is selected, which can optimize the encoded representation in certain scenarios.api/cloud/unittest/cloud_resource_test.cpp (1)
- 255-255: The change from
oc_cloud_endpoints_cleartooc_endpoint_addresses_clearaligns with the refactor in the cloud component to generalize endpoint address management. This update ensures that the test reflects the new approach to endpoint management.api/cloud/oc_cloud.c (3)
- 40-40: The inclusion of
"util/oc_endpoint_address_internal.h"aligns with the PR's objective to refactor cloud endpoint management. This new header likely contains the definitions and declarations for the newoc_endpoint_addresses_tstructure and related functions.- 117-117: The call to
oc_endpoint_addresses_selected_urirepresents a shift towards the new generalized endpoint address management approach. This function likely retrieves the selected URI from a list of endpoint addresses, which is a more flexible method compared to the previous direct handling of cloud endpoints.- 185-192: The use of
oc_endpoint_addresses_deinitandoc_cloud_endpoint_addresses_initwithincloud_set_cloudconffunction indicates a significant change in how cloud configuration is handled. This approach allows for dynamic reinitialization of cloud server endpoints, enhancing flexibility and maintainability. However, the comment about potential memory deallocation and relocation issues warrants careful consideration to ensure memory safety and avoid potential bugs.Verification successful
Given the limitations, the next step is to gather more information about the implementation of
oc_endpoint_addresses_deinitandoc_cloud_endpoint_addresses_initfunctions, and any related tests or documentation that might provide insights into their memory management practices. Let's proceed with generating shell scripts to find these details.
Based on the implementation details and their usage in tests, the memory management practices for
oc_endpoint_addresses_deinitandoc_cloud_endpoint_addresses_initappear to be carefully considered, suggesting an effort to ensure memory safety. However, without explicit memory analysis results, the absolute absence of memory leaks cannot be definitively verified. The review comment highlighting the potential for memory deallocation and relocation issues seems valid, and the developers' awareness and handling of these aspects in the code are evident. The approach allows for dynamic reinitialization of cloud server endpoints, enhancing flexibility and maintainability while being mindful of memory safety.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify memory safety and absence of memory leaks related to oc_endpoint_addresses_deinit and oc_cloud_endpoint_addresses_init usage. # This script is a placeholder and should be replaced with actual memory analysis tools commands. echo "Memory analysis tools should be used here to verify memory safety."Length of output: 142
Script:
#!/bin/bash # Find the implementation of oc_endpoint_addresses_deinit and oc_cloud_endpoint_addresses_init functions rg "oc_endpoint_addresses_deinit" -A 20 rg "oc_cloud_endpoint_addresses_init" -A 20 # Search for tests related to these functions rg "oc_endpoint_addresses_deinit" --glob '*test*' rg "oc_cloud_endpoint_addresses_init" --glob '*test*'Length of output: 22243
api/cloud/unittest/cloud_store_test.cpp (6)
- 36-36: The inclusion of
"util/oc_endpoint_address.h"aligns with the PR's objective of transitioning to a generalized endpoint address management system. This change is necessary for the test file to access the new functionalities introduced by the refactor.- 72-95: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [75-103]
The
compareEndpointsfunction has been updated to useoc_endpoint_addresses_tandoc_endpoint_address_metadata_t, reflecting the transition from specific cloud endpoint management to a more generalized approach. This change is crucial for ensuring that the test suite accurately tests the new endpoint address management functionalities. However, it's important to ensure that the comparison logic correctly handles all relevant fields of theoc_endpoint_address_metadata_tstructure, including any new fields introduced by the refactor.
- 134-140: The
makeEmptyStorefunction has been updated to initializeoc_cloud_store_twith the newoc_cloud_endpoint_addresses_initfunction. This change is consistent with the refactor's goal of using the new endpoint address management system. Ensure that the initialization correctly sets up theoc_cloud_store_tstructure for tests, especially considering any default values or initial states required for the new endpoint address management system.- 131-148: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [144-164]
The
makeStoreandfreeStorefunctions have been updated to use the new endpoint address management functions. These changes are necessary to create and clean up test data that aligns with the new system. It's important to ensure that these functions correctly handle all aspects of theoc_cloud_store_tstructure, especially any new fields or structures introduced by the refactor.
- 242-248: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [232-304]
The test cases
Decode_ServersArray,Decode_FailUnknownProperty,Decode_FailUnknownIntProperty,Decode_FailUnknownStringProperty,Decode_FailUnknownObjectArrayProperty, andDecode_FailInvalidSidhave been updated to reflect the changes in the cloud store's data structure and decoding logic. These updates are crucial for validating the correctness and robustness of the new endpoint address management system. It's important to ensure that these test cases cover all relevant scenarios, including edge cases and error conditions.
- 337-350: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [312-472]
The test cases
LoadDefaults,DumpAndLoad,DumpAndLoad_MultipleEndpoints,SingleStoreData,InvalidStoreData,DumpAsync, andDumphave been updated to use the new endpoint address management functionalities. These changes are essential for testing the integration of the new system with the cloud store's load and dump operations. It's important to ensure that these test cases adequately test the functionality and reliability of the new system, including scenarios involving multiple endpoints and asynchronous operations.util/unittest/endpoint_address_test.cpp (17)
- 19-30: Please verify that all included headers are necessary for this test file. It's good practice to include only what's needed to keep compile times short and dependencies clear.
- 36-39: The use of
OC_MEMBforg_endpoint_addresses_sis appropriate here, ensuring that memory for endpoint addresses is allocated statically. This aligns well with the constraints of IoT devices, which often have limited dynamic memory.- 41-59: The
TestEndpointAddressesclass is well-structured, providing setup and teardown methods for the test environment. UsingSetUpTestCaseandTearDownTestCasefor initializing and destroying global resources is a good practice.- 70-84: This test case,
InitWithDefault, correctly verifies the initialization ofoc_endpoint_addresses_twith default values. It's comprehensive, checking both the presence of the default URI and the correct UUID.- 86-94: The
Init_FailInvalidDefaulttest case effectively checks for failure when initializing with an excessively long URI. This is a good practice for validating input constraints.- 96-105: The
IsEmptytest case properly checks both an explicitly emptyoc_endpoint_addresses_tinstance and one initialized by the test setup. It's a straightforward and effective validation of the empty state.- 107-114: The
Containstest case correctly verifies that theoc_endpoint_addresses_containsfunction returnsfalsefor an empty list andOC_STRING_VIEW_NULL. This aligns with expected behavior.- 116-140: The
AddAndRemovetest case thoroughly tests adding and removing endpoint addresses, including checking the list size and contents at each step. This comprehensive approach ensures the correctness of both addition and removal functionalities.- 142-161: The
Add_Failtest case correctly tests edge cases for adding endpoint addresses, including null URIs and excessively long URIs. The inclusion of a loop to test the limit of statically allocated addresses underOC_DYNAMIC_ALLOCATIONis commendable.- 163-170: The
Add_DuplicateFailtest case effectively checks that adding a duplicate URI fails, ensuring the uniqueness of endpoint addresses in the list.- 172-187: The
Remove_Failtest case properly tests failure scenarios for removing endpoint addresses, including non-existent elements and null URIs. This thorough testing of edge cases is crucial for robustness.- 189-246: The
Selecttest case comprehensively tests the selection functionality, including automatic selection of the first added item, failure to select a non-existing URI, and correct selection among multiple addresses. The use ofOC_DYNAMIC_ALLOCATIONto conditionally test additional scenarios is a good practice.- 249-304: The
RemoveSelectedtest case thoroughly tests the behavior of removing selected endpoint addresses, including the automatic selection of the next available address. This test case effectively ensures the correct management of the selected state during removal operations.- 306-327: The
Findtest case correctly tests the finding functionality for existing and non-existing URIs. It's a straightforward and effective validation of theoc_endpoint_addresses_findfunction.- 329-342: The
EncodeEmptytest case verifies that no data is encoded when the list of endpoint addresses is empty. This is an important test for ensuring the correct behavior of the encoding function in edge cases.- 343-373: The
EncodeSingletest case effectively tests the encoding of a single endpoint address, both when skipping and not skipping the single selected address. This dual scenario testing is crucial for validating the encoding logic under different conditions.- 375-411: The
EncodeMultipletest case thoroughly tests the encoding of multiple endpoint addresses, ensuring the correct JSON output. This comprehensive approach is essential for validating the encoding functionality with more complex data.include/oc_cloud.h (7)
- 33-33: The inclusion of
"util/oc_endpoint_address.h"is appropriate and aligns with the PR's objectives to support generalized endpoint address management.- 267-267: The renaming of functions to reflect the new generalized approach to endpoint address management is clear and consistent, enhancing the readability and maintainability of the code.
Also applies to: 565-565, 585-585, 611-611, 627-627, 656-656
- 580-582: The function
oc_cloud_add_server_addresscorrectly implements the addition of a new cloud server address to the context. Ensure proper validation of theurianduri_lenparameters to prevent potential security issues or crashes due to invalid input.- 606-607: The function
oc_cloud_remove_server_addressprovides functionality to remove a cloud server address. It's important to ensure that the removal logic correctly handles edge cases, such as attempting to remove a non-existent address or the currently selected address.- 622-624: The
oc_cloud_iterate_server_addressesfunction allows iterating over cloud server addresses. Verify that the iteration process is robust and does not allow modification of the list during iteration, which could lead to undefined behavior.- 651-653: The
oc_cloud_select_server_addressfunction enables the selection of a specific cloud server address. Confirm that the selection logic properly updates the context and that the selected address is correctly used in subsequent operations.- 664-664: The
oc_cloud_selected_server_addressfunction retrieves the currently selected cloud server address. Ensure that it returns the correct address and handles cases where no address is selected gracefully.swig/java_lang/oc_iotivity_lite_jni.c (2)
- 81-84: The conditional compilation introduced for
cls_OCCloudContextandcls_OCCloudHandlerbased on theOC_CLOUDflag is a good practice for enabling or disabling features based on compilation flags. This approach allows for more flexible configuration of the IoTivity Lite framework, especially for builds where cloud functionality might not be needed or desired. It's important to ensure that all references to these classes elsewhere in the code are also conditionally compiled or appropriately handled to avoid anyClassNotFoundExceptionat runtime.- 319-333: The addition of
cls_OCCloudContextandcls_OCCloudHandlerwithin theOC_CLOUDconditional block during theJNI_OnLoadfunction ensures that these classes are only loaded and referenced when cloud functionality is enabled. This is consistent with the conditional declaration earlier in the file. However, it's crucial to verify that the Java classesorg/iotivity/OCCloudContextandorg/iotivity/OCCloudHandlerare correctly implemented and available in the Java layer when theOC_CLOUDflag is enabled. Additionally, ensure that there are no memory leaks associated with the global references created here, especially in scenarios where the cloud functionality might be dynamically enabled or disabled at runtime.swig/swig_interfaces/oc_cloud.i (5)
- 11-11: The addition of
%import "oc_endpoint_address.i"is consistent with the PR's objective to generalize cloud endpoint management. This import statement ensures that the new endpoint address functionalities are accessible in the SWIG-generated interfaces, facilitating their use in Java and potentially other languages.- 717-724: The
addServerAddressfunction correctly wraps the C functionoc_cloud_add_server_address, providing a JNI interface for adding server addresses. This aligns with the PR's goal of enhancing server address management. Ensure that the corresponding C function properly handles the inputs and returns a validoc_endpoint_address_metadata_tpointer.- 726-733: The
removeServerAddressfunction provides a JNI interface for removing server addresses, correctly callingoc_cloud_remove_server_address. This addition is crucial for managing server addresses dynamically. It's important to verify that the C function correctly identifies and removes the specified server address.- 735-742: The
selectServerAddressfunction introduces the ability to select a specific server address as active, which is a new capability provided by this PR. This function wrapsoc_cloud_select_server_address, and its inclusion is essential for scenarios where multiple server addresses are available, and one needs to be chosen based on certain criteria.- 744-751: The
selectedServerAddressfunction provides access to the currently selected server address, wrappingoc_cloud_selected_server_address. This function is necessary for retrieving the active server address, allowing higher-level code to query which server address is currently in use. This addition supports the PR's objective of flexible server address management.api/cloud/unittest/cloud_manager_test.cpp (6)
- 73-75: The use of
oc_endpoint_addresses_addandoc_endpoint_addresses_select_by_uriin the test setup reflects the PR's objective of transitioning to a more generalized endpoint management approach. Ensure that the corresponding functions are thoroughly tested elsewhere to validate their correctness and robustness in various scenarios.- 344-347: The addition of a new endpoint address using
oc_endpoint_addresses_addand the subsequent assertion to check if the endpoint is not selected by default (oc_endpoint_addresses_is_selected) are correctly implemented. This change effectively tests the new endpoint management functionality within the context of retry logic.- 371-372: The initialization of cloud endpoint addresses using
oc_cloud_endpoint_addresses_initin theTestCloudManagerDatasetup is correctly implemented. This step is essential for ensuring that the test environment accurately reflects the changes made to the endpoint management system.- 567-569: The reinitialization of cloud endpoint addresses using
oc_endpoint_addresses_reinitand the subsequent verification of endpoint selection (oc_endpoint_addresses_is_selected) are correctly implemented. These changes effectively test the new functionality introduced for managing cloud endpoint addresses, ensuring that the system behaves as expected when handling redirect responses.- 577-581: The thorough testing of cloud endpoint address selection using
oc_endpoint_addresses_is_selectedafter reinitialization and handling redirect responses is commendable. These tests ensure that the new endpoint management system behaves correctly across different scenarios, providing confidence in the robustness of the changes introduced by the PR.- 590-590: The consistent testing of cloud endpoint address selection using
oc_endpoint_addresses_is_selectedafter handling redirect responses further demonstrates the thoroughness of the test coverage for the new endpoint management system. This approach ensures that the system's behavior is well-understood and reliable across various scenarios.api/cloud/unittest/cloud_test.cpp (8)
- 268-268: The call to
oc_endpoint_addresses_clear(&ctx->store.ci_servers);is used to clear the list of cloud endpoints. This is part of the transition to using the newoc_endpoint_addresses_tstructure for managing cloud endpoints. Ensure that this operation is safe and does not lead to memory leaks or dangling pointers, especially in a test environment where resources are frequently allocated and deallocated.- 347-347: Similar to the previous comment, the call to
oc_endpoint_addresses_clear(&ctx->store.ci_servers);is used here to clear the list of cloud endpoints before attempting a login operation. It's crucial to verify that this operation is correctly implemented to prevent resource leaks or unintended side effects, particularly in the context of testing cloud login functionality.- 410-410: Again, the call to
oc_endpoint_addresses_clear(&ctx->store.ci_servers);is used to clear the list of cloud endpoints before refreshing the token. This pattern of clearing endpoints before performing cloud operations is consistent across the test cases. It's important to ensure that the clearing operation is correctly handled to avoid any potential issues with resource management.- 477-477: The use of
oc_endpoint_addresses_clear(&ctx->store.ci_servers);to clear the list of cloud endpoints before logging out is noted. This consistent approach across different cloud operations tests underscores the importance of thoroughly verifying the implementation of theoc_endpoint_addresses_clearfunction to ensure it properly manages resources without causing leaks or other issues.- 559-559: Here, the
oc_endpoint_addresses_clear(&ctx->store.ci_servers);function is called to clear the list of cloud endpoints before deregistration. This consistent use of the clearing function across various cloud operation tests highlights the need for a detailed review of its implementation to ensure it effectively manages the lifecycle of cloud endpoint resources.- 680-680: The call to
oc_endpoint_addresses_clear(&ctx->store.ci_servers);in the context of deregistering with a refresh token indicates a pattern of preparing the cloud context for operations by clearing existing endpoints. As with previous instances, it's critical to verify that this operation is implemented correctly to manage resources efficiently and avoid potential issues.- 727-727: The use of
oc_endpoint_addresses_clear(&ctx->store.ci_servers);to clear the list of cloud endpoints before attempting deregistration with login underscores the importance of ensuring that the clearing operation is robust and correctly manages the resources associated with cloud endpoints.- 763-815: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [766-859]
The
EndpointAPItest case introduces and manipulates cloud endpoints using the newoc_endpoint_addresses_tstructure. It tests adding, removing, selecting, and iterating over cloud endpoints. This test case is crucial for verifying the correct behavior of the new endpoint management functionality. Ensure that all operations on theoc_endpoint_addresses_tstructure are correctly implemented and that the test case adequately covers various scenarios, including edge cases that might arise during endpoint management.api/cloud/oc_cloud_manager.c (6)
- 40-40: The inclusion of
"util/oc_endpoint_address_internal.h"aligns with the PR's objective of transitioning to a more generalized endpoint address management system. This change is necessary for the file to utilize the newoc_endpoint_addresses_tstructure and related functions.- 141-141: The call to
oc_endpoint_addresses_select_next(&ctx->store.ci_servers);replaces the previous approach to selecting the next cloud server endpoint. This change is crucial for implementing the new generalized endpoint address management system. Ensure that theoc_endpoint_addresses_select_nextfunction is thoroughly tested to handle edge cases and maintain the expected behavior.- 366-366: The use of
oc_endpoint_addresses_is_selectedto check if a given endpoint address is currently selected is a good application of the new endpoint address management functionalities. This change is consistent with the PR's goal of enhancing flexibility in cloud interactions.- 363-374: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [370-382]
The handling of the
redirecturiresponse by adding a new server to the list of endpoint addresses if it's not already present, and then selecting it, demonstrates a thoughtful implementation of the new endpoint address management system. However, ensure that theoc_endpoint_addresses_addandoc_endpoint_addresses_select_by_urifunctions are robust and handle all possible edge cases, especially in scenarios involving multiple redirects or failures in adding a new address.
- 389-389: Removing the original server from the list of endpoint addresses after a redirect is a necessary step to maintain an accurate and up-to-date list of servers. This action should be carefully managed to ensure that it does not inadvertently disrupt ongoing communications with other servers or lead to resource leaks.
- 393-393: Selecting the new server after a redirect by using
oc_endpoint_addresses_select_by_uriis a critical step in handling redirects properly. This ensures that subsequent communications are directed to the newly selected server. It's important to verify that this function correctly updates the internal state to reflect the new selection.
Danielius1922
force-pushed
the
adam/feature/endpoint-list
branch
from
a3c3d38 to
6cf828e
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
tools/doxygen.iniis excluded by:!**/*.ini
Files selected for processing (29)
- api/cloud/oc_cloud.c (3 hunks)
- api/cloud/oc_cloud_context.c (4 hunks)
- api/cloud/oc_cloud_context_internal.h (1 hunks)
- api/cloud/oc_cloud_endpoints.c (1 hunks)
- api/cloud/oc_cloud_endpoints_internal.h (1 hunks)
- api/cloud/oc_cloud_manager.c (3 hunks)
- api/cloud/oc_cloud_resource.c (3 hunks)
- api/cloud/oc_cloud_store.c (5 hunks)
- api/cloud/oc_cloud_store_internal.h (3 hunks)
- api/cloud/unittest/cloud_context_test.cpp (1 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (7 hunks)
- api/cloud/unittest/cloud_resource_test.cpp (1 hunks)
- api/cloud/unittest/cloud_store_test.cpp (18 hunks)
- api/cloud/unittest/cloud_test.cpp (10 hunks)
- include/oc_api.h (2 hunks)
- include/oc_cloud.h (4 hunks)
- port/esp32/main/CMakeLists.txt (2 hunks)
- swig/Makefile (2 hunks)
- swig/java_lang/oc_iotivity_lite_jni.c (5 hunks)
- swig/java_lang/oc_iotivity_lite_jni.h (2 hunks)
- swig/swig_interfaces/iotivity.swg (1 hunks)
- swig/swig_interfaces/oc_cloud.i (2 hunks)
- swig/swig_interfaces/oc_endpoint.i (1 hunks)
- swig/swig_interfaces/oc_endpoint_address.i (1 hunks)
- util/oc_endpoint_address.c (1 hunks)
- util/oc_endpoint_address.h (1 hunks)
- util/oc_endpoint_address_internal.h (1 hunks)
- util/oc_features.h (1 hunks)
- util/unittest/endpoint_address_test.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (27)
- 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_manager.c
- api/cloud/oc_cloud_resource.c
- api/cloud/oc_cloud_store.c
- api/cloud/oc_cloud_store_internal.h
- 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
- include/oc_api.h
- port/esp32/main/CMakeLists.txt
- swig/Makefile
- swig/java_lang/oc_iotivity_lite_jni.c
- swig/java_lang/oc_iotivity_lite_jni.h
- swig/swig_interfaces/iotivity.swg
- swig/swig_interfaces/oc_cloud.i
- swig/swig_interfaces/oc_endpoint.i
- swig/swig_interfaces/oc_endpoint_address.i
- util/oc_endpoint_address.c
- util/oc_endpoint_address.h
- util/oc_endpoint_address_internal.h
- util/oc_features.h
- util/unittest/endpoint_address_test.cpp
Additional comments: 11
api/cloud/unittest/cloud_store_test.cpp (5)
- 36-36: The inclusion of
"util/oc_endpoint_address.h"aligns with the PR's objective to refactor cloud endpoint management. This change is necessary for the unit tests to access the functionalities provided by the newoc_endpoint_addresses_tstructure and related functions.- 72-93: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [75-116]
The
compareEndpointsfunction has been significantly updated to accommodate the newoc_endpoint_addresses_tstructure. This function now uses maps to compare endpoint addresses based on their URIs and metadata, which is a logical approach for ensuring that the endpoints in twooc_endpoint_addresses_tinstances match.However, there's a potential improvement in the way the maps are populated and compared:
- The process of populating
e1ande2maps is duplicated code. Consider abstracting this logic into a separate function to adhere to the DRY (Don't Repeat Yourself) principle.- The comparison logic could be simplified by using standard library functions or algorithms designed for map comparison, if available in the project's C++ standard version.
Overall, the changes are aligned with the PR's objectives, but there's room for code quality improvements.
- 142-148: The
makeEmptyStorefunction has been updated to initialize anoc_cloud_store_tinstance with an emptyoc_endpoint_addresses_tforci_servers. This change is consistent with the PR's goal of using the new endpoint address management approach.One thing to note is the use of
OC_STRING_VIEW_NULLand an empty initializer list{}for theoc_cloud_endpoint_addresses_initcall. This is correct and ensures that theci_serversfield is initialized properly without any addresses. It's a good example of how to initialize the new structure in a clean state.
- 172-172: The
freeStorefunction correctly deinitializes theoc_endpoint_addresses_tstructure within anoc_cloud_store_tinstance usingoc_endpoint_addresses_deinit. This is crucial for preventing memory leaks and ensuring that dynamically allocated memory for endpoint addresses is properly freed. The update is in line with the PR's objectives and demonstrates good memory management practices.- 348-354: The
DumpAndLoad_MultipleEndpointstest case has been updated to add multiple endpoints to anoc_cloud_store_tinstance using the newoc_endpoint_addresses_addfunction. This test case effectively demonstrates the functionality of adding endpoints with UUID metadata to theci_serversfield.This change is a direct application of the new endpoint address management functionalities introduced in the PR. It's important to ensure that the added endpoints are correctly handled during the dump and load operations, which this test case aims to verify.
One minor suggestion is to ensure that the test case also verifies the correctness of the added endpoints after loading, beyond just ensuring that the
oc_cloud_store_loadfunction returnstrue. This could involve comparing the contents of theci_serversfield before and after to ensure that all added endpoints are correctly persisted and loaded.include/oc_cloud.h (6)
- 33-33: The inclusion of
"util/oc_endpoint_address.h"is a necessary change to support the new generalized approach to cloud endpoint management. This allows the file to utilize the functionalities provided byoc_endpoint_address.candoc_endpoint_address.h, such as initialization, addition, removal, selection, and encoding of endpoint addresses.- 267-267: Renaming
oc_cloud_get_servertooc_cloud_get_server_address(as implied by the context) aligns with the new approach of managing cloud server addresses rather than specific cloud servers. This change is consistent with the PR's objective to generalize cloud endpoint management. However, the actual renaming seems to be missing in the provided code, which might be an oversight or due to the limited context provided. It's important to ensure that all references and documentation are updated accordingly to reflect the new naming convention.Please confirm that all instances and documentation references have been updated to reflect the new naming convention for
oc_cloud_get_serverand related functions.
- 585-607: The
oc_cloud_remove_server_addressfunction is essential for managing the lifecycle of cloud server addresses, allowing for their removal from the system. This function complementsoc_cloud_add_server_addressby providing the necessary mechanism to clean up unused or outdated addresses. The note regarding the behavior when the selected server address is removed is particularly important, as it highlights the need for careful management and potential restart of the cloud manager to reflect changes. The implementation should ensure that the removal process is robust and handles edge cases gracefully.- 611-624: The
oc_cloud_iterate_server_addressesfunction introduces the ability to iterate over all cloud server addresses, which is useful for operations that need to apply actions or checks to each address. The inclusion of a callback function (fn) and custom user data (data) as parameters provides flexibility in how this iteration is utilized. It's important that the implementation ensures the iteration process is safe and does not allow the callback function to modify the list of addresses, as noted. This function enhances the modularity and flexibility of the cloud server address management.- 627-653: Introducing
oc_cloud_select_server_addressallows for the explicit selection of a cloud server address from the list of available addresses. This function is crucial for enabling dynamic selection based on criteria such as availability, latency, or other factors. The notes regarding the caching behavior and the need to potentially restart the cloud manager for changes to take effect are important considerations for developers using this function. Ensuring that the selection process is clear and well-documented will be key to its successful integration into the IoTivity Lite framework.- 656-663: The function
oc_cloud_selected_server_addressprovides a mechanism to retrieve the currently selected cloud server address, complementing the selection functionality introduced byoc_cloud_select_server_address. This function is essential for querying the active cloud server address, enabling other components of the IoTivity Lite framework to interact with the selected cloud server. Ensuring that this function correctly handles cases where no server address is selected will be important for robustness.
|
@coderabbitai is the doxygen documentation in the public header |
|
Tip For best results, initiate chat on the files or code changes. @Danielius1922, the Doxygen documentation in
Improving these areas could make the documentation even more valuable to developers, enhancing their ability to effectively utilize the IoTivity Lite framework for cloud interactions. Analysis chainScripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Locate and display the content of `oc_cloud.h` for review.
fd oc_cloud.h --exec cat {}
Length of output: 21045 |
Danielius1922
force-pushed
the
adam/feature/endpoint-list
branch
from
6cf828e to
f089c17
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
tools/doxygen.iniis excluded by:!**/*.ini
Files selected for processing (29)
- api/cloud/oc_cloud.c (3 hunks)
- api/cloud/oc_cloud_context.c (3 hunks)
- api/cloud/oc_cloud_context_internal.h (1 hunks)
- api/cloud/oc_cloud_endpoints.c (1 hunks)
- api/cloud/oc_cloud_endpoints_internal.h (1 hunks)
- api/cloud/oc_cloud_manager.c (3 hunks)
- api/cloud/oc_cloud_resource.c (3 hunks)
- api/cloud/oc_cloud_store.c (5 hunks)
- api/cloud/oc_cloud_store_internal.h (3 hunks)
- api/cloud/unittest/cloud_context_test.cpp (1 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (7 hunks)
- api/cloud/unittest/cloud_resource_test.cpp (1 hunks)
- api/cloud/unittest/cloud_store_test.cpp (18 hunks)
- api/cloud/unittest/cloud_test.cpp (10 hunks)
- include/oc_api.h (2 hunks)
- include/oc_cloud.h (4 hunks)
- port/esp32/main/CMakeLists.txt (2 hunks)
- swig/Makefile (2 hunks)
- swig/java_lang/oc_iotivity_lite_jni.c (5 hunks)
- swig/java_lang/oc_iotivity_lite_jni.h (2 hunks)
- swig/swig_interfaces/iotivity.swg (1 hunks)
- swig/swig_interfaces/oc_cloud.i (2 hunks)
- swig/swig_interfaces/oc_endpoint.i (1 hunks)
- swig/swig_interfaces/oc_endpoint_address.i (1 hunks)
- util/oc_endpoint_address.c (1 hunks)
- util/oc_endpoint_address.h (1 hunks)
- util/oc_endpoint_address_internal.h (1 hunks)
- util/oc_features.h (1 hunks)
- util/unittest/endpoint_address_test.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (26)
- 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_manager.c
- api/cloud/oc_cloud_resource.c
- api/cloud/oc_cloud_store.c
- api/cloud/oc_cloud_store_internal.h
- 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
- include/oc_api.h
- port/esp32/main/CMakeLists.txt
- swig/Makefile
- swig/java_lang/oc_iotivity_lite_jni.c
- swig/java_lang/oc_iotivity_lite_jni.h
- swig/swig_interfaces/iotivity.swg
- swig/swig_interfaces/oc_cloud.i
- swig/swig_interfaces/oc_endpoint.i
- util/oc_endpoint_address.c
- util/oc_endpoint_address.h
- util/oc_endpoint_address_internal.h
- util/oc_features.h
- util/unittest/endpoint_address_test.cpp
Additional comments: 13
swig/swig_interfaces/oc_endpoint_address.i (2)
- 23-24: Including both
"util/oc_endpoint_address_internal.h"and"util/oc_endpoint_address.h"ensures that the SWIG interface has access to the necessary types and functions for endpoint address management. This aligns with the PR's objective of enhancing endpoint management. However, ensure that the internal header is meant to be exposed in this context, as typically internal headers contain implementation details not intended for public API consumption.- 49-49: The
%renamedirective is used to exposeoc_endpoint_address_tasOCEndpointAddressin the generated bindings. This is a good practice as it provides a more user-friendly name for the type in the target language. Ensure that this renaming is consistent across all bindings and documentation to avoid confusion.api/cloud/unittest/cloud_store_test.cpp (4)
- 36-36: Including
"util/oc_endpoint_address.h"is necessary for the unit tests to access the new endpoint address management functionality. This aligns with the PR's objective of transitioning to a more generalized approach for endpoint management.- 72-93: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [75-116]
The
compareEndpointsfunction has been updated to compareoc_endpoint_addresses_tstructures. This update is crucial for ensuring that the new endpoint address management functionality works as expected. However, there are a few points to consider:
- The use of
std::mapfor comparing endpoints is efficient, but ensure that the comparison logic correctly handles all relevant fields ofoc_endpoint_address_t.- The comparison of UUIDs using
oc_uuid_is_equaland names usingEXPECT_STREQis appropriate. However, ensure comprehensive testing for edge cases, such as empty or null values.- The loop for populating
e1ande2maps is clear, but consider adding comments to explain the logic, especially for developers unfamiliar with the codebase.Consider adding more comments to explain the logic within the
compareEndpointsfunction, especially the loop for populating the maps and the conditions checked during comparison.
- 142-148: The
makeEmptyStorefunction has been updated to initializeoc_cloud_store_twithoc_cloud_endpoint_addresses_init. This change is necessary to align with the new endpoint address management approach. However, ensure that the initialization parameters are correctly set and that all necessary fields ofoc_cloud_store_tare initialized to prevent undefined behavior.- 172-172: The
freeStorefunction correctly deinitializes theoc_endpoint_addresses_tstructure withinoc_cloud_store_t. This is crucial for avoiding memory leaks and ensuring proper resource management. Ensure that all other resources withinoc_cloud_store_tare also correctly freed.include/oc_cloud.h (7)
- 33-33: The inclusion of
"util/oc_endpoint_address.h"is appropriate and necessary for supporting the new endpoint address management functionality introduced in this PR.- 267-267: Renaming
oc_cloud_get_serverto reflect its new functionality in the context of server address management is a clear and meaningful change that aligns with the PR's objectives.- 579-582: The function
oc_cloud_add_server_addressis a key addition, enabling dynamic management of cloud server addresses. Ensure that theurianduri_lenparameters are validated within this function or in any called functions to prevent security issues or crashes due to invalid input.- 606-607: The function
oc_cloud_remove_server_addressprovides necessary functionality for managing cloud server addresses dynamically. It's important to ensure that the removal logic correctly handles the selection of a new server address if the current selected address is removed, as described in the notes.- 622-624: The
oc_cloud_iterate_server_addressesfunction is crucial for enabling operations on the list of cloud server addresses. It's important that the callback function provided to this iterator does not modify the list of addresses to maintain integrity during iteration.- 651-653: The
oc_cloud_select_server_addressfunction allows for the selection of a specific cloud server address. It's important to ensure that the selection logic correctly updates the cloud context to reflect the new selected address and that any necessary cloud manager restarts are handled appropriately.- 663-663: The function
oc_cloud_selected_server_addressprovides a way to retrieve the currently selected cloud server address. This function is essential for querying the active cloud server address and aligns with the new approach to endpoint management.
Danielius1922
force-pushed
the
adam/feature/endpoint-list
branch
from
f089c17 to
9c354be
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
tools/doxygen.iniis excluded by:!**/*.ini
Files selected for processing (30)
- api/cloud/oc_cloud.c (3 hunks)
- api/cloud/oc_cloud_context.c (3 hunks)
- api/cloud/oc_cloud_context_internal.h (1 hunks)
- api/cloud/oc_cloud_endpoints.c (1 hunks)
- api/cloud/oc_cloud_endpoints_internal.h (1 hunks)
- api/cloud/oc_cloud_manager.c (3 hunks)
- api/cloud/oc_cloud_resource.c (3 hunks)
- api/cloud/oc_cloud_store.c (5 hunks)
- api/cloud/oc_cloud_store_internal.h (3 hunks)
- api/cloud/unittest/cloud_context_test.cpp (1 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (7 hunks)
- api/cloud/unittest/cloud_resource_test.cpp (1 hunks)
- api/cloud/unittest/cloud_store_test.cpp (18 hunks)
- api/cloud/unittest/cloud_test.cpp (10 hunks)
- api/oc_helpers.c (1 hunks)
- include/oc_api.h (2 hunks)
- include/oc_cloud.h (4 hunks)
- port/esp32/main/CMakeLists.txt (2 hunks)
- swig/Makefile (2 hunks)
- swig/java_lang/oc_iotivity_lite_jni.c (5 hunks)
- swig/java_lang/oc_iotivity_lite_jni.h (2 hunks)
- swig/swig_interfaces/iotivity.swg (1 hunks)
- swig/swig_interfaces/oc_cloud.i (2 hunks)
- swig/swig_interfaces/oc_endpoint.i (1 hunks)
- swig/swig_interfaces/oc_endpoint_address.i (1 hunks)
- util/oc_endpoint_address.c (1 hunks)
- util/oc_endpoint_address.h (1 hunks)
- util/oc_endpoint_address_internal.h (1 hunks)
- util/oc_features.h (1 hunks)
- util/unittest/endpoint_address_test.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (28)
- 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_manager.c
- api/cloud/oc_cloud_resource.c
- api/cloud/oc_cloud_store.c
- api/cloud/oc_cloud_store_internal.h
- 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
- include/oc_api.h
- include/oc_cloud.h
- port/esp32/main/CMakeLists.txt
- swig/Makefile
- swig/java_lang/oc_iotivity_lite_jni.c
- swig/java_lang/oc_iotivity_lite_jni.h
- swig/swig_interfaces/iotivity.swg
- swig/swig_interfaces/oc_cloud.i
- swig/swig_interfaces/oc_endpoint.i
- swig/swig_interfaces/oc_endpoint_address.i
- util/oc_endpoint_address.c
- util/oc_endpoint_address.h
- util/oc_endpoint_address_internal.h
- util/oc_features.h
- util/unittest/endpoint_address_test.cpp
Additional comments: 5
api/oc_helpers.c (1)
- 50-52: The addition of the conditional assertion
oc_assert(alloc_ret > 0);within theoc_mallocfunction, guarded by#ifndef OC_TEST, is a good practice for catching memory allocation failures. However, ensure that this behavior aligns with the overall error handling strategy and is thoroughly tested in both test and production environments.api/cloud/unittest/cloud_store_test.cpp (4)
- 36-36: The inclusion of
"util/oc_endpoint_address.h"aligns with the PR's objective to transition to a more generalized approach usingoc_endpoint_addresses_t. This change is necessary for the updated test cases to compile and run successfully.- 72-93: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [75-116]
The
compareEndpointsfunction has been updated to compareoc_endpoint_addresses_tstructures. This update is crucial for ensuring that the test cases accurately reflect the changes in the underlying data structures used for endpoint management. The use ofstd::mapfor comparison is a good approach to handle potential differences in the order of endpoints. Ensure that all edge cases, such as empty endpoint lists and differing metadata, are adequately tested.
- 142-148: The
makeEmptyStorefunction has been updated to initializeoc_cloud_store_twithoc_endpoint_addresses_tusingoc_cloud_endpoint_addresses_init. This change is consistent with the PR's objectives and ensures that the test setup reflects the new endpoint address management approach.- 172-172: The
freeStorefunction now correctly deinitializesoc_endpoint_addresses_tusingoc_endpoint_addresses_deinit. This change is necessary to prevent memory leaks and ensure proper cleanup of resources.
Danielius1922
force-pushed
the
adam/feature/endpoint-list
branch
from
9c354be to
93df755
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
tools/doxygen.iniis excluded by:!**/*.ini
Files selected for processing (30)
- api/cloud/oc_cloud.c (3 hunks)
- api/cloud/oc_cloud_context.c (3 hunks)
- api/cloud/oc_cloud_context_internal.h (1 hunks)
- api/cloud/oc_cloud_endpoints.c (1 hunks)
- api/cloud/oc_cloud_endpoints_internal.h (1 hunks)
- api/cloud/oc_cloud_manager.c (3 hunks)
- api/cloud/oc_cloud_resource.c (3 hunks)
- api/cloud/oc_cloud_store.c (5 hunks)
- api/cloud/oc_cloud_store_internal.h (3 hunks)
- api/cloud/unittest/cloud_context_test.cpp (1 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (7 hunks)
- api/cloud/unittest/cloud_resource_test.cpp (1 hunks)
- api/cloud/unittest/cloud_store_test.cpp (18 hunks)
- api/cloud/unittest/cloud_test.cpp (10 hunks)
- api/oc_helpers.c (1 hunks)
- include/oc_api.h (2 hunks)
- include/oc_cloud.h (4 hunks)
- port/esp32/main/CMakeLists.txt (2 hunks)
- swig/Makefile (2 hunks)
- swig/java_lang/oc_iotivity_lite_jni.c (5 hunks)
- swig/java_lang/oc_iotivity_lite_jni.h (2 hunks)
- swig/swig_interfaces/iotivity.swg (1 hunks)
- swig/swig_interfaces/oc_cloud.i (2 hunks)
- swig/swig_interfaces/oc_endpoint.i (1 hunks)
- swig/swig_interfaces/oc_endpoint_address.i (1 hunks)
- util/oc_endpoint_address.c (1 hunks)
- util/oc_endpoint_address.h (1 hunks)
- util/oc_endpoint_address_internal.h (1 hunks)
- util/oc_features.h (1 hunks)
- util/unittest/endpoint_address_test.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (29)
- 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_manager.c
- api/cloud/oc_cloud_resource.c
- api/cloud/oc_cloud_store.c
- api/cloud/oc_cloud_store_internal.h
- 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_helpers.c
- include/oc_api.h
- include/oc_cloud.h
- port/esp32/main/CMakeLists.txt
- swig/Makefile
- swig/java_lang/oc_iotivity_lite_jni.c
- swig/java_lang/oc_iotivity_lite_jni.h
- swig/swig_interfaces/iotivity.swg
- swig/swig_interfaces/oc_cloud.i
- swig/swig_interfaces/oc_endpoint.i
- swig/swig_interfaces/oc_endpoint_address.i
- util/oc_endpoint_address.c
- util/oc_endpoint_address.h
- util/oc_endpoint_address_internal.h
- util/oc_features.h
- util/unittest/endpoint_address_test.cpp
Additional comments: 4
api/cloud/unittest/cloud_store_test.cpp (4)
- 36-36: The inclusion of
"util/oc_endpoint_address.h"is necessary for the new endpoint address management functionality. This change aligns with the PR's objective to refactor cloud endpoint management usingoc_endpoint_addresses_t.- 72-93: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [75-116]
The
compareEndpointsfunction has been updated to compareoc_endpoint_addresses_tstructures. This update is crucial for ensuring that the tests can accurately compare the results of operations on endpoint addresses. The use of maps to store and compare endpoints by their URI is a good approach for ensuring uniqueness and simplifying comparison logic. However, it's important to ensure that all relevant fields ofoc_endpoint_address_tare considered during comparison to avoid false positives or negatives in tests.Consider verifying that all relevant fields of
oc_endpoint_address_tare being compared, especially in complex scenarios where endpoints might differ by more than just their URI or UUID.
- 142-148: The
makeEmptyStorefunction initializes anoc_cloud_store_tinstance with empty endpoint addresses usingoc_cloud_endpoint_addresses_init. This change reflects the transition to usingoc_endpoint_addresses_tfor managing cloud endpoints. It's important to ensure that this initialization correctly sets up the structure for subsequent operations in the tests.- 172-172: The
freeStorefunction correctly deinitializes theoc_endpoint_addresses_twithin anoc_cloud_store_tinstance usingoc_endpoint_addresses_deinit. This ensures proper cleanup of dynamically allocated memory associated with endpoint addresses, preventing memory leaks in the tests.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- api/cloud/oc_cloud_store.c (5 hunks)
- util/oc_endpoint_address.c (1 hunks)
- util/oc_endpoint_address_internal.h (1 hunks)
- util/unittest/endpoint_address_test.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- api/cloud/oc_cloud_store.c
- util/oc_endpoint_address.c
- util/oc_endpoint_address_internal.h
- util/unittest/endpoint_address_test.cpp
Danielius1922
force-pushed
the
adam/feature/endpoint-list
branch
from
1d667fb to
453c197
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- api/cloud/oc_cloud_store.c (5 hunks)
- util/oc_endpoint_address.c (1 hunks)
- util/oc_endpoint_address_internal.h (1 hunks)
- util/unittest/endpoint_address_test.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- api/cloud/oc_cloud_store.c
- 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: 0
Configuration used: CodeRabbit UI
Files selected for processing (5)
- api/cloud/oc_cloud_context.c (3 hunks)
- api/cloud/oc_cloud_store.c (5 hunks)
- util/oc_endpoint_address.c (1 hunks)
- util/oc_endpoint_address_internal.h (1 hunks)
- util/unittest/endpoint_address_test.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- api/cloud/oc_cloud_context.c
- api/cloud/oc_cloud_store.c
- util/oc_endpoint_address.c
- util/oc_endpoint_address_internal.h
- util/unittest/endpoint_address_test.cpp
Danielius1922
force-pushed
the
adam/feature/endpoint-list
branch
from
04efdb7 to
d58ceac
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
tools/doxygen.iniis excluded by:!**/*.ini
Files selected for processing (30)
- api/cloud/oc_cloud.c (3 hunks)
- api/cloud/oc_cloud_context.c (5 hunks)
- api/cloud/oc_cloud_context_internal.h (1 hunks)
- api/cloud/oc_cloud_endpoints.c (1 hunks)
- api/cloud/oc_cloud_endpoints_internal.h (1 hunks)
- api/cloud/oc_cloud_manager.c (3 hunks)
- api/cloud/oc_cloud_resource.c (3 hunks)
- api/cloud/oc_cloud_store.c (5 hunks)
- api/cloud/oc_cloud_store_internal.h (3 hunks)
- api/cloud/unittest/cloud_context_test.cpp (1 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (7 hunks)
- api/cloud/unittest/cloud_resource_test.cpp (1 hunks)
- api/cloud/unittest/cloud_store_test.cpp (18 hunks)
- api/cloud/unittest/cloud_test.cpp (10 hunks)
- api/oc_helpers.c (1 hunks)
- include/oc_api.h (2 hunks)
- include/oc_cloud.h (4 hunks)
- port/esp32/main/CMakeLists.txt (2 hunks)
- swig/Makefile (2 hunks)
- swig/java_lang/oc_iotivity_lite_jni.c (5 hunks)
- swig/java_lang/oc_iotivity_lite_jni.h (2 hunks)
- swig/swig_interfaces/iotivity.swg (1 hunks)
- swig/swig_interfaces/oc_cloud.i (2 hunks)
- swig/swig_interfaces/oc_endpoint.i (1 hunks)
- swig/swig_interfaces/oc_endpoint_address.i (1 hunks)
- util/oc_endpoint_address.c (1 hunks)
- util/oc_endpoint_address.h (1 hunks)
- util/oc_endpoint_address_internal.h (1 hunks)
- util/oc_features.h (1 hunks)
- util/unittest/endpoint_address_test.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (29)
- 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_manager.c
- api/cloud/oc_cloud_resource.c
- api/cloud/oc_cloud_store.c
- api/cloud/oc_cloud_store_internal.h
- 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_helpers.c
- include/oc_api.h
- include/oc_cloud.h
- port/esp32/main/CMakeLists.txt
- swig/Makefile
- swig/java_lang/oc_iotivity_lite_jni.c
- swig/java_lang/oc_iotivity_lite_jni.h
- swig/swig_interfaces/iotivity.swg
- swig/swig_interfaces/oc_cloud.i
- swig/swig_interfaces/oc_endpoint.i
- swig/swig_interfaces/oc_endpoint_address.i
- util/oc_endpoint_address.c
- util/oc_endpoint_address.h
- util/oc_endpoint_address_internal.h
- util/oc_features.h
- util/unittest/endpoint_address_test.cpp
Additional comments: 6
api/cloud/unittest/cloud_store_test.cpp (6)
- 36-36: The inclusion of
"util/oc_endpoint_address.h"aligns with the PR's objective to generalize theoc_cloud_endpoints_tstructure. This change is necessary for the tests to utilize the newoc_endpoint_addresses_tstructure.- 72-93: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [75-116]
The
compareEndpointsfunction has been updated to compareoc_endpoint_addresses_tstructures. This update is crucial for ensuring that the endpoint addresses are correctly managed and compared within the tests. The use of maps to store endpoints keyed by their URI is a good approach for comparison. However, consider adding comments to explain the logic, especially for developers unfamiliar with the changes or the structure of endpoint addresses.+ // Maps are used to store endpoints keyed by URI for easy comparison. + // This approach allows for efficient lookup and comparison of endpoint addresses.
- 142-148: The
makeEmptyStorefunction correctly initializes anoc_cloud_store_tinstance with emptyoc_endpoint_addresses_t. This is a straightforward and necessary change to support the new structure. The initialization ofci_serverswithoc_cloud_endpoint_addresses_initis correctly done.- 139-156: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [152-172]
The
makeStoreandfreeStorefunctions have been updated to accommodate the newoc_endpoint_addresses_tstructure. These changes are essential for creating and cleaning up test data. The use ofoc_cloud_endpoint_addresses_initinmakeStoreandoc_endpoint_addresses_deinitinfreeStoreis appropriate. Ensure that all memory allocated during the test setup is correctly freed to avoid memory leaks.
- 348-354: The
DumpAndLoad_MultipleEndpointstest case has been updated to add multiple endpoints tooc_cloud_store_tusingoc_endpoint_addresses_add. This change is in line with the PR's objective and tests the functionality of adding multiple endpoints. As previously commented, it's important to also test failure scenarios, such as adding an endpoint failing due to memory constraints, and ensure that the cleanup logic correctly handles partially initialized structures.Consider adding test cases to cover failure scenarios for
oc_endpoint_addresses_addand ensure that cleanup logic is robust.
- 388-388: The debug logging conditionally compiled with
OC_DBG_IS_ENABLEDis a good practice for development and debugging purposes. However, ensure that this does not leak any sensitive information or significantly impact performance in production builds.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- api/cloud/oc_cloud_manager.c (3 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (7 hunks)
- util/oc_endpoint_address.c (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- api/cloud/oc_cloud_manager.c
- api/cloud/unittest/cloud_manager_test.cpp
- util/oc_endpoint_address.c
Danielius1922
force-pushed
the
adam/feature/endpoint-list
branch
from
821712a to
2448968
Compare
ocf-conformance-test-tool
bot
removed
the
OCF Conformance Testing
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
tools/doxygen.iniis excluded by:!**/*.ini
Files selected for processing (31)
- api/cloud/oc_cloud.c (3 hunks)
- api/cloud/oc_cloud_context.c (5 hunks)
- api/cloud/oc_cloud_context_internal.h (1 hunks)
- api/cloud/oc_cloud_endpoints.c (1 hunks)
- api/cloud/oc_cloud_endpoints_internal.h (1 hunks)
- api/cloud/oc_cloud_manager.c (3 hunks)
- api/cloud/oc_cloud_resource.c (3 hunks)
- api/cloud/oc_cloud_store.c (5 hunks)
- api/cloud/oc_cloud_store_internal.h (3 hunks)
- api/cloud/unittest/cloud_context_test.cpp (1 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (7 hunks)
- api/cloud/unittest/cloud_resource_test.cpp (1 hunks)
- api/cloud/unittest/cloud_store_test.cpp (18 hunks)
- api/cloud/unittest/cloud_test.cpp (10 hunks)
- api/oc_helpers.c (1 hunks)
- include/oc_api.h (2 hunks)
- include/oc_cloud.h (4 hunks)
- include/oc_helpers.h (2 hunks)
- port/esp32/main/CMakeLists.txt (2 hunks)
- swig/Makefile (2 hunks)
- swig/java_lang/oc_iotivity_lite_jni.c (5 hunks)
- swig/java_lang/oc_iotivity_lite_jni.h (2 hunks)
- swig/swig_interfaces/iotivity.swg (1 hunks)
- swig/swig_interfaces/oc_cloud.i (2 hunks)
- swig/swig_interfaces/oc_endpoint.i (1 hunks)
- swig/swig_interfaces/oc_endpoint_address.i (1 hunks)
- util/oc_endpoint_address.c (1 hunks)
- util/oc_endpoint_address.h (1 hunks)
- util/oc_endpoint_address_internal.h (1 hunks)
- util/oc_features.h (1 hunks)
- util/unittest/endpoint_address_test.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (29)
- 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_manager.c
- api/cloud/oc_cloud_resource.c
- api/cloud/oc_cloud_store.c
- api/cloud/oc_cloud_store_internal.h
- 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_helpers.c
- include/oc_api.h
- include/oc_cloud.h
- port/esp32/main/CMakeLists.txt
- swig/Makefile
- swig/java_lang/oc_iotivity_lite_jni.c
- swig/java_lang/oc_iotivity_lite_jni.h
- swig/swig_interfaces/iotivity.swg
- swig/swig_interfaces/oc_cloud.i
- swig/swig_interfaces/oc_endpoint.i
- swig/swig_interfaces/oc_endpoint_address.i
- util/oc_endpoint_address.c
- util/oc_endpoint_address.h
- util/oc_endpoint_address_internal.h
- util/oc_features.h
- util/unittest/endpoint_address_test.cpp
Additional comments: 5
include/oc_helpers.h (2)
- 29-29: Including
<assert.h>is necessary for the newly addedassertstatement within theoc_string_len_unsafefunction. This inclusion ensures that the assertion functionality is available, which is a standard debugging technique to verify assumptions made by the program.- 176-184: The addition of
oc_string_len_unsafeprovides a way to get the length of a non-NULLoc_string_t. However, it's crucial to ensure that this function is used in contexts where the non-NULL precondition is guaranteed, as the function name suggests. The use ofassertto enforce this precondition is appropriate, but it's important to note that assertions are typically disabled in production builds, so this does not replace proper error handling or validation in critical code paths.api/cloud/unittest/cloud_store_test.cpp (3)
- 36-36: The inclusion of
"util/oc_endpoint_address.h"is necessary for the updated test cases that now useoc_endpoint_addresses_tand related functions. This change aligns with the PR's objective to enhance cloud interaction capabilities by generalizing endpoint address handling.- 72-93: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [75-116]
The
compareEndpointsfunction has been updated to compareoc_endpoint_addresses_tstructures. This update is crucial for ensuring that the test cases accurately assess the equality of endpoint addresses. The use ofstd::mapto facilitate comparison by URI is a logical approach, allowing for an efficient comparison of potentially unordered lists. The detailed comparison of metadata, including UUIDs and names, ensures thorough testing of the endpoint address functionality.
- 142-148: The
makeEmptyStore,makeStore, andfreeStorefunctions have been updated to use the newoc_endpoint_addresses_tstructure and related functions. These changes are necessary for setting up and tearing down test cases that involve cloud store data. The use ofoc_cloud_endpoint_addresses_initandoc_endpoint_addresses_deinitin these functions demonstrates proper management of the new endpoint address structures.Also applies to: 152-161, 172-177
Danielius1922
force-pushed
the
adam/feature/endpoint-list
branch
2 times, most recently
from
6e06fe6 to
74b79b2
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
tools/doxygen.iniis excluded by:!**/*.ini
Files selected for processing (32)
- api/cloud/oc_cloud.c (3 hunks)
- api/cloud/oc_cloud_context.c (5 hunks)
- api/cloud/oc_cloud_context_internal.h (1 hunks)
- api/cloud/oc_cloud_endpoints.c (1 hunks)
- api/cloud/oc_cloud_endpoints_internal.h (1 hunks)
- api/cloud/oc_cloud_manager.c (3 hunks)
- api/cloud/oc_cloud_resource.c (3 hunks)
- api/cloud/oc_cloud_store.c (5 hunks)
- api/cloud/oc_cloud_store_internal.h (3 hunks)
- api/cloud/unittest/cloud_context_test.cpp (1 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (7 hunks)
- api/cloud/unittest/cloud_resource_test.cpp (1 hunks)
- api/cloud/unittest/cloud_store_test.cpp (18 hunks)
- api/cloud/unittest/cloud_test.cpp (10 hunks)
- api/oc_core_res.c (1 hunks)
- api/oc_helpers.c (1 hunks)
- include/oc_api.h (2 hunks)
- include/oc_cloud.h (4 hunks)
- include/oc_helpers.h (2 hunks)
- port/esp32/main/CMakeLists.txt (2 hunks)
- swig/Makefile (2 hunks)
- swig/java_lang/oc_iotivity_lite_jni.c (5 hunks)
- swig/java_lang/oc_iotivity_lite_jni.h (2 hunks)
- swig/swig_interfaces/iotivity.swg (1 hunks)
- swig/swig_interfaces/oc_cloud.i (2 hunks)
- swig/swig_interfaces/oc_endpoint.i (1 hunks)
- swig/swig_interfaces/oc_endpoint_address.i (1 hunks)
- util/oc_endpoint_address.c (1 hunks)
- util/oc_endpoint_address.h (1 hunks)
- util/oc_endpoint_address_internal.h (1 hunks)
- util/oc_features.h (1 hunks)
- util/unittest/endpoint_address_test.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (30)
- 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_manager.c
- api/cloud/oc_cloud_resource.c
- api/cloud/oc_cloud_store.c
- api/cloud/oc_cloud_store_internal.h
- 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_helpers.c
- include/oc_api.h
- include/oc_cloud.h
- include/oc_helpers.h
- port/esp32/main/CMakeLists.txt
- swig/Makefile
- swig/java_lang/oc_iotivity_lite_jni.c
- swig/java_lang/oc_iotivity_lite_jni.h
- swig/swig_interfaces/iotivity.swg
- swig/swig_interfaces/oc_cloud.i
- swig/swig_interfaces/oc_endpoint.i
- swig/swig_interfaces/oc_endpoint_address.i
- util/oc_endpoint_address.c
- util/oc_endpoint_address.h
- util/oc_endpoint_address_internal.h
- util/oc_features.h
- util/unittest/endpoint_address_test.cpp
Additional comments: 10
api/cloud/unittest/cloud_store_test.cpp (10)
- 36-36: The inclusion of
"util/oc_endpoint_address.h"aligns with the refactor fromoc_cloud_endpoints_ttooc_endpoint_addresses_t, ensuring the necessary types and functions are available for the tests.- 72-93: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [75-116]
The
compareEndpointsfunction has been updated to useoc_endpoint_addresses_tandoc_endpoint_address_tinstead of the previous types. This change is consistent with the refactor objectives. However, it's important to ensure that the comparison logic correctly handles all aspects ofoc_endpoint_address_t, including themetadatafield.
- 142-148: The
makeEmptyStorefunction has been updated to initializeoc_cloud_store_twithoc_cloud_endpoint_addresses_init, reflecting the transition to the new endpoint address management approach. This change is appropriate and ensures that the test setup aligns with the updated data structures.- 139-156: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [152-172]
The
makeStoreandfreeStorefunctions have been updated to accommodate the newoc_endpoint_addresses_tstructure. These changes are necessary for creating and cleaning up test data that aligns with the updated cloud store structure. It's crucial to ensure that all fields are correctly initialized and freed to prevent memory leaks or uninitialized data usage in tests.
- 240-240: The
Decode_ServersArraytest case demonstrates decoding a servers array from a CBOR payload into theoc_cloud_store_tstructure. This test case is crucial for validating the correct parsing of endpoint addresses from network payloads. It's important to ensure comprehensive coverage, including edge cases and error conditions.- 253-253: The
Decode_FailUnknownPropertytest case checks the behavior when an unknown property is encountered during decoding. This test is important for ensuring robust error handling in the presence of unexpected input. It would be beneficial to extend this pattern to other potential error conditions to ensure comprehensive validation.- 348-357: The
DumpAndLoad_MultipleEndpointstest case has been updated to test the functionality of adding multiple endpoints usingoc_endpoint_addresses_add. This change aligns with the refactor's objectives and is crucial for testing the handling of multiple endpoints. As previously commented, it's important to also test failure scenarios, such as adding an endpoint failing due to memory constraints, and ensure that the cleanup logic correctly handles partially initialized structures.- 399-399: The
SingleStoreDatatest case focuses on storing and loading a single piece of data, testing the persistence layer's basic functionality. Ensuring that this test case passes is fundamental to validating the cloud store's reliability for simple use cases.- 421-421: The
InvalidStoreDatatest case checks the behavior when invalid data is encountered during the loading process. This test is essential for ensuring that the system gracefully handles corrupt or unexpected data in the storage layer.- 460-460: The
DumpAsynctest case tests the asynchronous dumping of cloud store data, which is critical for ensuring that data persistence operations do not block critical execution paths. It's important to verify that asynchronous operations complete successfully and that the data integrity is maintained.
Danielius1922
force-pushed
the
adam/feature/endpoint-list
branch
from
74b79b2 to
2a02955
Compare
oc_endpoint_addresess_t is generalized oc_cloud_endpoints_t
Danielius1922
force-pushed
the
adam/feature/endpoint-list
branch
from
2a02955 to
cf259ed
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
tools/doxygen.iniis excluded by:!**/*.ini
Files selected for processing (33)
- api/cloud/oc_cloud.c (3 hunks)
- api/cloud/oc_cloud_context.c (5 hunks)
- api/cloud/oc_cloud_context_internal.h (1 hunks)
- api/cloud/oc_cloud_endpoints.c (1 hunks)
- api/cloud/oc_cloud_endpoints_internal.h (1 hunks)
- api/cloud/oc_cloud_manager.c (3 hunks)
- api/cloud/oc_cloud_resource.c (3 hunks)
- api/cloud/oc_cloud_store.c (5 hunks)
- api/cloud/oc_cloud_store_internal.h (3 hunks)
- api/cloud/unittest/cloud_context_test.cpp (1 hunks)
- api/cloud/unittest/cloud_manager_test.cpp (7 hunks)
- api/cloud/unittest/cloud_resource_test.cpp (1 hunks)
- api/cloud/unittest/cloud_store_test.cpp (18 hunks)
- api/cloud/unittest/cloud_test.cpp (10 hunks)
- api/oc_helpers.c (2 hunks)
- api/unittest/helperstest.cpp (1 hunks)
- include/oc_api.h (2 hunks)
- include/oc_cloud.h (4 hunks)
- include/oc_helpers.h (1 hunks)
- messaging/coap/coap_internal.h (1 hunks)
- port/esp32/main/CMakeLists.txt (2 hunks)
- swig/Makefile (2 hunks)
- swig/java_lang/oc_iotivity_lite_jni.c (5 hunks)
- swig/java_lang/oc_iotivity_lite_jni.h (2 hunks)
- swig/swig_interfaces/iotivity.swg (1 hunks)
- swig/swig_interfaces/oc_cloud.i (2 hunks)
- swig/swig_interfaces/oc_endpoint.i (1 hunks)
- swig/swig_interfaces/oc_endpoint_address.i (1 hunks)
- util/oc_endpoint_address.c (1 hunks)
- util/oc_endpoint_address.h (1 hunks)
- util/oc_endpoint_address_internal.h (1 hunks)
- util/oc_features.h (1 hunks)
- util/unittest/endpoint_address_test.cpp (1 hunks)
Files skipped from review as they are similar to previous changes (30)
- 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_manager.c
- api/cloud/oc_cloud_resource.c
- api/cloud/oc_cloud_store.c
- api/cloud/oc_cloud_store_internal.h
- 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_helpers.c
- include/oc_api.h
- include/oc_cloud.h
- include/oc_helpers.h
- port/esp32/main/CMakeLists.txt
- swig/Makefile
- swig/java_lang/oc_iotivity_lite_jni.c
- swig/java_lang/oc_iotivity_lite_jni.h
- swig/swig_interfaces/iotivity.swg
- swig/swig_interfaces/oc_cloud.i
- swig/swig_interfaces/oc_endpoint.i
- swig/swig_interfaces/oc_endpoint_address.i
- util/oc_endpoint_address.c
- util/oc_endpoint_address.h
- util/oc_endpoint_address_internal.h
- util/oc_features.h
- util/unittest/endpoint_address_test.cpp
Additional comments: 6
messaging/coap/coap_internal.h (1)
- 108-108: The explicit casting of
COAP_OPTION_SIZE1andOPTION_MAP_SIZEtosize_tbefore division and addition ensures that the calculation of theoptionsarray size is performed correctly on platforms where the default integer type might cause overflow or precision loss. This change enhances the portability and correctness of the code.api/cloud/unittest/cloud_store_test.cpp (5)
- 36-36: Including
"util/oc_endpoint_address.h"is necessary for the newoc_endpoint_addresses_tstructure and related functions. This change aligns with the refactor objectives.- 72-93: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [75-116]
The
compareEndpointsfunction has been updated to compareoc_endpoint_addresses_tstructures. This update is crucial for testing the equality of endpoint addresses in unit tests. The use ofstd::mapto compare endpoints by their URI is a good approach, ensuring that the order does not affect the comparison. Additionally, checking both the URI and the metadata (including UUID or name based on theid_type) is thorough.
- 142-147: The
makeEmptyStorefunction initializes anoc_cloud_store_tinstance with emptyoc_endpoint_addresses_t. This is a straightforward adaptation to the new structure and ensures that tests start with a clean state.- 172-172: Properly deinitializing
oc_endpoint_addresses_tinfreeStoreis essential for avoiding memory leaks. This change correctly cleans up the new endpoint addresses structure.- 348-354: The addition of multiple endpoints to
oc_endpoint_addresses_tinDumpAndLoad_MultipleEndpointstest case demonstrates the flexibility of the new structure. It's good to see the test coverage extending to scenarios with multiple endpoints.
|
oc_endpoint_addresess_t is generalized oc_cloud_endpoints_t
Summary by CodeRabbit
New Features
Refactor
Tests
Documentation
Chores
Style