Add OC_API annotations to oc_buffer_settings.h #647
Conversation
WalkthroughThe changes involve multiple updates across various files, primarily focusing on enhancing resource management and observation handling within the OC_SERVER context. Key modifications include the addition of new include directives, the removal of deprecated functions and structures, and the introduction of new functions to improve resource allocation, validation, and notification processes. Documentation updates are also present, enhancing clarity and export status for several functions. Changes
Suggested labels
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
🎉 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 (e4b351b), label this PR with |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
include/oc_buffer_settings.h (1)
87-88: Add documentation for the new oc_get_block_size function.The addition of the
oc_get_block_sizefunction with the OC_API annotation is good. However, unlike other functions in this file, it's missing documentation. Please add a documentation comment explaining the purpose of this function and what the returned block size represents.Consider adding documentation similar to other functions in this file:
/** * @brief retrieve the block size * * @return long the block size in bytes */ OC_API long oc_get_block_size(void);api/oc_buffer_settings.c (2)
105-110: Simplify the conditional compilation inoc_set_min_app_data_size.The preprocessor condition:
#if defined(OC_APP_DATA_BUFFER_SIZE) || !defined(OC_REP_ENCODING_REALLOC)results in the function doing nothing when either
OC_APP_DATA_BUFFER_SIZEis defined orOC_REP_ENCODING_REALLOCis not defined. This might be confusing. Consider revising the condition or adding comments to clarify under which configurations the function has an effect.
1-17: Ensure correct license and documentation comments.The header comment appears standard, but double-check that it complies with the project's licensing requirements and that all necessary attributions are included.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- api/oc_buffer_settings.c (1 hunks)
- api/oc_main.c (0 hunks)
- include/oc_buffer_settings.h (3 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- api/oc_main.c
🔇 Additional comments (12)
include/oc_buffer_settings.h (8)
24-25: LGTM: Include statement for OC_API macro added.The addition of
#include "oc_export.h"is appropriate as it likely defines the OC_API macro used in the function declarations below. This change aligns with the PR objective of adding OC_API annotations.
38-39: LGTM: OC_API annotation added to oc_set_mtu_size.The addition of the OC_API macro to the
oc_set_mtu_sizefunction declaration is correct and aligns with the PR objective. The function signature remains unchanged, maintaining backwards compatibility.
46-47: LGTM: OC_API annotation added to oc_get_mtu_size.The addition of the OC_API macro to the
oc_get_mtu_sizefunction declaration is correct and aligns with the PR objective. The function signature remains unchanged, maintaining backwards compatibility.
54-55: LGTM: OC_API annotation added to oc_set_max_app_data_size.The addition of the OC_API macro to the
oc_set_max_app_data_sizefunction declaration is correct and aligns with the PR objective. The function signature remains unchanged, maintaining backwards compatibility.
62-63: LGTM: OC_API annotation added to oc_get_max_app_data_size.The addition of the OC_API macro to the
oc_get_max_app_data_sizefunction declaration is correct and aligns with the PR objective. The function signature remains unchanged, maintaining backwards compatibility.
71-72: LGTM: OC_API annotation added to oc_set_min_app_data_size.The addition of the OC_API macro to the
oc_set_min_app_data_sizefunction declaration is correct and aligns with the PR objective. The function signature remains unchanged, maintaining backwards compatibility.
79-80: LGTM: OC_API annotation added to oc_get_min_app_data_size.The addition of the OC_API macro to the
oc_get_min_app_data_sizefunction declaration is correct and aligns with the PR objective. The function signature remains unchanged, maintaining backwards compatibility.
Line range hint
1-93: Overall assessment: Changes look good with a minor improvement needed.The changes in this file consistently add OC_API annotations to the existing functions, which aligns well with the PR objective. The addition of the new
oc_get_block_sizefunction is also good, but it needs documentation to maintain consistency with other functions in the file.Here's a summary of the changes:
- Added include for "oc_export.h"
- Added OC_API annotations to all existing functions
- Added a new function
oc_get_block_sizewith OC_API annotationOnce the documentation for
oc_get_block_sizeis added, this file will be in excellent shape.api/oc_buffer_settings.c (4)
30-33: Ensure consistent default MTU size calculation.In the
#elseblock forOC_INOUT_BUFFER_SIZE, the default_OC_MTU_SIZEis set to2048 + COAP_MAX_HEADER_SIZE. Verify that this default value aligns with the expected MTU size in the absence ofOC_INOUT_BUFFER_SIZE. If2048is meant to be a placeholder, consider defining it as a macro or explaining its derivation for clarity.
80-94: Ensure correct handling inoc_set_max_app_data_size.In the function
oc_set_max_app_data_size:
- When
OC_APP_DATA_BUFFER_SIZEis not defined,_OC_MAX_APP_DATA_SIZEis updated with the providedsize.- If
OC_REP_ENCODING_REALLOCis not defined,_OC_MIN_APP_DATA_SIZEis also updated, ensuring consistency between max and min sizes.Please confirm that this logic accurately reflects the intended behavior, especially in different configuration scenarios.
19-24: Include necessary headers for complete functionality.Ensure that all required headers are included, especially those related to the types used (e.g.,
size_t) and any external dependencies. For example, ifCOAP_MAX_HEADER_SIZEis defined elsewhere, verify that its header is included.Run the following script to check for required includes:
✅ Verification successful
To gather the necessary information, I'll need to examine the contents of the api/oc_buffer_settings.c file and search for any usage of COAP_MAX_HEADER_SIZE. Here's the script to do that:
The includes in api/oc_buffer_settings.c are correct and complete.
The file includes all necessary headers for the types and functions it uses. The COAP_MAX_HEADER_SIZE constant is correctly included via "messaging/coap/conf.h" inside a conditional block, which matches its conditional usage in the code. This design choice reduces unnecessary includes when dynamic allocation is not used.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for necessary header files. # List all include statements. rg --type c "^#include" # Search for the definition of COAP_MAX_HEADER_SIZE. rg --type c "define COAP_MAX_HEADER_SIZE"Length of output: 246276
Script:
#!/bin/bash # Search for COAP_MAX_HEADER_SIZE usage in api/oc_buffer_settings.c echo "Searching for COAP_MAX_HEADER_SIZE usage:" grep -n "COAP_MAX_HEADER_SIZE" api/oc_buffer_settings.c # Display the contents of api/oc_buffer_settings.c echo -e "\nContents of api/oc_buffer_settings.c:" cat api/oc_buffer_settings.cLength of output: 4587
48-71: Validate MTU size adjustment logic inoc_set_mtu_size.In the function
oc_set_mtu_size, there is conditional logic that adjusts_OC_MTU_SIZEand_OC_BLOCK_SIZEbased on the providedmtu_size. Please verify:
- The check
if (mtu_size < (COAP_MAX_HEADER_SIZE + 16))ensures the MTU size is sufficient. Confirm that the16byte overhead is appropriate.- The loop calculating
_OC_BLOCK_SIZEuses bit shifting to find the highest power of two less thanmtu_size. Ensure this correctly derives the desired block size.- When
OC_OSCOREis defined,_OC_MTU_SIZEis adjusted differently. Verify that this aligns with the security requirements.Run the following script to check for consistent usage of
COAP_MAX_HEADER_SIZEand calculations of MTU and block sizes:✅ Verification successful
MTU Size Adjustment Logic Verified.
- The
16byte overhead inif (mtu_size < (COAP_MAX_HEADER_SIZE + 16))is consistently enforced through both compile-time and run-time checks, ensuring compliance with protocol specifications.- The bit-shifting loop accurately calculates the highest power of two less than
mtu_size, correctly determining_OC_BLOCK_SIZE.- When
OC_OSCOREis defined,_OC_MTU_SIZEis appropriately adjusted to includeCOAP_MAX_HEADER_SIZE, aligning with security requirements.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check calculations of MTU and block sizes. # Search for all instances where MTU and block sizes are calculated. rg --type c "_OC_MTU_SIZE|_OC_BLOCK_SIZE|COAP_MAX_HEADER_SIZE"Length of output: 5253
Script:
#!/bin/bash # Description: Validate the 16-byte overhead and inspect the loop logic in oc_set_mtu_size. # Find where the 16-byte overhead is defined or used in relation to COAP_MAX_HEADER_SIZE rg --type c 'COAP_MAX_HEADER_SIZE\s*\+\s*16' # Extract the implementation of the oc_set_mtu_size function to inspect the loop logic ast-grep --lang c --pattern $'void oc_set_mtu_size($_) { $$$ }' api/oc_buffer_settings.cLength of output: 288
Script:
#!/bin/bash # Description: Extract the oc_set_mtu_size function with a flexible pattern and further investigate related calculations. # Attempt to extract the oc_set_mtu_size function with a relaxed pattern to account for different signatures ast-grep --lang c --pattern $'void oc_set_mtu_size($_) { $$$ }' api/oc_buffer_settings.c # If ast-grep fails, use ripgrep to locate the function definition rg --type c 'void\s+oc_set_mtu_size\s*\(' api/oc_buffer_settings.c # Investigate the source or documentation for the 16-byte overhead to understand its rationale rg --type c 'COAP_MAX_HEADER_SIZE\s*\+\s*16' -C 5 # Check definitions related to OC_OSCORE to verify security alignment rg --type c 'OC_OSCORE' --context 3Length of output: 49198
Script:
#!/bin/bash # Description: Extract the oc_set_mtu_size function with a flexible pattern and gather detailed context. # Attempt to extract the oc_set_mtu_size function with a flexible pattern to account for different return types and qualifiers ast-grep --lang c --pattern $'$_ oc_set_mtu_size($_) { $$$ }' api/oc_buffer_settings.c # If ast-grep fails, use ripgrep to locate the function definition and extract surrounding lines for context rg --type c 'oc_set_mtu_size\s*\(' api/oc_buffer_settings.c -A 20 -B 10 # Trace the origin of the 16-byte overhead to understand its rationale rg --type c 'COAP_MAX_HEADER_SIZE' api/oc_buffer_settings.c port/oc_connectivity.h -C 5 # Inspect the definitions and usages of OC_OSCORE to verify alignment with security requirements rg --type c 'OC_OSCORE' api/oc_buffer_settings.c port/oc_connectivity.h security/oc_oscore_engine.c security/oc_obt.c security/oc_tls.cLength of output: 9003
api/oc_buffer_settings.c
Outdated
| static size_t _OC_MAX_APP_DATA_SIZE = 7168; | ||
| static size_t _OC_MIN_APP_DATA_SIZE = 7168; | ||
| #else /* OC_APP_DATA_BUFFER_SIZE */ | ||
| static size_t _OC_MAX_APP_DATA_SIZE = 7168; | ||
| #ifdef OC_REP_ENCODING_REALLOC | ||
| static size_t _OC_MIN_APP_DATA_SIZE = 256; | ||
| #else /* OC_REP_ENCODING_REALLOC */ | ||
| static size_t _OC_MIN_APP_DATA_SIZE = 7168; | ||
| #endif /* !OC_REP_ENCODING_REALLOC */ | ||
| #endif /* !OC_APP_DATA_BUFFER_SIZE */ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify application data size settings.
The initialization of _OC_MAX_APP_DATA_SIZE and _OC_MIN_APP_DATA_SIZE has some redundancy:
- When
OC_APP_DATA_BUFFER_SIZEis defined, both sizes are set to7168. - When it's not defined,
_OC_MAX_APP_DATA_SIZEis set to7168, but_OC_MIN_APP_DATA_SIZEmay be either256or7168based onOC_REP_ENCODING_REALLOC.
Consider refactoring this section to eliminate redundancy and improve readability. Additionally, ensure that the default sizes are appropriate for the application's requirements.
| static size_t _OC_MIN_APP_DATA_SIZE = 7168; | ||
| #endif /* !OC_REP_ENCODING_REALLOC */ | ||
| #endif /* !OC_APP_DATA_BUFFER_SIZE */ | ||
| static size_t _OC_BLOCK_SIZE = 1024; // FIX |
There was a problem hiding this comment.
Address the FIX comment for _OC_BLOCK_SIZE.
The line:
static size_t _OC_BLOCK_SIZE = 1024; // FIXincludes a // FIX comment. This suggests that this value or implementation needs attention. Ensure that the block size is correctly calculated or defined, and remove the temporary comment once resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
include/oc_enums.h (1)
237-239: LGTM: Add OC_API to oc_str_to_enum_locn function. Minor documentation improvement suggested.The addition of the OC_API macro to the
oc_str_to_enum_locnfunction declaration is correct and aligns with the PR objective. This change appropriately exposes the function as part of the public API.Consider updating the function's documentation to reflect that the
oc_definedparameter is non-null. This can be done by changing:* @param oc_defined value to check if the conversion is successfull (cannot be * NULL)to:
* @param oc_defined [out] Non-null pointer to a boolean that will be set to indicate if the conversion was successfulThis change would make the documentation more consistent with the
OC_NONNULLattribute and provide clearer information about the parameter's usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- include/oc_enums.h (5 hunks)
- port/esp32/main/CMakeLists.txt (1 hunks)
- port/windows/vs2015/IoTivity-lite.vcxproj (1 hunks)
- port/windows/vs2015/IoTivity-lite.vcxproj.filters (1 hunks)
- swig/swig_interfaces/oc_buffer_settings.i (1 hunks)
- swig/swig_interfaces/oc_enums.i (1 hunks)
🔇 Additional comments (10)
swig/swig_interfaces/oc_buffer_settings.i (2)
25-26: LGTM: OC_API definition added correctlyThe addition of
#define OC_APIbefore including theoc_buffer_settings.hheader is correct and aligns with the PR objective of adding OC_API annotations.
Line range hint
18-22: Verify function renames mentioned in the AI summaryThe AI summary mentions renaming of 'getMaxAppDataSize' to 'oc_get_max_app_data_size' and 'getBlockSize' to 'oc_get_block_size'. However, the existing renames in the code seem to be in the opposite direction. Please verify if any changes are needed here.
Let's check for any missed renames or inconsistencies:
swig/swig_interfaces/oc_enums.i (1)
35-35: Consider the necessity and impact ofOC_APIin SWIG interfaceThe addition of
#define OC_APIseems to be related to API visibility in the C/C++ code. However, in the context of a SWIG interface file, this definition might not have the intended effect or be necessary.To ensure this change is consistent with other SWIG interface files and doesn't introduce any issues, please run the following script:
This script will help us understand if
OC_APIis consistently used across SWIG interfaces and if it's actually used in theoc_enums.hheader or other related headers.✅ Verification successful
OC_API definition is consistent across SWIG interface files
The addition of
#define OC_APIinswig/swig_interfaces/oc_enums.ialigns with its usage across other SWIG interface and header files. No issues detected.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for OC_API usage in other SWIG interface files # Test 1: Check if OC_API is defined in other SWIG interface files echo "Checking for OC_API in other SWIG interface files:" rg --type swig '#define\s+OC_API' swig/ # Test 2: Check if OC_API is used in the oc_enums.h header echo "Checking for OC_API usage in oc_enums.h:" rg --type c 'OC_API' include/oc_enums.h # Test 3: Check if OC_API is used in other header files echo "Checking for OC_API usage in other header files:" rg --type c 'OC_API' include/Length of output: 12247
include/oc_enums.h (5)
30-30: LGTM: Include "oc_export.h" for OC_API macro.The addition of
#include "oc_export.h"is appropriate and necessary for the OC_API macro used in the function declarations below.
208-209: LGTM: Add OC_API to oc_enum_to_str function.The addition of the OC_API macro to the
oc_enum_to_strfunction declaration is correct and aligns with the PR objective. This change appropriately exposes the function as part of the public API.
217-218: LGTM: Add OC_API to oc_enum_pos_desc_to_str function.The addition of the OC_API macro to the
oc_enum_pos_desc_to_strfunction declaration is correct and aligns with the PR objective. This change appropriately exposes the function as part of the public API.
226-227: LGTM: Add OC_API to oc_enum_locn_to_str function.The addition of the OC_API macro to the
oc_enum_locn_to_strfunction declaration is correct and aligns with the PR objective. This change appropriately exposes the function as part of the public API.
Line range hint
1-262: Overall assessment: Changes look good and align with PR objectives.The modifications to
include/oc_enums.hconsistently add the OC_API macro to the relevant function declarations, which aligns with the PR objective of adding OC_API annotations. These changes appropriately expose the functions as part of the public API, improving their visibility and linkage.The inclusion of "oc_export.h" is correct and necessary for the OC_API macro definition. All changes are consistent and do not introduce any issues that would prevent merging this PR.
Great job on improving the API visibility of these functions!
port/windows/vs2015/IoTivity-lite.vcxproj (1)
357-357: LGTM: New source file correctly added to the project.The addition of
oc_buffer_settings.cto the project file is correct and aligns with the PR objectives. This ensures that the new functionality related to buffer settings will be compiled and linked properly.port/windows/vs2015/IoTivity-lite.vcxproj.filters (1)
116-118: LGTM: New file added correctly to the project.The addition of
oc_buffer_settings.cto the project is correct and consistent with the existing structure. It's appropriately placed in the "Core" filter, which aligns with its likely functionality as a core component of the IoTivity-lite project.To ensure this file exists and is in the correct location, run the following script:
✅ Verification successful
Verification Successful:
oc_buffer_settings.cis correctly added to the project.The file exists in the expected location and is appropriately placed within the "Core" filter. No issues detected.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence and location of oc_buffer_settings.c # Check if the file exists in the expected location if fd -p "api/oc_buffer_settings.c" ; then echo "File oc_buffer_settings.c found in the correct location." else echo "Error: oc_buffer_settings.c not found in the expected location." fi # Check if there are any other files with similar names that might have been misnamed fd -p "oc_buffer_settings.*\.c"Length of output: 239
| @@ -30,6 +30,7 @@ set(sources | |||
| ${CMAKE_CURRENT_SOURCE_DIR}/../../../api/client/oc_client_cb.c | |||
| ${CMAKE_CURRENT_SOURCE_DIR}/../../../api/oc_base64.c | |||
| ${CMAKE_CURRENT_SOURCE_DIR}/../../../api/oc_blockwise.c | |||
| ${CMAKE_CURRENT_SOURCE_DIR}/../../../api/oc_buffer_settings.c | |||
There was a problem hiding this comment.
💡 Codebase verification
Issue Found: Incorrect path for oc_buffer_settings.c in CMakeLists.txt.
The file oc_buffer_settings.c exists at ./api/oc_buffer_settings.c, but the path specified in port/esp32/main/CMakeLists.txt is incorrect. Please update the path to the correct location in the CMake configuration.
🔗 Analysis chain
LGTM: New source file added for buffer settings management.
The addition of ${CMAKE_CURRENT_SOURCE_DIR}/../../../api/oc_buffer_settings.c to the list of source files is appropriate. This change aligns with the PR objective of adding OC_API annotations to buffer settings, and it's consistent with the implementation of new buffer settings management functions mentioned in the AI summary.
To ensure the new file exists in the specified location, please run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of the new source file.
# Test: Check if the file exists
if [ -f "${CMAKE_CURRENT_SOURCE_DIR}/../../../api/oc_buffer_settings.c" ]; then
echo "File oc_buffer_settings.c exists in the expected location."
else
echo "Error: File oc_buffer_settings.c not found in the expected location."
exit 1
fi
# Test: Check if the file is not empty
if [ -s "${CMAKE_CURRENT_SOURCE_DIR}/../../../api/oc_buffer_settings.c" ]; then
echo "File oc_buffer_settings.c is not empty."
else
echo "Warning: File oc_buffer_settings.c is empty."
fi
Length of output: 203
Script:
#!/bin/bash
# Description: Search the repository for oc_buffer_settings.c
# Search for the file in the repository
fd "oc_buffer_settings\.c$" --exec echo "Found: {}"
Length of output: 87
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (15)
swig/swig_interfaces/oc_session_events.i (1)
42-42: LGTM! Consider adding documentation for the new macro.The addition of the
OC_DEPRECATEDmacro is a good preparation for managing deprecated APIs in the future. This change doesn't affect the current functionality but provides a tool for better API lifecycle management.Consider adding a comment above the macro definition to explain its purpose and usage. For example:
// Macro to mark deprecated functions or methods. Usage: OC_DEPRECATED(message) before the function declaration. #define OC_DEPRECATED(...)include/oc_network_monitor.h (2)
49-49: LGTM: New function added with correct annotationThe addition of the
oc_remove_network_interface_event_callbackfunction is a good improvement, providing symmetry with the existing add function. It's correctly annotated withOC_APIand has a consistent signature with the add function.Consider updating the comment above the function to include the same level of detail as the add function, specifically mentioning that the callback must not be NULL:
/** * @brief Remove the callback to receive change notifications for Network * interface event. * @param cb The callback to be removed. Must not be NULL. * @return 0 on success * @return -1 on error */
Detected usages of removed functions in the codebase
Several parts of the codebase still use the removed functions
oc_add_session_event_callbackandoc_remove_session_event_callback. These usages could lead to compatibility issues.Key locations:
port/unittest/connectivitytest.cpp:
- Multiple test cases using
oc_add_session_event_callbackandoc_remove_session_event_callback.include/oc_session_events.h:
- Deprecated function declarations still present.
api/oc_session_events.c:
- Implementations of deprecated functions.
api/plgd/unittest/plgdtimetest.cpp:
- Test cases invoking deprecated functions.
- Other ports and API implementations also reference these deprecated functions.
🔗 Analysis chain
Line range hint
1-53: Verify impact of API changes and update documentationThe changes to this header file represent a significant update to the network monitoring API:
- Removal of deprecated session event callback functions.
- Clear marking of public API functions with
OC_API.- Addition of a new function to remove network interface event callbacks.
These changes improve the API's clarity and completeness. However, they may break backwards compatibility for any code still using the removed functions.
Please run the following script to check for any remaining uses of the removed functions in the codebase:
Also, ensure that the API documentation (if separate from this header) is updated to reflect these changes, including the removal of deprecated functions and the addition of the new remove function.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for uses of removed functions in the codebase echo "Checking for uses of removed functions:" rg --type c --type cpp "oc_add_session_event_callback(_v1)?" || echo "No uses of oc_add_session_event_callback found" rg --type c --type cpp "oc_remove_session_event_callback(_v1)?" || echo "No uses of oc_remove_session_event_callback found" echo "Checking for potential uses of new function:" rg --type c --type cpp "oc_remove_network_interface_event_callback" || echo "No uses of oc_remove_network_interface_event_callback found"Length of output: 7999
port/oc_storage.h (1)
47-53: LGTM: OC_API and OC_NONNULL added to oc_storage_readThe changes to oc_storage_read are well-implemented:
- The OC_API annotation correctly marks this function as part of the public API.
- The OC_NONNULL() attribute enhances safety by enforcing non-null parameters.
- The updated comments clearly indicate which parameters cannot be NULL.
These modifications improve both the usability and safety of the function.
Consider updating the return value comment to clarify the behavior on error:
- * @return long amount of bytes read + * @return long amount of bytes read on success, or a negative value on errorapi/unittest/buffersettingstest.cpp (4)
25-37: Consider adding more test cases for SetMTUSize.The current test case checks for an invalid MTU size, which is good. However, to ensure comprehensive coverage, consider adding the following test cases:
- Setting a valid MTU size and verifying it's set correctly.
- Setting the minimum allowed MTU size.
- Setting the maximum allowed MTU size.
This will help ensure that the function behaves correctly across its entire range of valid inputs.
39-46: Enhance the SetMaxAppDataSize test case.While the current test case covers basic functionality, consider the following improvements:
- Test setting the maximum allowed app data size.
- Test setting a size larger than the maximum allowed and verify it fails or is capped.
- Test setting the minimum allowed app data size.
- Verify that setting an invalid size (e.g., 0 or a negative value) is handled correctly.
These additions will provide more comprehensive coverage of the
oc_set_max_app_data_sizefunction's behavior.
62-85: Improve test descriptions for non-dynamic allocation scenarios.The current tests effectively check the behavior when dynamic allocation is disabled. To enhance clarity and maintainability, consider the following improvements:
- Add comments or update the test names to explain why -1 is the expected result in each case.
- For the
GetBlockSizetest, explain the significance of the -1 return value.Example:
TEST(BufferSettings, SetMTUSize_ReturnsNegativeOneWhenDynamicAllocationDisabled) { // When dynamic allocation is disabled, setting MTU size should not be possible EXPECT_EQ(-1, oc_set_mtu_size(42)); EXPECT_EQ(-1, oc_get_mtu_size()); }This will make the tests more self-documenting and easier to understand for future maintainers.
1-87: Consider enhancing overall test structure and coverage.The test file provides good coverage for different compilation scenarios. To further improve it, consider the following suggestions:
- Add a test fixture (e.g.,
BufferSettingsTest) to manage common setup and teardown operations, if any.- Group related tests using test suites (e.g.,
TEST_F(BufferSettingsTest, DynamicAllocation)andTEST_F(BufferSettingsTest, StaticAllocation)).- Add edge case tests, such as testing with maximum and minimum valid values for each setting.
- Consider adding tests for any error logging or error handling mechanisms in the buffer settings functions.
- If possible, add tests that verify the actual impact of these settings on the buffer behavior in the system.
These improvements will enhance the overall quality and maintainability of the test suite.
include/oc_session_events.h (3)
80-91: LGTM: New function addition with proper deprecation noticeThe addition of
oc_add_session_event_callbackwith a clear deprecation notice is well-implemented. The function signature, documentation, and return values are clearly defined.Consider adding a brief explanation in the documentation about why this function is deprecated and the benefits of using the new
v1version. This can help users understand the motivation behind the change and encourage them to migrate to the new function.
93-103: LGTM: New v1 function with improved flexibilityThe addition of
oc_add_session_event_callback_v1with the extrauser_dataparameter is a good improvement. It provides more flexibility for users and follows a common pattern for callback functions.Consider adding a brief example in the documentation showing how to use this function, especially demonstrating the use of the
user_dataparameter. This can help users quickly understand how to migrate from the deprecated version to this new one.
117-132: LGTM: New v1 removal function with improved flexibilityThe addition of
oc_remove_session_event_callback_v1with the extrauser_dataandignore_user_dataparameters is a good improvement. It provides more flexibility for users when removing callbacks and follows a consistent pattern with the add function.Consider adding a brief explanation or example in the documentation showing how the
ignore_user_dataparameter affects the behavior of the function. This can help users understand when and how to use this parameter effectively.api/plgd/device-provisioning-client/plgd_dps.c (1)
232-242: LGTM: Conditional network monitoring callback registrationThe changes to
plgd_dps_interface_callbacks_initandplgd_dps_interface_callbacks_deinitare consistent with the conditional inclusion of the network monitoring header. This ensures that network monitoring functionality is only active when OC_NETWORK_MONITOR is defined, allowing for better control over feature inclusion.For consistency, consider adding a comment to explain why these functions are empty when OC_NETWORK_MONITOR is not defined. For example:
void plgd_dps_interface_callbacks_init(void) { +#ifndef OC_NETWORK_MONITOR + // No-op when network monitoring is disabled +#else oc_add_network_interface_event_callback(dps_interface_event_handler); #endif /* OC_NETWORK_MONITOR */ } void plgd_dps_interface_callbacks_deinit(void) { +#ifndef OC_NETWORK_MONITOR + // No-op when network monitoring is disabled +#else oc_remove_network_interface_event_callback(dps_interface_event_handler); #endif /* OC_NETWORK_MONITOR */ }This addition would make the intent clearer and improve code readability.
port/unittest/connectivitytest.cpp (1)
Line range hint
160-180: LGTM! Consider adding more comprehensive tests.The addition of the
interface_event_handlerfunction and the modification of thehandle_network_interface_event_callbacktest case look good. The changes improve the testability of the network monitoring feature.Consider expanding the test case to cover more scenarios, such as:
- Testing with different event types (not just NETWORK_INTERFACE_UP).
- Verifying the behavior when multiple callbacks are registered.
- Testing the order of callback execution if applicable.
This would provide more comprehensive coverage of the network monitoring functionality.
port/linux/ipadapter.c (2)
43-46: LGTM! Consider moving the include statement.The conditional inclusion of
oc_network_monitor.his correct and allows for flexible compilation based on theOC_NETWORK_MONITORfeature flag.For better code organization, consider moving this include statement to the block of other conditional includes (e.g., near the
OC_SESSION_EVENTSandOC_TCPblocks) to keep all feature-dependent includes together.
Line range hint
1-1330: Consider refactoring for improved maintainability and readability.While the changes made are correct, there are several areas where the overall file could be improved:
File size: This file is quite long and handles multiple responsibilities. Consider splitting it into smaller, more focused files (e.g., separate files for IPv4 and IPv6 handling, event management, etc.).
Function complexity: Some functions, like
network_event_thread,process_interface_change_event, andinitialize_ip_context, are quite long and complex. Consider refactoring these into smaller, more focused functions to improve readability and maintainability.Code duplication: There are instances of similar code for IPv4 and IPv6 handling. Consider creating shared utility functions to reduce duplication.
Error handling: Some error messages could be more descriptive. Consider adding more context to error messages, especially in network initialization and event handling sections.
Here's an example of how you might start refactoring the
initialize_ip_contextfunction:static bool initialize_ip_context_common(ip_context_t *dev, size_t device) { dev->device = device; OC_LIST_STRUCT_INIT(dev, eps); if (pthread_mutex_init(&dev->rfds_mutex, NULL) != 0) { oc_abort("error initializing TCP adapter mutex"); } if (pipe(dev->wakeup_pipe) < 0) { OC_ERR("wakeup pipe: %d", errno); return false; } if (!oc_fcntl_set_nonblocking(dev->wakeup_pipe[0])) { OC_ERR("Could not set non-block wakeup_pipe[0]"); return false; } return true; } static bool initialize_ip_context(ip_context_t *dev, size_t device, oc_connectivity_ports_t ports) { if (!initialize_ip_context_common(dev, device)) { return false; } if (!initialize_ip_context_ipv6(dev, ports)) { return false; } #ifdef OC_IPV4 if (!initialize_ip_context_ipv4(dev, ports)) { OC_ERR("Could not initialize IPv4"); return false; } #endif // ... rest of the function ... }This refactoring separates the common initialization logic and makes the function easier to read and maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
- api/cloud/oc_cloud.c (1 hunks)
- api/oc_buffer_settings.c (1 hunks)
- api/oc_session_events.c (0 hunks)
- api/plgd/device-provisioning-client/plgd_dps.c (2 hunks)
- api/plgd/device-provisioning-client/plgd_dps_manager.c (0 hunks)
- api/plgd/unittest/plgdtimetest.cpp (0 hunks)
- api/unittest/buffersettingstest.cpp (1 hunks)
- include/oc_network_monitor.h (2 hunks)
- include/oc_session_events.h (2 hunks)
- include/oc_sp.h (2 hunks)
- port/android/ipadapter.c (1 hunks)
- port/esp32/adapter/src/ipadapter.c (1 hunks)
- port/linux/ipadapter.c (1 hunks)
- port/oc_poll_loop.h (2 hunks)
- port/oc_storage.h (2 hunks)
- port/unittest/connectivitytest.cpp (1 hunks)
- port/windows/ipadapter.c (1 hunks)
- swig/swig_interfaces/oc_session_events.i (1 hunks)
💤 Files not reviewed due to no reviewable changes (3)
- api/oc_session_events.c
- api/plgd/device-provisioning-client/plgd_dps_manager.c
- api/plgd/unittest/plgdtimetest.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- api/oc_buffer_settings.c
🔇 Additional comments (25)
include/oc_network_monitor.h (1)
38-38: LGTM: OC_API annotation added correctlyThe addition of the
OC_APIannotation to theoc_add_network_interface_event_callbackfunction is correct and aligns with the PR objective. This clearly marks the function as part of the public API, which is a good practice for API design and documentation.port/oc_storage.h (4)
24-26: LGTM: New includes added for API annotationsThe addition of "oc_export.h" and "util/oc_compiler.h" is appropriate for supporting the new OC_API and OC_NONNULL annotations. This change enhances consistency across the project and prepares the functions for use in the public API.
41-42: LGTM: OC_API annotation added to oc_storage_configThe addition of the OC_API annotation to oc_storage_config is appropriate and aligns with the PR objectives. This change clearly marks the function as part of the public API, improving the interface's clarity for users of the library.
58-66: LGTM: OC_API and OC_NONNULL added to oc_storage_writeThe modifications to oc_storage_write are well-implemented and consistent with the changes made to oc_storage_read:
- The OC_API annotation correctly marks this function as part of the public API.
- The OC_NONNULL() attribute enhances safety by enforcing non-null parameters.
- The updated comments clearly indicate which parameters cannot be NULL.
These changes improve both the usability and safety of the function, maintaining consistency across the API.
Line range hint
1-73: Overall assessment: Excellent improvements to API clarity and safetyThe changes made to this file consistently enhance the public API by:
- Adding OC_API annotations to clearly mark public functions.
- Introducing OC_NONNULL attributes to improve function call safety.
- Updating comments to provide clearer documentation on parameter constraints.
These modifications align well with the PR objectives and significantly improve the usability and safety of the storage-related functions. The consistent application of these changes across all affected functions is commendable.
port/oc_poll_loop.h (8)
32-32: LGTM: Necessary include for OC_API macroThe addition of
#include "oc_export.h"is appropriate and necessary for theOC_APImacro used in the new function declarations.
42-43: LGTM: Well-defined initialization functionThe
oc_poll_loop_initfunction declaration is clear and consistent with the PR objectives. TheOC_APIannotation and the brief comment provide good context for its usage.
46-47: LGTM: Clear shutdown function declarationThe
oc_poll_loop_shutdownfunction declaration is appropriate and aligns with the PR objectives. TheOC_APIannotation and the brief comment provide good context for its usage.
50-51: LGTM: Well-defined main loop execution functionThe
oc_poll_loop_runfunction declaration is clear and consistent with the PR objectives. TheOC_APIannotation and the brief comment provide good context for its usage.
54-55: LGTM: Clear signaling function for the main loopThe
oc_poll_loop_signalfunction declaration is appropriate and aligns with the PR objectives. TheOC_APIannotation and the brief comment provide good context for its usage.
58-59: LGTM: Well-defined termination function for the main loopThe
oc_poll_loop_terminatefunction declaration is clear and consistent with the PR objectives. TheOC_APIannotation and the brief comment provide good context for its usage.
62-63: LGTM: Clear status check function for main loop terminationThe
oc_poll_loop_is_terminatedfunction declaration is appropriate and aligns with the PR objectives. TheOC_APIannotation and the brief comment provide good context for its usage.
Line range hint
32-63: Overall: Excellent addition of polling loop APIThe changes to this file are well-structured and align perfectly with the PR objectives. The new function declarations provide a comprehensive API for managing the polling loop, including initialization, execution, signaling, termination, and status checking. The consistent use of
OC_APIannotations and theOC_HAS_FEATURE_SIMPLE_MAIN_LOOPfeature flag demonstrate good coding practices.These additions enhance the functionality of the polling loop by providing clear mechanisms for its lifecycle management. The API is well-documented with brief comments, making it easier for developers to understand and use these functions.
api/unittest/buffersettingstest.cpp (1)
1-23: LGTM: Proper file header and includes.The file starts with an appropriate Apache 2.0 license header and includes the necessary headers for the unit tests.
include/oc_session_events.h (3)
30-30: LGTM: Utility header inclusionThe addition of the "util/oc_compiler.h" header is appropriate. It likely provides necessary compiler-specific macros or utilities for the new functionality introduced in this file.
105-115: LGTM: Removal function with proper deprecation noticeThe addition of
oc_remove_session_event_callbackwith a clear deprecation notice is well-implemented. The function signature, documentation, and return values are clearly defined.
Line range hint
1-133: Overall assessment: Well-implemented API evolutionThe changes in this file represent a well-executed evolution of the session events API. The introduction of
v1functions with additionaluser_dataparameters provides more flexibility for users while maintaining backward compatibility through properly deprecated older functions. The consistency between the newaddandremovefunctions is commendable.Key improvements:
- Added flexibility with
user_dataparameter in callback functions.- Proper deprecation of older functions.
- Consistent API design between new functions.
- Clear documentation of function signatures and return values.
These changes should make the API more versatile and easier to use in various contexts. The minor suggestions for documentation improvements, if implemented, will further enhance the usability of these new functions.
include/oc_sp.h (3)
28-29: LGTM: Include directive for API export definitions.The addition of the
"oc_export.h"include is appropriate. This header likely contains definitions for theOC_APImacro used later in the file, which is consistent with the PR objective of adding OC_API annotations.
131-133: LGTM: OC_API annotation added to function declaration.The
OC_APImacro has been correctly added to theoc_pki_set_security_profilefunction declaration. This change aligns with the PR objective of adding OC_API annotations to public API functions. The function signature remains unchanged, which preserves backward compatibility.
Line range hint
1-141: Overall assessment: Changes are appropriate and align with PR objectives.The modifications to this file, including the addition of the
"oc_export.h"include and theOC_APIannotation to theoc_pki_set_security_profilefunction, are consistent with the PR objective of adding OC_API annotations. These changes improve the API visibility without introducing breaking changes. The implementation is clean and maintains the existing functionality.api/plgd/device-provisioning-client/plgd_dps.c (2)
33-36: LGTM: Conditional inclusion of network monitoring headerThe addition of the conditional compilation directive for including "oc_network_monitor.h" is a good practice. It allows for more flexible compilation based on whether the OC_NETWORK_MONITOR feature is enabled.
Line range hint
1-542: Overall assessment: Changes look goodThe modifications to conditionally include network monitoring functionality are well-implemented and consistent throughout the file. These changes improve the flexibility of the code by allowing selective compilation of network monitoring features based on the OC_NETWORK_MONITOR macro.
A minor suggestion was made to improve code readability by adding comments to explain empty function bodies when network monitoring is disabled.
api/cloud/oc_cloud.c (1)
43-45: Approve: Good use of conditional compilation for optional featureThe introduction of conditional inclusion for
oc_network_monitor.his a positive change. It allows for more flexible compilation by including the network monitor functionality only when theOC_NETWORK_MONITORmacro is defined. This approach is beneficial for:
- Reducing compilation time and binary size when the feature is not needed.
- Allowing easier configuration of builds with or without network monitoring.
- Improving code modularity by clearly separating optional features.
port/android/ipadapter.c (1)
43-46: Approved: Good use of conditional compilation for optional feature.The addition of the conditional inclusion for "oc_network_monitor.h" is a good practice. It allows for the network monitoring functionality to be optionally compiled based on the OC_NETWORK_MONITOR macro. This approach helps in managing optional features and keeping the codebase modular.
port/esp32/adapter/src/ipadapter.c (1)
35-38: Appropriate use of conditional compilationThe addition of the conditional include for "oc_network_monitor.h" is well-placed and follows good practices for optional feature inclusion. This change allows for more flexible compilation based on the presence of the network monitoring feature.
| #ifdef OC_NETWORK_MONITOR | ||
| #include "oc_network_monitor.h" | ||
| #endif /* OC_NETWORK_MONITOR */ |
There was a problem hiding this comment.
💡 Codebase verification
Conditional Compilation Required for Multiple Files
The inclusion of oc_network_monitor.h was found in several files without conditional compilation. To maintain consistency and modularity, please wrap these includes with #ifdef OC_NETWORK_MONITOR directives as done in port/windows/ipadapter.c.
Affected files:
port/unittest/connectivitytest.cppport/linux/ipadapter.cport/android/ipadapter.cport/esp32/adapter/src/ipadapter.capi/plgd/device-provisioning-client/plgd_dps.capi/cloud/oc_cloud.c
🔗 Analysis chain
Approved: Good use of conditional compilation for optional feature.
The addition of the #ifdef OC_NETWORK_MONITOR directive for including oc_network_monitor.h is a good practice. It allows for conditional compilation of network monitoring features, improving modularity and flexibility of the codebase.
Let's check if there are any other occurrences of oc_network_monitor.h that might need similar treatment:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of oc_network_monitor.h
rg --type c --type cpp 'oc_network_monitor\.h'
Length of output: 483
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (6)
port/oc_dns.h (2)
19-21: Enhance the file header commentWhile the file header comment is present, it could be more informative. Consider adding a brief description of the file's purpose and the functionality it provides.
Here's a suggested improvement:
/** * @file + * @brief DNS lookup functionality for the OC (Open Connectivity) framework + * + * This file declares functions for DNS resolution and cache management. */
35-46: Enhance documentation for oc_dns_lookup functionWhile the function documentation provides basic information, it could be more detailed to improve clarity and usability.
Consider expanding the documentation as follows:
/** * @brief dns look up * - * @param domain the url - * @param addr the address - * @param flags the transport flags - * @return int 0 = success + * @param domain The domain name to be resolved + * @param addr Pointer to oc_string_t where the resolved address will be stored + * @param flags Transport flags to specify the type of address resolution needed + * @return int 0 on successful resolution, non-zero value on failure + * + * This function performs a DNS lookup for the given domain name and stores + * the resolved address in the provided oc_string_t structure. The transport + * flags parameter can be used to specify additional requirements for the + * address resolution. */port/oc_connectivity_internal.h (1)
85-92: LGTM: Updated documentation foroc_connectivity_wakeupis clear.The expanded documentation for
oc_connectivity_wakeupprovides better clarity on the function's purpose. The#ifdef OC_DYNAMIC_ALLOCATIONblock appropriately limits the function's availability to when dynamic allocation is enabled.Consider adding a brief note about why this function is only available with dynamic allocation, if there's a specific reason. This could help developers understand the constraints and use cases better.
port/oc_connectivity.h (3)
152-154: LGTM: API visibility and deprecation notice addedThe addition of
OC_APIandOC_DEPRECATEDmacros tooc_connectivity_end_sessionis appropriate. It clearly indicates that the function is part of the public API but is deprecated.Consider the following suggestions:
- Update the function's documentation to prominently mention its deprecated status.
- Add a link or reference to the documentation for the new
oc_close_sessionfunction.- If not already done, ensure that release notes or migration guides mention this deprecation.
162-163: LGTM: API visibility addedThe addition of the
OC_APImacro tooc_connectivity_get_endpointsis appropriate, clearly indicating that this function is part of the public API.Consider enhancing the function's documentation:
- Add information about the lifecycle of the returned
oc_endpoint_t*(e.g., whether the caller is responsible for freeing it).- Specify the behavior when no endpoints are available (e.g., returns NULL or an empty list).
- If applicable, mention any thread-safety considerations for this function.
Line range hint
1-280: Overall assessment: Positive changes improving API clarity and organizationThe modifications in this file contribute to better API management and code organization:
- DNS functionality has been modularized by moving it to a separate header file.
- Public API functions are now clearly marked with the
OC_APImacro.- Deprecated functions are properly annotated, guiding users towards newer alternatives.
These changes should improve the maintainability of the codebase and provide clearer guidance to API users. The consistent application of these changes suggests a systematic approach to API management.
As the project continues to evolve:
- Consider implementing a formal API versioning system if not already in place.
- Regularly review and update API documentation to reflect these changes.
- Maintain a comprehensive changelog or migration guide to assist users in adapting to API changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- api/unittest/buffersettingstest.cpp (1 hunks)
- port/oc_connectivity.h (2 hunks)
- port/oc_connectivity_internal.h (2 hunks)
- port/oc_dns.h (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/unittest/buffersettingstest.cpp
🔇 Additional comments (7)
port/oc_dns.h (3)
23-33: LGTM: Header guards and includes are well-structuredThe header guards, necessary includes, and C++ compatibility handling are all correctly implemented.
Also applies to: 60-64
1-64: Overall, the new DNS functionality is well-structuredThe introduction of DNS lookup functionality in this header file is well-organized with proper header guards, includes, and conditional compilation. The use of the OC_API macro for function declarations is consistent and appropriate.
Key points:
- The file structure follows good practices for C header files.
- Conditional compilation allows for flexible feature inclusion.
- Function declarations are clear and use appropriate types.
Areas for improvement:
- Enhance documentation for both functions to provide more context and usage details.
- Verify the implementation of
oc_dns_clear_cachein the corresponding source file.These improvements will enhance the usability and maintainability of the DNS functionality in the OC framework.
48-56: Enhance documentation for oc_dns_clear_cache function and clarify implementationThe function documentation for
oc_dns_clear_cacheis minimal and could be expanded to provide more context.Consider enhancing the documentation as follows:
/** * @brief clear the DNS cache + * + * This function clears the DNS cache, removing all stored DNS resolutions. + * It's useful for ensuring fresh DNS lookups or managing memory usage. + * + * @note This function is only available when OC_DNS_CACHE is defined. */Additionally, the implementation details of this function are not provided in the header file.
Could you clarify where the implementation of this function can be found? It would be helpful to ensure that the implementation exists and matches the intended functionality.
port/oc_connectivity_internal.h (3)
56-63: LGTM: New functionoc_send_bufferlooks good.The new function
oc_send_bufferis well-documented and consistent with the existing code style. The function signature and return value definitions are clear and appropriate for the task.
78-83: LGTM: New functionoc_send_discovery_requestis appropriately defined.The new function
oc_send_discovery_requestis consistent with the existing code style and has an appropriate signature for its purpose. The documentation, while minimal, is sufficient for an internal header file.
Line range hint
56-92: Summary: Enhancements to connectivity API look good overall.The changes to
port/oc_connectivity_internal.hintroduce two new functions (oc_send_bufferandoc_send_discovery_request) and improve the documentation foroc_connectivity_wakeup. These additions enhance the connectivity API's functionality and clarity. The changes are consistent with the existing code style and conventions, and there are no apparent issues with the implementations.port/oc_connectivity.h (1)
32-32: LGTM: DNS functionality modularizationThe addition of
#include "port/oc_dns.h"is a good step towards better code organization. It aligns with the removal of DNS-related functions from this file.To ensure completeness, please verify that all necessary DNS-related declarations are present in the new
oc_dns.hfile:✅ Verification successful
Verification Successful: DNS Declarations Present
The
port/oc_dns.hfile exists and contains the necessary DNS-related function declarations. The inclusion inport/oc_connectivity.his correctly handled.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the contents of the new oc_dns.h file # Expected: File should exist and contain DNS-related function declarations cat port/oc_dns.hLength of output: 1468
Danielius1922
force-pushed
the
adam/feature/182-export-public-api
branch
from
89c88f2
to
a9fb620
Compare
Danielius1922
force-pushed
the
adam/feature/182-export-public-api
branch
from
a9fb620
to
fae64b4
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
port/oc_connectivity_internal.h (1)
116-124: Approved: Deprecated functionoc_connectivity_end_sessionis clearly marked.The deprecation notice and use of the OC_DEPRECATED macro are good practices. This helps developers transition to the new
oc_close_sessionfunction.Consider adding a TODO comment to remove this function in a future version, helping to track technical debt:
+// TODO: Remove this function in a future version (e.g., v2.3.0) void oc_connectivity_end_session(const oc_endpoint_t *endpoint) OC_DEPRECATED("replaced by oc_close_session in v2.2.5.14");security/oc_oscore_engine.c (1)
Add OSCORE Functionality Tests
It appears that there are no existing OSCORE-related test files. To ensure that the recent inclusion of the new include statement does not introduce any unintended side effects, please add comprehensive tests covering OSCORE encryption, decryption, and message handling processes.
🔗 Analysis chain
Line range hint
1-1024: Ensure thorough testing of OSCORE functionality.While the only visible change is the addition of a new include statement, it's important to verify that this change doesn't introduce any unintended side effects. Please ensure that all OSCORE-related functionality is thoroughly tested, including encryption, decryption, and message handling processes.
To help verify the OSCORE functionality, you can run the following commands:
This will help identify the existing OSCORE tests and their contents, allowing you to ensure that all relevant scenarios are covered.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for OSCORE-related test files and their contents # Find OSCORE-related test files echo "OSCORE-related test files:" fd -e c -e cpp -e h oscore tests # Check contents of OSCORE-related test files echo "\nContents of OSCORE-related test files:" fd -e c -e cpp -e h oscore tests -x catLength of output: 230
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- api/oc_message_buffer.c (1 hunks)
- api/unittest/buffersettingstest.cpp (1 hunks)
- port/oc_connectivity.h (2 hunks)
- port/oc_connectivity_internal.h (4 hunks)
- port/oc_dns.h (1 hunks)
- security/oc_oscore_engine.c (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- api/unittest/buffersettingstest.cpp
- port/oc_connectivity.h
- port/oc_dns.h
🔇 Additional comments (3)
port/oc_connectivity_internal.h (2)
57-64: LGTM: New functionoc_send_bufferlooks good.The function signature, documentation, and return value definitions are clear and consistent with the file's style. The addition of this function enhances the connectivity functionality as intended.
78-83: LGTM: New functionoc_send_discovery_requestis well-defined.The function signature and documentation are clear and consistent with the file's style. This addition enhances the discovery functionality within the connectivity module.
api/oc_message_buffer.c (1)
Line range hint
1-214: Ensure thorough testing of network-related functionalities.Given the change in the connectivity header and the presence of network-related functions in this file (e.g.,
oc_send_buffer,oc_send_discovery_request), it's crucial to verify that all network operations continue to function correctly.Please ensure that:
- All network-related functions used in this file are still accessible and behave as expected.
- The change doesn't introduce any unintended side effects in the network communication logic.
Consider running comprehensive integration tests that cover:
- Inbound and outbound message handling
- Discovery requests
- Unicast and multicast communication
- Secure communication (if applicable)
This will help catch any potential issues introduced by the header change.
| @@ -22,7 +22,7 @@ | |||
| #include "messaging/coap/engine_internal.h" | |||
| #include "oc_signal_event_loop.h" | |||
| #include "oc_buffer.h" | |||
| #include "port/oc_connectivity.h" | |||
| #include "port/oc_connectivity_internal.h" | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent usage of connectivity headers detected.
The switch to oc_connectivity_internal.h in api/oc_message_buffer.c is not reflected across the codebase, as many other files still include the old oc_connectivity.h.
- Numerous files continue to include
oc_connectivity.h. - Ensure that this change is part of a comprehensive refactoring effort and update all dependent files accordingly to prevent potential build or runtime issues.
🔗 Analysis chain
Verify the impact of switching to the internal connectivity header.
The change from oc_connectivity.h to oc_connectivity_internal.h suggests a shift from a public API to an internal one. This could affect the visibility and accessibility of functions or structures used in this file.
Please ensure that:
- All functions and structures used from the connectivity header are still accessible.
- This change aligns with any ongoing refactoring efforts or API changes in the project.
Run the following script to check for any potential issues:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential issues after changing to internal header
# Test: Look for any connectivity-related function calls that might be affected
rg --type c "oc_(send|recv)_(buffer|message)" api/oc_message_buffer.c
# Test: Check if there are any other files still using the old header
rg --type c '#include.*oc_connectivity\.h'
# Test: Verify if the new internal header exists
fd oc_connectivity_internal.h
Length of output: 3521
| @@ -33,6 +33,7 @@ | |||
| #include "oc_pstat_internal.h" | |||
| #include "oc_store.h" | |||
| #include "oc_tls_internal.h" | |||
| #include "port/oc_connectivity_internal.h" | |||
There was a problem hiding this comment.
💡 Codebase verification
The included header port/oc_connectivity_internal.h is not used in security/oc_oscore_engine.c. Consider removing this include to eliminate unnecessary dependencies.
🔗 Analysis chain
LGTM. Verify the necessity of this include.
The addition of #include "port/oc_connectivity_internal.h" looks good. However, please ensure that this internal header is necessary and doesn't introduce unwanted coupling between modules.
To verify the usage of newly available functions or definitions, you can run:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of functions or definitions from oc_connectivity_internal.h
# Search for function calls or usage of definitions from oc_connectivity_internal.h
rg --type c --type cpp -e '\boc_connectivity_' -e '\bOC_CONNECTIVITY_' security/oc_oscore_engine.c
Length of output: 143
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
include/oc_signal_event_loop.h (2)
33-34: LGTM: OC_API macro added correctly.The addition of the
OC_APImacro to the_oc_signal_event_loopfunction declaration is correct and aligns with the PR objectives. This change will likely affect the function's visibility and linkage in other translation units.Consider renaming the function to remove the leading underscore, as identifiers beginning with an underscore followed by a capital letter or another underscore are reserved for use by the compiler and standard library. A suggested name could be
oc_signal_event_loop.
24-34: Summary: OC_API annotations successfully added.The changes in this file successfully add the OC_API annotations as per the PR objectives. The addition of the
oc_export.hinclude and theOC_APImacro to the function declaration will likely improve API visibility and standardization across the codebase. These changes appear to be part of a larger effort to refactor and improve the project's API.As you continue this refactoring effort, consider reviewing other header files for consistency in applying the OC_API macro to public functions. This will ensure a uniform API across the project.
api/oc_rep_internal.h (2)
86-93: LGTM! Consider adding a note about thread safety.The function declaration and its documentation look good. The return type and lack of parameters are consistent with the function's purpose of retrieving the start of the CBOR encoded buffer.
Consider adding a note about thread safety, especially if this function accesses shared state. For example:
* @return pointer to the start of the cbor encoded buffer * * @see oc_parse_rep + * @note This function is not thread-safe and should only be called from the main thread. */
95-103: LGTM! Consider clarifying ownership and adding const qualifier.The function declaration and its documentation are generally good. However, there are a few points that could be improved for clarity and safety:
- Clarify ownership in the documentation:
* Shrink the buffer pointer to length of encoded cbor data. * * @param[in] buf pointer to cbor encoded buffer * @return pointer to the start of the shrinked cbor encoded buffer + * @note The returned pointer may or may not be the same as the input pointer. + * The caller should not free the returned pointer separately. * * @see oc_parse_rep + * @note This function is not thread-safe and should only be called from the main thread. */
- Consider adding the
constqualifier to the input parameter:-uint8_t *oc_rep_shrink_encoder_buf(uint8_t *buf); +uint8_t *oc_rep_shrink_encoder_buf(const uint8_t *buf);This change would prevent accidental modification of the input buffer and make the function's intent clearer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- api/oc_mnt.c (1 hunks)
- api/oc_rep_internal.h (1 hunks)
- api/unittest/helperstest.cpp (1 hunks)
- include/oc_rep.h (12 hunks)
- include/oc_signal_event_loop.h (1 hunks)
- messaging/coap/observe.c (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/unittest/helperstest.cpp
🔇 Additional comments (4)
include/oc_signal_event_loop.h (1)
24-25: LGTM: Appropriate include added for OC_API macro.The addition of the
oc_export.hinclude is consistent with the use of theOC_APImacro later in the file. Good job on placing it in the correct location within the file structure.api/oc_mnt.c (1)
25-25: Approve the addition of the include directive, but clarification needed.The addition of
#include "api/oc_rep_internal.h"is logically placed and consistent with the AI-generated summary. However, I have a few points for clarification:
Could you please explain the necessity of including this header? The existing code doesn't seem to use any new functions from it, so it might be helpful to understand if this is preparation for future changes or resolves some implicit dependencies.
The PR title mentions adding OC_API annotations, but this change doesn't directly relate to that. Could you clarify how this include directive contributes to the PR's main objective?
To ensure this include is necessary and not redundant, let's check if oc_rep_internal.h is already indirectly included:
✅ Verification successful
Include directive addition verified.
The
#include "api/oc_rep_internal.h"is not included indirectly elsewhere inapi/oc_mnt.c, making the explicit addition necessary to ensure the required internal representations are available.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if oc_rep_internal.h is already included indirectly for header in $(grep -oP '#include "\K[^"]+' api/oc_mnt.c | grep -v 'api/oc_rep_internal.h'); do if grep -q '#include "api/oc_rep_internal.h"' "$header" 2>/dev/null; then echo "oc_rep_internal.h is already included in $header" exit 0 fi done echo "oc_rep_internal.h is not included indirectly"Length of output: 1775
messaging/coap/observe.c (1)
58-58: Verify the necessity of the new include statement.The addition of
#include "api/oc_rep_internal.h"suggests that functionality from this header is now required. However, there are no visible changes in the rest of the file that directly use this new include.Could you please verify if this include is necessary for any indirect usage or upcoming changes? If it's not being used, consider removing it to avoid potential confusion and maintain clean dependencies.
To check for any usage of symbols from this header, you can run the following command:
If this command doesn't return any results, it might indicate that the include is not necessary.
✅ Verification successful
The new include statement is necessary.
The addition of
#include "api/oc_rep_internal.h"is required as multiple functions prefixed withoc_rep_are utilized withinmessaging/coap/observe.c. This ensures that the necessary symbols and functionalities are correctly referenced.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for usage of symbols from oc_rep_internal.h grep -rn "oc_rep_" messaging/coap/observe.cLength of output: 1666
include/oc_rep.h (1)
132-132: Addition ofOC_APIandOC_NONNULL()Annotations Enhances API SafetyThe inclusion of
OC_APIandOC_NONNULL()annotations in the function declarations improves the clarity of the public API and enforces non-null constraints on pointers, enhancing code safety and usability.Also applies to: 141-141, 151-151, 161-161, 171-171, 184-184, 194-194, 206-206
| @@ -154,15 +129,16 @@ uint8_t *oc_rep_shrink_encoder_buf(uint8_t *buf); | |||
| * @param len Length of data. | |||
| */ | |||
| OC_API | |||
| void oc_rep_encode_raw(const uint8_t *data, size_t len); | |||
| void oc_rep_encode_raw(const uint8_t *data, size_t len) OC_NONNULL(); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure Consistent Specification of Non-Null Parameters in OC_NONNULL() Annotations
Currently, OC_NONNULL() is used without parameter indices in several function declarations (e.g., lines 132, 141, 151, 161, 171, 184, 194, 206, 231-234, 246-249, 263-266, 283, 288, 293, 298, 303, 314, 321), while parameter indices are specified in others (e.g., line 308). To improve clarity and maintain consistency, consider specifying the parameter indices in all OC_NONNULL() annotations to explicitly indicate which pointer parameters must not be null.
Also applies to: 141-141, 151-151, 161-161, 171-171, 184-184, 194-194, 206-206, 231-234, 246-249, 263-266, 283-283, 288-288, 293-293, 298-298, 303-303, 308-308, 314-314, 321-321
| CborError oc_rep_object_set_double(CborEncoder *object, const char *key, | ||
| size_t key_len, double value) OC_NONNULL(); | ||
|
|
||
| /** Add an string `value` to the cbor `object` under the `key` name */ | ||
| OC_API |
There was a problem hiding this comment.
Include All Non-Null Pointer Parameters in OC_NONNULL() Annotation
In the function oc_rep_object_set_text_string (line 308), the OC_NONNULL(1, 2) annotation specifies parameters 1 and 2 as non-null. However, parameter 4 (const char *value) is also a pointer that should not be null. Please update the annotation to OC_NONNULL(1, 2, 4) to accurately reflect all non-null pointer parameters.
| CborError oc_rep_object_set_text_string(CborEncoder *object, const char *key, | ||
| size_t key_len, const char *value, | ||
| size_t length) OC_NONNULL(1, 2); | ||
|
|
||
| /** Add an byte array `value` to the cbor `object` under the `key` name */ | ||
| OC_API |
There was a problem hiding this comment.
Specify Non-Null Parameters in OC_NONNULL() for oc_rep_object_set_byte_string
In oc_rep_object_set_byte_string (line 314), the OC_NONNULL() annotation does not specify parameter indices. Since parameters 1 (object), 2 (key), and 4 (value) are pointers that must not be null, consider updating the annotation to OC_NONNULL(1, 2, 4) for clarity and consistency.
| CborError oc_rep_object_set_byte_string(CborEncoder *object, const char *key, | ||
| size_t key_len, const uint8_t *value, | ||
| size_t length) OC_NONNULL(); | ||
|
|
||
| /** Add a string array using an oc_string_array_t as `values` to the cbor | ||
| * `object` under the `key` name. */ | ||
| OC_API |
There was a problem hiding this comment.
Update OC_NONNULL() Annotation in oc_rep_object_set_string_array
In oc_rep_object_set_string_array (line 321), the OC_NONNULL() annotation lacks parameter indices. Parameters 1 (CborEncoder *object), 2 (const char *key), and 4 (const oc_string_array_t *array) are pointers that should not be null. Please update the annotation to OC_NONNULL(1, 2, 4) to accurately specify the non-null parameters.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (11)
include/oc_ri.h (3)
Line range hint
637-652: LGTM: API updates, improved documentation, and deprecation notice.The changes include:
- Addition of OC_API annotations to
oc_ri_get_query_valueandoc_ri_query_exists_v1functions.- Updated comments using doxygen-style tags, improving documentation clarity.
- Addition of OC_NONNULL and OC_DEPRECATED macros to
oc_ri_get_query_value.- Introduction of
oc_ri_query_exists_v1, likely replacing the deprecatedoc_ri_query_exists.These changes improve the API's documentation and signal important updates to consumers. The deprecation notice is particularly important.
Consider adding a comment to
oc_ri_query_exists_v1directing users to use this function instead of the deprecatedoc_ri_query_exists. This will help guide API consumers towards the new function.
Line range hint
663-680: LGTM: API updates, improved documentation, and deprecation notice.The changes include:
- Addition of OC_API annotations to
oc_ri_query_existsandoc_ri_query_nth_key_existsfunctions.- Updated comments using doxygen-style tags, improving documentation clarity.
- Addition of OC_NONNULL and OC_DEPRECATED macros to
oc_ri_query_exists.These changes improve the API's documentation and signal important updates to consumers. The deprecation notice for
oc_ri_query_existsis particularly important.If
oc_ri_query_nth_key_existsis intended to replaceoc_ri_query_exists, consider adding a comment tooc_ri_query_nth_key_existsdirecting users to use this function instead of the deprecated one. This will help guide API consumers towards the new function.
Line range hint
1-689: Summary: Improved API documentation and consistencyOverall, the changes to
include/oc_ri.hsignificantly improve the file's quality and usability:
- Consistent comment style throughout the file, improving readability.
- Addition of OC_API annotations, clearly marking public API functions.
- Updated function documentation using doxygen-style tags, enhancing clarity.
- Proper marking of deprecated functions and introduction of their replacements.
- Addition of OC_NONNULL macros, improving function safety.
These changes will benefit users of the library by providing clearer documentation and API guidelines. The deprecation notices and new function introductions suggest an evolving API, which users should be aware of when updating to this version.
As the API evolves, consider providing a migration guide or updating the main documentation to help users transition from deprecated functions to their new counterparts. This will ensure a smoother upgrade process for consumers of the library.
api/oc_ri_server_internal.h (5)
90-95: Add missing documentation foroc_ri_handle_observationfunctionThe function
oc_ri_handle_observationlacks a detailed documentation comment block. For consistency and clarity, please add a documentation block that describes the purpose of the function, its parameters, and return value.
96-100: Expand documentation foroc_ri_notify_resource_observersfunctionThe documentation for
oc_ri_notify_resource_observersis minimal. Consider adding more detailed comments, including descriptions of the parameters and any important notes about the function's behavior.
101-103: Clarify the purpose ofoc_ri_server_initfunctionWhile the function
oc_ri_server_initis intended to initialize server variables, additional details about what specifically is being initialized would enhance readability and maintainability. Please consider expanding the documentation to include this information.
104-106: Clarify the functionality ofoc_ri_server_resetfunctionThe description "Reset observations" may not fully convey the scope of the function. Provide additional details in the documentation to explain what aspects are reset and any implications this may have on the server state.
107-109: Add documentation foroc_ri_server_shutdownfunctionThe function
oc_ri_server_shutdowncurrently lacks a documentation comment. To maintain consistency and aid future developers, please add a comment block describing its purpose and any important considerations when calling it.api/oc_ri_server.c (3)
Line range hint
263-274: Functionoc_ri_on_delete_resource_find_callbackused before its declarationIn the function
oc_ri_on_delete_resource_add_callback, the call tooc_ri_on_delete_resource_find_callback(cb)occurs before the functionoc_ri_on_delete_resource_find_callbackis declared or defined. This can lead to compilation errors or warnings about implicit function declarations.To fix this, ensure that the declaration or definition of
oc_ri_on_delete_resource_find_callbackprecedes its usage inoc_ri_on_delete_resource_add_callback. You can rearrange the code as follows:+// Move the definition of oc_ri_on_delete_resource_find_callback before its usage + oc_ri_on_delete_resource_t * oc_ri_on_delete_resource_find_callback(oc_ri_delete_resource_cb_t cb) { oc_ri_on_delete_resource_t *item = oc_list_head(g_on_delete_resource_cb_list); for (; item != NULL; item = item->next) { if (cb == item->cb) { return item; } } return NULL; } bool oc_ri_on_delete_resource_add_callback(oc_ri_delete_resource_cb_t cb) { if (oc_ri_on_delete_resource_find_callback(cb) != NULL) { OC_ERR("delete resource callback already exists"); return false; } // Rest of the function... }🧰 Tools
🪛 cppcheck
[error] 74-74: There is an unknown macro here somewhere. Configuration is required. If oc_ri_on_delete_resource_find_callback is a macro then please configure it.
(unknownMacro)
70-74: Unused functionoc_ri_alloc_resourceThe function
oc_ri_alloc_resourceis defined but not used within the provided code. While it's acceptable for utility functions to exist without immediate usage, ensure that this function is necessary and utilized appropriately in the codebase.If the function is intended for future use or is used elsewhere, you can disregard this comment. Otherwise, consider removing it to keep the code clean.
🧰 Tools
🪛 cppcheck
[error] 74-74: There is an unknown macro here somewhere. Configuration is required. If oc_ri_on_delete_resource_find_callback is a macro then please configure it.
(unknownMacro)
76-80: Unused functionoc_ri_dealloc_resourceSimilarly, the function
oc_ri_dealloc_resourceis defined but not used within the provided code. Verify if this function is required.If it's intended for memory management in other parts of the code, ensure it's being called where necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- api/oc_collection.c (1 hunks)
- api/oc_ri.c (5 hunks)
- api/oc_ri_internal.h (0 hunks)
- api/oc_ri_server.c (2 hunks)
- api/oc_ri_server_internal.h (2 hunks)
- include/oc_ri.h (16 hunks)
💤 Files with no reviewable changes (1)
- api/oc_ri_internal.h
🧰 Additional context used
🪛 cppcheck
api/oc_ri_server.c
[error] 74-74: There is an unknown macro here somewhere. Configuration is required. If oc_ri_on_delete_resource_find_callback is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (11)
include/oc_ri.h (6)
Line range hint
40-48: LGTM: Improved comment style consistency.The change from multi-line to single-line comments for the CoAP methods enumeration improves the overall consistency of the code. The content remains the same, ensuring no loss of information.
Line range hint
49-68: LGTM: Improved comment style and added informative note.The change to single-line comments for resource properties and response status enumerations improves code consistency. The added note about status code translation to HTTP or CoAP is informative and helpful for developers.
Line range hint
160-176: LGTM: Consistent comment style for interface masks.The change to single-line comments for the interface masks enumeration improves the overall consistency of the code. The content remains unchanged, maintaining the same level of information.
Line range hint
227-294: LGTM: Consistent comment style for structures and typedefs.The change to single-line comments for various structures and typedefs improves the overall consistency of the code. The content remains essentially the same, with some minor rewording that doesn't affect the meaning. This change enhances readability without altering functionality.
Line range hint
610-627: LGTM: Improved API annotations, documentation, and safety checks.The changes to
oc_ri_get_query_nth_key_valueandoc_ri_get_query_value_v1functions include:
- Addition of OC_API annotations, indicating they are part of the public API.
- Updated comments using doxygen-style @param and @return tags, improving documentation clarity.
- Addition of OC_NONNULL macro, likely enforcing that certain parameters cannot be NULL, which improves function safety.
These changes enhance the API's usability, documentation, and robustness without altering the core functionality.
Line range hint
507-516: LGTM: Added OC_API annotations to public functions.The addition of OC_API annotations to
oc_ri_get_app_resource_by_uriandoc_ri_get_app_resourcesfunctions indicates that these are part of the public API. This change may affect how these functions are exported or used by consumers of the library, but it doesn't alter the functionality of the code itself.To ensure this change doesn't introduce any inconsistencies, let's check if other similar functions in the file also have the OC_API annotation:
✅ Verification successful
Verified: OC_API annotations are consistently applied to public functions.
The addition of
OC_APIannotations tooc_ri_get_app_resource_by_uriandoc_ri_get_app_resourcesaligns with existing annotations in the file, ensuring these functions are correctly exposed as part of the public API.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other function declarations in the file grep -n "OC_API" include/oc_ri.hLength of output: 285
api/oc_collection.c (3)
28-28: LGTM: Include statement added for internal server functionality.The addition of
#include "api/oc_ri_server_internal.h"is appropriate as it likely provides access to internal server-related functions and definitions that may be needed for collection handling. This change appears to be consistent with the file's purpose and existing includes.
Line range hint
1-1009: Summary: Minor change with potential for future improvements.The addition of the include statement for "oc_ri_server_internal.h" is a minor change that doesn't introduce any immediate issues. The file's overall structure and functionality remain intact. However, this new include may provide opportunities for future code improvements or optimizations using the newly available internal server functions. Consider reviewing the usage of this header in the context of the entire file in a future refactoring task.
Line range hint
1-1009: Consider reviewing usage of newly included header.While the addition of the new include statement is appropriate, it may be beneficial to review the contents of "oc_ri_server_internal.h" and consider if any of its newly available functions or definitions could be used to improve or simplify the existing code in this file.
To ensure that the new include is being utilized effectively, you can run the following command to check for any usage of symbols from the newly included header:
✅ Verification successful
Verify Necessity of "oc_ri_server_internal.h" Inclusion
The inclusion of
"oc_ri_server_internal.h"inapi/oc_collection.cis currently utilized by only one function:oc_ri_URI_is_in_use. If this function is essential and there's no alternative in a more commonly included header, retaining the inclusion is justified. However, if possible, consider refactoring to minimize dependencies by relocating the function or using forward declarations where appropriate.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of symbols from oc_ri_server_internal.h # Get all function and type names from oc_ri_server_internal.h header_symbols=$(rg --no-filename '(typedef|struct|enum|union|#define|\w+\s+\w+\s*\()' api/oc_ri_server_internal.h | awk '{print $2}' | cut -d'(' -f1 | sort -u) # Check for usage of these symbols in the current file for symbol in $header_symbols; do rg "\b$symbol\b" api/oc_collection.c doneLength of output: 1750
api/oc_ri_server_internal.h (1)
44-45: Ensure consistent use ofOC_NONNULLmacroThe function
oc_ri_URI_is_in_useis annotated withOC_NONNULL(), indicating that certain parameters must not beNULL. Verify that all functions with non-nullable pointer parameters are consistently annotated withOC_NONNULL()to enforce this contract throughout the codebase.api/oc_ri.c (1)
1304-1307: Validate Parameters foroc_ri_handle_observation()The updated call to
oc_ri_handle_observation()in GET requests correctly handles observation options. However, ensure that all parameters passed to the function are valid and that any potential errors are handled properly.Run the following script to verify the correctness of parameters used in all calls to
oc_ri_handle_observation():This script searches for all occurrences of
oc_ri_handle_observation()in the codebase and displays surrounding lines for context. Review these instances to ensure parameters are correctly used.
|
|
||
| bool removed = oc_list_remove2(g_app_resources, resource) != NULL; | ||
| removed = | ||
| oc_list_remove2(g_app_resources_to_be_deleted, resource) != NULL || removed; | ||
|
|
There was a problem hiding this comment.
Potential memory leak in ri_delete_resource
In ri_delete_resource, after removing the resource from g_app_resources and g_app_resources_to_be_deleted, there is no check to ensure that the resource is properly deallocated if it was not in either list.
Ensure that resources are properly deallocated to prevent memory leaks. Consider adding a check or log statement for when a resource is not found in the lists.
bool removed = oc_list_remove2(g_app_resources, resource) != NULL;
removed = oc_list_remove2(g_app_resources_to_be_deleted, resource) != NULL || removed;
+if (!removed) {
+ OC_WARN("Resource (%p) was not found in the resource lists", (void *)resource);
+}Committable suggestion was skipped due to low confidence.
api/oc_ri_server.c
Outdated
| ri_add_observation(const coap_packet_t *request, const coap_packet_t *response, | ||
| oc_resource_t *resource, bool resource_is_collection, | ||
| uint16_t block2_size, const oc_endpoint_t *endpoint, | ||
| oc_interface_mask_t iface_query) | ||
| { | ||
| if (ri_observe_handler(request, response, resource, block2_size, endpoint, | ||
| iface_query) >= 0) { | ||
| /* If the resource is marked as periodic observable it means it must be | ||
| * polled internally for updates (which would lead to notifications being | ||
| * sent). If so, add the resource to a list of periodic GET callbacks to | ||
| * utilize the framework's internal polling mechanism. | ||
| */ | ||
| if ((resource->properties & OC_PERIODIC) != 0 && | ||
| !oc_periodic_observe_callback_add(resource)) { | ||
| return false; | ||
| } | ||
| } | ||
| #ifdef OC_COLLECTIONS | ||
| if (resource_is_collection) { | ||
| oc_collection_t *collection = (oc_collection_t *)resource; | ||
| if (!ri_add_collection_observation(collection, endpoint, | ||
| iface_query == OC_IF_B)) { | ||
| // TODO: shouldn't we remove the periodic observe callback here? | ||
| return false; | ||
| } | ||
| } | ||
| #else /* !OC_COLLECTIONS */ | ||
| (void)resource_is_collection; | ||
| #endif /* OC_COLLECTIONS */ | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Error handling in ri_add_observation
The function ri_add_observation returns true even if ri_observe_handler returns a negative value, which indicates an error.
Modify the function to correctly handle the return value of ri_observe_handler to ensure that errors are propagated properly.
if (ri_observe_handler(request, response, resource, block2_size, endpoint,
iface_query) >= 0) {
// Existing code...
} else {
+ return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ri_add_observation(const coap_packet_t *request, const coap_packet_t *response, | |
| oc_resource_t *resource, bool resource_is_collection, | |
| uint16_t block2_size, const oc_endpoint_t *endpoint, | |
| oc_interface_mask_t iface_query) | |
| { | |
| if (ri_observe_handler(request, response, resource, block2_size, endpoint, | |
| iface_query) >= 0) { | |
| /* If the resource is marked as periodic observable it means it must be | |
| * polled internally for updates (which would lead to notifications being | |
| * sent). If so, add the resource to a list of periodic GET callbacks to | |
| * utilize the framework's internal polling mechanism. | |
| */ | |
| if ((resource->properties & OC_PERIODIC) != 0 && | |
| !oc_periodic_observe_callback_add(resource)) { | |
| return false; | |
| } | |
| } | |
| #ifdef OC_COLLECTIONS | |
| if (resource_is_collection) { | |
| oc_collection_t *collection = (oc_collection_t *)resource; | |
| if (!ri_add_collection_observation(collection, endpoint, | |
| iface_query == OC_IF_B)) { | |
| // TODO: shouldn't we remove the periodic observe callback here? | |
| return false; | |
| } | |
| } | |
| #else /* !OC_COLLECTIONS */ | |
| (void)resource_is_collection; | |
| #endif /* OC_COLLECTIONS */ | |
| return true; | |
| } | |
| ri_add_observation(const coap_packet_t *request, const coap_packet_t *response, | |
| oc_resource_t *resource, bool resource_is_collection, | |
| uint16_t block2_size, const oc_endpoint_t *endpoint, | |
| oc_interface_mask_t iface_query) | |
| { | |
| if (ri_observe_handler(request, response, resource, block2_size, endpoint, | |
| iface_query) >= 0) { | |
| /* If the resource is marked as periodic observable it means it must be | |
| * polled internally for updates (which would lead to notifications being | |
| * sent). If so, add the resource to a list of periodic GET callbacks to | |
| * utilize the framework's internal polling mechanism. | |
| */ | |
| if ((resource->properties & OC_PERIODIC) != 0 && | |
| !oc_periodic_observe_callback_add(resource)) { | |
| return false; | |
| } | |
| } else { | |
| return false; | |
| } | |
| #ifdef OC_COLLECTIONS | |
| if (resource_is_collection) { | |
| oc_collection_t *collection = (oc_collection_t *)resource; | |
| if (!ri_add_collection_observation(collection, endpoint, | |
| iface_query == OC_IF_B)) { | |
| // TODO: shouldn't we remove the periodic observe callback here? | |
| return false; | |
| } | |
| } | |
| #else /* !OC_COLLECTIONS */ | |
| (void)resource_is_collection; | |
| #endif /* OC_COLLECTIONS */ | |
| return true; | |
| } |
| if (removed) { | ||
| oc_notify_resource_removed(resource); | ||
| } else { | ||
| #if defined(OC_COLLECTIONS) && defined(OC_RES_BATCH_SUPPORT) && \ | ||
| defined(OC_DISCOVERY_RESOURCE_OBSERVABLE) | ||
| // if oc_notify_resource_removed is not called, then we need to dispatch | ||
| // manually if it is requested | ||
| if (needsBatchDispatch) { | ||
| coap_dispatch_process_batch_observers(); | ||
| } | ||
| #endif /* OC_COLLECTIONS && OC_RES_BATCH_SUPPORT && \ | ||
| OC_DISCOVERY_RESOURCE_OBSERVABLE */ | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Conditional Compilation Issue Detected
The macro OC_DISCOVERY_RESOURCE_OBSERVABLE is not consistently defined across all configuration files. This inconsistency can lead to the conditional compilation block being excluded in certain builds, potentially causing unexpected behavior.
- Affected Files:
port/android/oc_config.hport/arduino/adapter/oc_config.hport/linux/oc_config.hport/openthread/oc_config.hport/windows/oc_config.hport/zephyr/src/oc_config.h
🔗 Analysis chain
Conditional compilation block may cause unexpected behavior
In the if (notify) block, the #if defined conditions may lead to code being excluded unintentionally if the macros are not properly defined.
Please verify that all macros OC_COLLECTIONS, OC_RES_BATCH_SUPPORT, and OC_DISCOVERY_RESOURCE_OBSERVABLE are defined as expected to ensure that the code within the conditional compilation blocks executes correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the macros are defined in the build configuration.
# Check for macro definitions in the source files
rg --type c --define-macro "OC_COLLECTIONS" --define-macro "OC_RES_BATCH_SUPPORT" --define-macro "OC_DISCOVERY_RESOURCE_OBSERVABLE" "api/"
# Alternatively, check the build configuration files
grep -E "OC_COLLECTIONS|OC_RES_BATCH_SUPPORT|OC_DISCOVERY_RESOURCE_OBSERVABLE" build_config.h
Length of output: 693
Script:
#!/bin/bash
# Description: Correctly verify that the macros are defined in the codebase.
# Search for macro definitions in the source files recursively
grep -r -w '#define OC_COLLECTIONS' api/ || echo "OC_COLLECTIONS not defined in api/"
grep -r -w '#define OC_RES_BATCH_SUPPORT' api/ || echo "OC_RES_BATCH_SUPPORT not defined in api/"
grep -r -w '#define OC_DISCOVERY_RESOURCE_OBSERVABLE' api/ || echo "OC_DISCOVERY_RESOURCE_OBSERVABLE not defined in api/"
# Additionally, search all source files if not found in api/
grep -r -w '#define OC_COLLECTIONS' . || echo "OC_COLLECTIONS not defined in the codebase."
grep -r -w '#define OC_RES_BATCH_SUPPORT' . || echo "OC_RES_BATCH_SUPPORT not defined in the codebase."
grep -r -w '#define OC_DISCOVERY_RESOURCE_OBSERVABLE' . || echo "OC_DISCOVERY_RESOURCE_OBSERVABLE not defined in the codebase."
Length of output: 1549
| #endif /* OC_COLLECTIONS */ | ||
|
|
||
| ri_delete_all_app_resources(); | ||
| oc_ri_server_shutdown(); |
There was a problem hiding this comment.
Add Error Handling for oc_ri_server_shutdown()
Including oc_ri_server_shutdown(); in oc_ri_shutdown() ensures proper cleanup of server-specific components. It's recommended to check the return value to handle any potential errors during the shutdown process.
Apply this diff to add error handling:
oc_process_shutdown();
#ifdef OC_SERVER
- oc_ri_server_shutdown();
+ if (oc_ri_server_shutdown() != 0) {
+ OC_ERR("Server shutdown failed");
+ }
#endif /* OC_SERVER */
}Committable suggestion was skipped due to low confidence.
| /* If the recently handled request was a PUT/POST, it conceivably | ||
| * altered the resource state, so attempt to notify all observers | ||
| * of that resource with the change. */ | ||
| oc_ri_notify_resource_observers(ctx.resource, ctx.iface_mask); |
There was a problem hiding this comment.
Check Resource State Before Notifying Observers
The call to oc_ri_notify_resource_observers(ctx.resource, ctx.iface_mask); after a successful PUT/POST request assumes that the resource state has changed. To prevent unnecessary notifications, consider verifying whether the resource state has indeed changed before notifying observers.
Apply this diff to check if the resource has actually changed:
/* If the recently handled request was a PUT/POST, it conceivably
* altered the resource state, so attempt to notify all observers
* of that resource with the change. */
- oc_ri_notify_resource_observers(ctx.resource, ctx.iface_mask);
+ if (resource_state_changed) {
+ oc_ri_notify_resource_observers(ctx.resource, ctx.iface_mask);
+ }Ensure that resource_state_changed is set to true only when the resource state is modified.
Committable suggestion was skipped due to low confidence.
Danielius1922
force-pushed
the
adam/feature/182-export-public-api
branch
from
86e4940
to
72c27a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
api/oc_ri_server.c (7)
93-130: LGTM: Comprehensive resource addition functionThe
oc_ri_add_resourcefunction is well-implemented with thorough error checking and validation. It ensures that resources are not duplicated and have valid handlers.Consider grouping related checks together for better readability. For example, move the URI check closer to the other resource property checks:
if ((resource->properties & OC_PERIODIC) != 0 && resource->observe_period_seconds == 0) { OC_ERR("resource(%s) has invalid observe period", oc_string(resource->uri)); return false; } +if (oc_ri_URI_is_in_use(resource->device, oc_string(resource->uri), + oc_string_len(resource->uri))) { + OC_ERR("resource(%s) URI is already in use", oc_string(resource->uri)); + return false; +} if (oc_ri_is_app_resource_valid(resource)) { OC_ERR("resource(%s) already exists in IoTivity stack", oc_string(resource->uri)); return false; } if (oc_ri_is_app_resource_to_be_deleted(resource)) { OC_ERR("resource(%s) is scheduled to be deleted", oc_string(resource->uri)); return false; } -if (oc_ri_URI_is_in_use(resource->device, oc_string(resource->uri), - oc_string_len(resource->uri))) { - OC_ERR("resource(%s) URI is already in use", oc_string(resource->uri)); - return false; -}
132-166: LGTM: Efficient resource retrieval functionsThe
oc_ri_get_app_resourcesandoc_ri_get_app_resource_by_urifunctions are well-implemented and provide necessary functionality for resource management.Consider optimizing
oc_ri_get_app_resource_by_urifor cases where the URI starts with a slash:oc_resource_t * oc_ri_get_app_resource_by_uri(const char *uri, size_t uri_len, size_t device) { if (!uri || uri_len == 0) { return NULL; } - int skip = 0; - if (uri[0] != '/') { - skip = 1; - } + const char *compare_uri = uri; + size_t compare_len = uri_len; + if (uri[0] == '/') { + compare_uri++; + compare_len--; + } oc_resource_t *res = oc_ri_get_app_resources(); while (res != NULL) { - if (oc_string_len(res->uri) == (uri_len + skip) && - strncmp(uri, oc_string(res->uri) + skip, uri_len) == 0 && + if (oc_string_len(res->uri) - 1 == compare_len && + strncmp(compare_uri, oc_string(res->uri) + 1, compare_len) == 0 && res->device == device) { return res; } res = res->next; } // ... rest of the function }This change eliminates the need for the
skipvariable and simplifies the comparison logic.
168-231: LGTM: Comprehensive resource and URI validation functionsThe new functions for resource validation and URI checking are well-implemented and cover all necessary cases, including core resources, dynamic resources, and collections.
Consider optimizing the URI comparison in
ri_uri_is_in_listto avoid repeated string manipulations:static bool ri_uri_is_in_list(oc_list_t list, const char *uri, size_t uri_len, size_t device) { + const char *compare_uri = uri; + size_t compare_len = uri_len; while (uri[0] == '/') { - uri++; - uri_len--; + compare_uri++; + compare_len--; } const oc_resource_t *res = oc_list_head(list); for (; res != NULL; res = res->next) { - if (res->device == device && oc_string_len(res->uri) == (uri_len + 1) && - strncmp(oc_string(res->uri) + 1, uri, uri_len) == 0) { + if (res->device == device && oc_string_len(res->uri) == (compare_len + 1) && + strncmp(oc_string(res->uri) + 1, compare_uri, compare_len) == 0) { return true; } } return false; }This change reduces the number of string manipulations performed in the loop, potentially improving performance for long lists of resources.
Line range hint
233-415: LGTM: Comprehensive resource deletion functionalityThe resource deletion functions, including the delayed deletion mechanism, are well-implemented and cover all necessary aspects of resource cleanup.
Consider adding error handling in the
oc_delayed_delete_resource_cbfunction:static oc_event_callback_retval_t oc_delayed_delete_resource_cb(void *data) { oc_resource_t *resource = (oc_resource_t *)data; OC_DBG("delayed delete resource(%p)", (void *)resource); oc_ri_on_delete_resource_invoke(resource); - oc_delete_resource(resource); + if (!oc_delete_resource(resource)) { + OC_ERR("Failed to delete resource(%p) in delayed callback", (void *)resource); + // Consider if any recovery action is needed here + } return OC_EVENT_DONE; }This change adds error checking for the
oc_delete_resourcecall, which could fail if the resource has already been deleted or if there's an issue with the deletion process.
527-662: LGTM: Enhanced observation handlingThe modifications to the observation handling functions improve support for collections and periodic observables. The code structure is clear and logical.
Consider enhancing error handling in the
ri_add_observationfunction:static bool ri_add_observation(const coap_packet_t *request, const coap_packet_t *response, oc_resource_t *resource, bool resource_is_collection, uint16_t block2_size, const oc_endpoint_t *endpoint, oc_interface_mask_t iface_query) { - if (ri_observe_handler(request, response, resource, block2_size, endpoint, - iface_query) >= 0) { + int observe_result = ri_observe_handler(request, response, resource, block2_size, endpoint, + iface_query); + if (observe_result > 0) { + // Observation was unregistered + return true; + } else if (observe_result == 0) { /* If the resource is marked as periodic observable it means it must be * polled internally for updates (which would lead to notifications being * sent). If so, add the resource to a list of periodic GET callbacks to * utilize the framework's internal polling mechanism. */ if ((resource->properties & OC_PERIODIC) != 0 && !oc_periodic_observe_callback_add(resource)) { + OC_ERR("Failed to add periodic observe callback for resource(%p)", (void *)resource); return false; } + } else { + OC_ERR("Failed to handle observation for resource(%p)", (void *)resource); + return false; } #ifdef OC_COLLECTIONS if (resource_is_collection) { oc_collection_t *collection = (oc_collection_t *)resource; if (!ri_add_collection_observation(collection, endpoint, iface_query == OC_IF_B)) { - // TODO: shouldn't we remove the periodic observe callback here? + OC_ERR("Failed to add collection observation for resource(%p)", (void *)resource); + if ((resource->properties & OC_PERIODIC) != 0) { + oc_periodic_observe_callback_remove(resource); + } return false; } } #else /* !OC_COLLECTIONS */ (void)resource_is_collection; #endif /* OC_COLLECTIONS */ return true; }These changes provide more detailed error handling and logging, and address the TODO comment about removing the periodic observe callback when adding a collection observation fails.
664-693: LGTM: ETag functionality for resources and batch operationsThe new ETag-related functions provide a good mechanism for handling ETags in different scenarios, including discovery and collections.
Consider modifying the default case in
oc_ri_get_batch_etagto provide more information:uint64_t oc_ri_get_batch_etag(const oc_resource_t *resource, const oc_endpoint_t *endpoint, size_t device) { #ifdef OC_RES_BATCH_SUPPORT if (oc_core_get_resource_by_index(OCF_RES, device) == resource) { return oc_discovery_get_batch_etag(endpoint, device); } #endif /* OC_RES_BATCH_SUPPORT */ #ifdef OC_COLLECTIONS if (oc_check_if_collection(resource)) { return oc_collection_get_batch_etag((const oc_collection_t *)resource); } #endif /* OC_COLLECTIONS */ (void)resource; (void)endpoint; (void)device; - OC_DBG("custom batch etag"); + OC_DBG("Unhandled case in oc_ri_get_batch_etag for resource type: %d", resource->types); return OC_ETAG_UNINITIALIZED; }This change provides more context in the debug log when an unhandled case is encountered, which could be helpful for debugging and future extensions of the function.
Line range hint
1-694: Final Review: Significant improvements with some areas for refinementThe changes to
api/oc_ri_server.csignificantly enhance the resource management capabilities of the IoT framework. Key improvements include:
- More robust resource allocation and deallocation.
- Enhanced observation handling, especially for collections and periodic observables.
- Addition of ETag support for resources and batch operations.
Overall, the code quality is good, with clear structure and logical organization. However, there are several areas where the code could be further improved:
- Error handling: While present, error handling could be more consistent and comprehensive throughout the file.
- Memory management: Consider adding checks for allocation failures in resource creation functions.
- Modularity: The file covers multiple concerns; consider splitting it into more focused modules.
- Documentation: Add more detailed function documentation, especially for complex functions.
- TODOs: Address or convert existing TODOs into tracked issues for future improvements.
To further improve the codebase:
- Implement a consistent error handling strategy, possibly using error code enums.
- Consider creating a resource context structure to encapsulate global state.
- Review the use of conditional compilation to ensure it doesn't overly complicate the code.
- Implement unit tests for the new and modified functions to ensure reliability.
These changes will enhance the maintainability, reliability, and performance of the resource management system.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- api/cloud/unittest/cloud_store_test.cpp (1 hunks)
- api/oc_collection.c (1 hunks)
- api/oc_ri.c (5 hunks)
- api/oc_ri_internal.h (0 hunks)
- api/oc_ri_server.c (2 hunks)
- api/oc_ri_server_internal.h (2 hunks)
- include/oc_ri.h (16 hunks)
💤 Files with no reviewable changes (1)
- api/oc_ri_internal.h
🚧 Files skipped from review as they are similar to previous changes (3)
- api/oc_collection.c
- api/oc_ri_server_internal.h
- include/oc_ri.h
🧰 Additional context used
🪛 cppcheck
api/oc_ri_server.c
[error] 74-74: There is an unknown macro here somewhere. Configuration is required. If oc_ri_on_delete_resource_find_callback is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (7)
api/cloud/unittest/cloud_store_test.cpp (1)
25-25: LGTM. Verify usage of new include.The addition of
#include "api/oc_rep_internal.h"looks good. However, please ensure that this header is actually used within this test file. If it's not directly used, consider removing it to avoid unnecessary includes.To verify the usage of the newly added include, you can run the following command:
If this command returns results, it indicates that the include is being used. If not, you may want to reconsider its inclusion.
✅ Verification successful
Verified inclusion of
api/oc_rep_internal.h.The include is actively used in
cloud_store_test.cpp, ensuring its necessity.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if oc_rep_internal.h is used in the file # Test: Search for usage of functions or types from oc_rep_internal.h rg --type cpp -e 'oc_rep_' -e 'OC_REP_' api/cloud/unittest/cloud_store_test.cppLength of output: 2786
api/oc_ri_server.c (2)
20-56: LGTM: New includes and structure definitionThe new includes and the
oc_resource_defaults_data_tstructure definition are well-organized and appropriate for the added functionality.
Line range hint
1-694: Overall structure and documentation suggestionsThe file structure is generally well-organized, grouping related functions together and using conditional compilation for flexibility. However, there are a few areas that could be improved:
Consider adding more detailed function documentation, especially for complex functions like
ri_delete_resourceandri_add_observation.There are some TODOs left in the code (e.g., line 516-518). It would be beneficial to address these or convert them into tracked issues if they represent longer-term improvements.
The file mixes different levels of abstraction (e.g., low-level memory management with high-level resource handling). Consider if some of these could be separated into different files for better modularity.
To improve the overall architecture and maintainability of the code:
Consider creating separate files for different concerns, such as:
- Resource allocation and deallocation
- Observation handling
- ETag management
Implement a more robust error handling and logging strategy throughout the file. This could include creating error code enums and using them consistently across functions.
Review the use of global variables (e.g.,
g_app_resources) and consider if some of these could be encapsulated in a context structure for better state management.To ensure consistent error handling and logging, run the following script:
This script will help identify areas where error handling and logging can be improved for consistency.
api/oc_ri.c (4)
1304-1304: LGTM: Improved observation handlingThe modification to
oc_ri_handle_observationcall with additional parameters enhances the context provided for observation handling. This change appears to be consistent with the updated function signature and improves the overall functionality.
327-327: 🛠️ Refactor suggestionConsider adding error handling for
oc_ri_server_init()The addition of
oc_ri_server_init();is good for initializing server-specific components. However, it's recommended to check the return value and handle any potential errors during the initialization process.Consider applying this diff to add error handling:
#ifdef OC_SERVER - oc_ri_server_init(); + if (oc_ri_server_init() != 0) { + OC_ERR("Server initialization failed"); + return; + } #endif /* OC_SERVER */Likely invalid or redundant comment.
1386-1386: 🛠️ Refactor suggestionConsider adding error handling for
oc_ri_server_shutdown()The addition of
oc_ri_server_shutdown();ensures proper cleanup of server-specific components. However, it's recommended to check the return value and handle any potential errors during the shutdown process.Consider applying this diff to add error handling:
#ifdef OC_SERVER - oc_ri_server_shutdown(); + if (oc_ri_server_shutdown() != 0) { + OC_ERR("Server shutdown failed"); + } #endif /* OC_SERVER */Likely invalid or redundant comment.
630-633: 🛠️ Refactor suggestionConsider checking for state change before notifying observers
The added comment explains the rationale for notifying observers after a PUT/POST request. However, notifying observers for every such request might be inefficient if the resource state hasn't actually changed.
Consider applying this diff to check if the resource has actually changed before notifying observers:
/* If the recently handled request was a PUT/POST, it conceivably * altered the resource state, so attempt to notify all observers * of that resource with the change. */ - oc_ri_notify_resource_observers(ctx.resource, ctx.iface_mask); + if (resource_state_changed) { + oc_ri_notify_resource_observers(ctx.resource, ctx.iface_mask); + }Ensure that
resource_state_changedis set totrueonly when the resource state is modified.Likely invalid or redundant comment.
| oc_resource_t * | ||
| oc_ri_alloc_resource(void) | ||
| { | ||
| return oc_memb_alloc(&g_app_resources_s); | ||
| } | ||
|
|
||
| void | ||
| oc_ri_dealloc_resource(oc_resource_t *resource) | ||
| { | ||
| oc_memb_free(&g_app_resources_s, resource); | ||
| } | ||
|
|
||
| static oc_resource_defaults_data_t * | ||
| oc_ri_alloc_resource_defaults(void) | ||
| { | ||
| return oc_memb_alloc(&g_resource_default_s); | ||
| } | ||
|
|
||
| static void | ||
| oc_ri_dealloc_resource_defaults(oc_resource_defaults_data_t *data) | ||
| { | ||
| oc_memb_free(&g_resource_default_s, data); | ||
| } |
There was a problem hiding this comment.
Consider adding error handling for allocation failures
The new allocation functions (oc_ri_alloc_resource, oc_ri_alloc_resource_defaults) don't check for allocation failures. In resource-constrained environments, it's crucial to handle these cases.
Consider modifying these functions to check for NULL returns and handle the error appropriately. For example:
oc_resource_t *
oc_ri_alloc_resource(void)
{
- return oc_memb_alloc(&g_app_resources_s);
+ oc_resource_t *resource = oc_memb_alloc(&g_app_resources_s);
+ if (resource == NULL) {
+ OC_ERR("Failed to allocate resource");
+ }
+ return resource;
}Apply similar changes to oc_ri_alloc_resource_defaults.
Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 cppcheck
[error] 74-74: There is an unknown macro here somewhere. Configuration is required. If oc_ri_on_delete_resource_find_callback is a macro then please configure it.
(unknownMacro)
| coap_free_all_discovery_batch_observers(); | ||
| #endif /* OC_RES_BATCH_SUPPORT && OC_DISCOVERY_RESOURCE_OBSERVABLE */ | ||
| coap_free_all_observers(); | ||
| oc_ri_server_reset(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for oc_ri_server_reset()
The addition of oc_ri_server_reset(); is appropriate for resetting server-specific components. However, it's recommended to check the return value and handle any potential errors during the reset process.
Consider applying this diff to add error handling:
#ifdef OC_SERVER
- oc_ri_server_reset();
+ if (oc_ri_server_reset() != 0) {
+ OC_ERR("Server reset failed");
+ }
#endif /* OC_SERVER */📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| oc_ri_server_reset(); | |
| #ifdef OC_SERVER | |
| if (oc_ri_server_reset() != 0) { | |
| OC_ERR("Server reset failed"); | |
| } | |
| #endif /* OC_SERVER */ |
|
Danielius1922
force-pushed
the
adam/feature/182-export-public-api
branch
from
72c27a8
to
66ff75f
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (8)
api/oc_ri_server.c (4)
69-91: Consider adding error handling for allocation failuresThe new allocation functions (
oc_ri_alloc_resource,oc_ri_alloc_resource_defaults) don't check for allocation failures. In resource-constrained environments, it's crucial to handle these cases.Consider modifying these functions to check for NULL returns and handle the error appropriately. For example:
oc_resource_t * oc_ri_alloc_resource(void) { - return oc_memb_alloc(&g_app_resources_s); + oc_resource_t *resource = oc_memb_alloc(&g_app_resources_s); + if (resource == NULL) { + OC_ERR("Failed to allocate resource"); + } + return resource; }Apply similar changes to
oc_ri_alloc_resource_defaults.🧰 Tools
🪛 cppcheck
[error] 74-74: There is an unknown macro here somewhere. Configuration is required. If oc_ri_on_delete_resource_find_callback is a macro then please configure it.
(unknownMacro)
93-130: Consider enhancing error reporting in oc_ri_add_resourceThe
oc_ri_add_resourcefunction includes comprehensive checks, which is excellent. To further improve its usability, consider enhancing the error reporting by returning specific error codes or using a more detailed logging mechanism.For example:
bool oc_ri_add_resource(oc_resource_t *resource) { if (resource == NULL) { + OC_ERR("Cannot add NULL resource"); return false; } // ... (existing checks) if (oc_ri_URI_is_in_use(resource->device, oc_string(resource->uri), oc_string_len(resource->uri))) { - OC_ERR("resource(%s) URI is already in use", oc_string(resource->uri)); + OC_ERR("Resource URI '%s' is already in use for device %zu", + oc_string(resource->uri), resource->device); return false; } // ... (rest of the function) }This approach provides more context in the error messages, making debugging easier.
527-555: Enhance error handling and logging in ri_observe_handlerThe
ri_observe_handlerfunction could benefit from more detailed error handling and logging. Consider the following improvements:
- Add more specific error codes or messages for different failure scenarios.
- Include logging for successful operations to aid in debugging and monitoring.
Here's an example of how you might enhance this function:
static int ri_observe_handler(const coap_packet_t *request, const coap_packet_t *response, oc_resource_t *resource, uint16_t block2_size, const oc_endpoint_t *endpoint, oc_interface_mask_t iface_mask) { if (request->code != COAP_GET || response->code >= 128 || !IS_OPTION(request, COAP_OPTION_OBSERVE)) { + OC_DBG("Invalid observe request: code=%d, response_code=%d, has_observe_option=%d", + request->code, response->code, IS_OPTION(request, COAP_OPTION_OBSERVE)); return -1; } if (request->observe == OC_COAP_OPTION_OBSERVE_REGISTER) { if (NULL == coap_add_observer(resource, block2_size, endpoint, request->token, request->token_len, request->uri_path, request->uri_path_len, iface_mask)) { - OC_ERR("failed to add observer"); + OC_ERR("Failed to add observer for resource %s", oc_string(resource->uri)); return -1; } + OC_DBG("Successfully added observer for resource %s", oc_string(resource->uri)); return 0; } if (request->observe == OC_COAP_OPTION_OBSERVE_UNREGISTER) { if (!coap_remove_observer_by_token(endpoint, request->token, request->token_len)) { + OC_DBG("Observer not found for unregister request on resource %s", oc_string(resource->uri)); return 0; } + OC_DBG("Successfully removed observer for resource %s", oc_string(resource->uri)); return 1; } + OC_DBG("Invalid observe option: %d", request->observe); return -1; }These changes provide more context in logs, making it easier to track and debug observation-related issues.
663-692: Enhance consistency and error handling in ETag functionsThe ETag-related functions are a good addition, but consider the following improvements:
- Add input validation for the
oc_ri_get_batch_etagfunction.- Use consistent naming conventions for the ETag-related macros and functions.
- Consider adding more detailed comments explaining the behavior of
oc_ri_get_batch_etagfor different resource types.Here's an example of how you might enhance these functions:
#ifdef OC_HAS_FEATURE_ETAG uint64_t oc_ri_get_etag(const oc_resource_t *resource) { if (resource == NULL) { OC_ERR("Attempted to get ETag for NULL resource"); return OC_ETAG_UNINITIALIZED; } return resource->etag; } uint64_t oc_ri_get_batch_etag(const oc_resource_t *resource, const oc_endpoint_t *endpoint, size_t device) { if (resource == NULL || endpoint == NULL) { OC_ERR("Invalid input to oc_ri_get_batch_etag"); return OC_ETAG_UNINITIALIZED; } // Handle discovery resource #ifdef OC_RES_BATCH_SUPPORT if (oc_core_get_resource_by_index(OCF_RES, device) == resource) { return oc_discovery_get_batch_etag(endpoint, device); } #endif /* OC_RES_BATCH_SUPPORT */ // Handle collections #ifdef OC_COLLECTIONS if (oc_check_if_collection(resource)) { return oc_collection_get_batch_etag((const oc_collection_t *)resource); } #endif /* OC_COLLECTIONS */ // Handle other resource types OC_DBG("No specific batch ETag handling for resource type, using default"); return OC_ETAG_UNINITIALIZED; } #endif /* OC_HAS_FEATURE_ETAG */These changes improve error handling, add input validation, and provide more context through comments and debug logging.
api/oc_ri.c (2)
1304-1304: Approve addition of observation handling with suggestion for error handlingThe addition of
oc_ri_handle_observationis appropriate for managing observation requests on GET methods. This enhances the functionality of the resource interface.Consider adding error handling for the
oc_ri_handle_observationcall:- observe = oc_ri_handle_observation( + int result = oc_ri_handle_observation( ctx->request, ctx->response, cur_resource, get_resource_is_collection(ctx->preparsed_request_obj), block2_size, endpoint, ctx->preparsed_request_obj->iface_query); + if (result < 0) { + OC_ERR("Failed to handle observation request"); + // Consider appropriate error handling + } else { + observe = result; + }This change will help catch and handle any errors that might occur during the observation process.
1360-1360: Approve server reset and shutdown additions with suggestion for error handlingThe additions of
oc_ri_server_reset();andoc_ri_server_shutdown();are appropriate for proper cleanup and shutdown of server components. They are correctly placed within theOC_SERVERblocks.Consider adding error handling for both function calls:
#ifdef OC_SERVER - oc_ri_server_reset(); + if (oc_ri_server_reset() != 0) { + OC_ERR("Failed to reset OCF Resource Interface server"); + // Consider appropriate error handling + } #endif /* OC_SERVER */ // ... (in oc_ri_shutdown function) #ifdef OC_SERVER - oc_ri_server_shutdown(); + if (oc_ri_server_shutdown() != 0) { + OC_ERR("Failed to shutdown OCF Resource Interface server"); + // Consider appropriate error handling + } #endif /* OC_SERVER */These changes will help detect and handle any errors that might occur during the reset and shutdown processes, improving the robustness of the server management.
Also applies to: 1386-1386
api/oc_discovery.c (2)
Line range hint
1244-1278: Improve robustness of 'rt' query parameter handling and response construction.There are a few areas in this function that could be improved:
The handling of the 'rt' query parameter could be more robust. Currently, it only checks for exact matches with "oic.wk.res" or the device type. It might be beneficial to support partial matches or multiple 'rt' values.
The construction of the response string is done by directly adding to a buffer. This approach is prone to buffer overflow issues if the constructed string exceeds the buffer size. Consider using a safer string construction method, such as snprintf, or dynamically allocating the buffer based on the required size.
The function assumes that there will always be a secured endpoint available. It might be worth adding a check to handle cases where no secured endpoint is found.
Consider refactoring the response construction to use a safer method:
char response_buffer[MAX_RESPONSE_LENGTH]; int response_length = snprintf(response_buffer, sizeof(response_buffer), "<coap://%s/oic/res>;rt=\"oic.wk.res %s\";if=\"oic.if.ll oic.if.baseline\";ct=10000", endpoint_string, rt_device ? rt_device : ""); if (response_length >= sizeof(response_buffer)) { // Handle buffer overflow return OC_STATUS_INTERNAL_SERVER_ERROR; }Also, consider adding a check for the availability of a secured endpoint:
if (eps == NULL) { OC_ERR("No secured endpoint available for discovery response"); return OC_STATUS_INTERNAL_SERVER_ERROR; }
Line range hint
1-1278: Overall, the code is well-structured but could benefit from some improvements.The
api/oc_discovery.cfile implements discovery-related functionality for the OCF stack. While the code is generally well-organized and follows consistent practices, there are a few areas where improvements could be made:
Error Handling: Some functions could benefit from more robust error handling, particularly when dealing with memory allocation or buffer operations.
Buffer Management: There are instances where buffer operations could potentially lead to overflows. Consider using safer string handling functions or implementing bounds checking more consistently.
Code Complexity: The extensive use of preprocessor directives for conditional compilation, while providing flexibility, also increases the code's complexity. Consider if some of this complexity could be reduced through alternative design patterns.
Documentation: While the code is generally well-commented, some complex sections could benefit from additional inline documentation explaining the rationale behind certain decisions.
Testing: Given the complexity of the discovery process, it would be beneficial to ensure comprehensive unit tests are in place for this file.
Consider refactoring some of the larger functions (e.g.,
oc_wkcore_discovery_handler) into smaller, more focused functions to improve readability and maintainability. This could also help in reducing the nesting level of conditional statements in some areas.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- api/cloud/unittest/cloud_store_test.cpp (1 hunks)
- api/oc_collection.c (1 hunks)
- api/oc_discovery.c (2 hunks)
- api/oc_ri.c (5 hunks)
- api/oc_ri_internal.h (0 hunks)
- api/oc_ri_server.c (2 hunks)
- api/oc_ri_server_internal.h (2 hunks)
- include/oc_ri.h (16 hunks)
💤 Files with no reviewable changes (1)
- api/oc_ri_internal.h
🚧 Files skipped from review as they are similar to previous changes (4)
- api/cloud/unittest/cloud_store_test.cpp
- api/oc_collection.c
- api/oc_ri_server_internal.h
- include/oc_ri.h
🧰 Additional context used
🪛 cppcheck
api/oc_ri_server.c
[error] 74-74: There is an unknown macro here somewhere. Configuration is required. If oc_ri_on_delete_resource_find_callback is a macro then please configure it.
(unknownMacro)
🔇 Additional comments (4)
api/oc_ri_server.c (3)
20-50: LGTM: New includes and conditional compilation blocksThe added includes and conditional compilation blocks improve code modularity and feature management. This approach allows for better control over enabled features and dependencies.
52-56: LGTM: New structure for resource defaultsThe
oc_resource_defaults_data_tstructure is well-defined and appropriate for managing resource defaults. It provides a clean way to associate a resource with its interface mask.
Line range hint
1-693: Overall assessment: Significant improvements with some suggestions for enhancementThe changes in this file significantly enhance the resource management and observation handling capabilities of the IoTivity stack. Key improvements include:
- Better modularization through new includes and conditional compilation.
- Enhanced resource allocation and management functions.
- Improved observation handling with support for collections.
- Addition of ETag support for resources and batches.
While these changes are generally well-implemented, several suggestions have been made to further improve the code:
- Enhance error handling and logging across various functions.
- Refactor complex functions for better readability and maintainability.
- Address TODO comments, particularly in observation handling.
- Improve consistency in naming conventions and function signatures.
- Add input validation where appropriate.
Implementing these suggestions will further increase the robustness and maintainability of the code. Great work overall on these improvements!
api/oc_ri.c (1)
630-633: 🛠️ Refactor suggestionConsider checking for state change before notifying observers
The addition of the comment and the call to
oc_ri_notify_resource_observersis good for maintaining consistency after PUT/POST requests. However, notifying observers when the state hasn't changed could lead to unnecessary network traffic.Consider modifying the code to check if the resource state has actually changed:
/* If the recently handled request was a PUT/POST, it conceivably * altered the resource state, so attempt to notify all observers * of that resource with the change. */ - oc_ri_notify_resource_observers(ctx.resource, ctx.iface_mask); + if (resource_state_changed) { + oc_ri_notify_resource_observers(ctx.resource, ctx.iface_mask); + }This change assumes you have a way to track if the resource state has changed during the request handling. If not, you might need to implement such a mechanism.
Likely invalid or redundant comment.
| static void | ||
| ri_delete_resource(oc_resource_t *resource, bool notify) | ||
| { | ||
| OC_DBG("delete resource(%p)", (void *)resource); | ||
|
|
||
| #ifdef OC_COLLECTIONS | ||
| #ifdef OC_COLLECTIONS_IF_CREATE | ||
| oc_rt_created_t *rtc = oc_rt_get_factory_create_for_resource(resource); | ||
| if (rtc != NULL) { | ||
| /* For dynamically created resources invoke the created instance destructor | ||
| * and return. The destructor invokes at the end oc_delete_resource again, | ||
| * but the resource will no longer be in the list of created resources so | ||
| * this if-branch will be skipped and normal resource deallocation will be | ||
| * executed. */ | ||
| oc_rt_factory_free_created_resource(rtc, rtc->rf); | ||
| return; | ||
| } | ||
| #endif /* OC_COLLECTIONS_IF_CREATE */ | ||
|
|
||
| #if defined(OC_RES_BATCH_SUPPORT) && defined(OC_DISCOVERY_RESOURCE_OBSERVABLE) | ||
| bool needsBatchDispatch = false; | ||
| #endif /* OC_RES_BATCH_SUPPORT && OC_DISCOVERY_RESOURCE_OBSERVABLE */ | ||
| // remove the resource from the collections | ||
| oc_collection_t *collection = | ||
| oc_get_next_collection_with_link(resource, NULL); | ||
| while (collection != NULL) { | ||
| oc_link_t *link = oc_get_link_by_uri(collection, oc_string(resource->uri), | ||
| oc_string_len(resource->uri)); | ||
| if (link != NULL) { | ||
| if (oc_collection_remove_link_and_notify( | ||
| &collection->res, link, notify, | ||
| /*discoveryBatchDispatch*/ false)) { | ||
| #if defined(OC_RES_BATCH_SUPPORT) && defined(OC_DISCOVERY_RESOURCE_OBSERVABLE) | ||
| needsBatchDispatch = true; | ||
| #endif /* OC_RES_BATCH_SUPPORT && OC_DISCOVERY_RESOURCE_OBSERVABLE */ | ||
| } | ||
| oc_delete_link(link); | ||
| } | ||
| collection = oc_get_next_collection_with_link(resource, collection); | ||
| } | ||
| #endif /* OC_COLLECTIONS */ | ||
|
|
||
| bool removed = oc_list_remove2(g_app_resources, resource) != NULL; | ||
| removed = | ||
| oc_list_remove2(g_app_resources_to_be_deleted, resource) != NULL || removed; | ||
|
|
||
| oc_remove_delayed_callback(resource, oc_delayed_delete_resource_cb); | ||
| oc_notify_clear(resource); | ||
|
|
||
| if (resource->num_observers > 0) { | ||
| int removed_num = coap_remove_observers_by_resource(resource); | ||
| OC_DBG("removing resource observers: removed(%d) vs expected(%d)", | ||
| removed_num, resource->num_observers); | ||
| #if !OC_DBG_IS_ENABLED | ||
| (void)removed_num; | ||
| #endif /* !OC_DBG_IS_ENABLED */ | ||
| } | ||
|
|
||
| if (notify) { | ||
| if (removed) { | ||
| oc_notify_resource_removed(resource); | ||
| } else { | ||
| #if defined(OC_COLLECTIONS) && defined(OC_RES_BATCH_SUPPORT) && \ | ||
| defined(OC_DISCOVERY_RESOURCE_OBSERVABLE) | ||
| // if oc_notify_resource_removed is not called, then we need to dispatch | ||
| // manually if it is requested | ||
| if (needsBatchDispatch) { | ||
| coap_dispatch_process_batch_observers(); | ||
| } | ||
| #endif /* OC_COLLECTIONS && OC_RES_BATCH_SUPPORT && \ | ||
| OC_DISCOVERY_RESOURCE_OBSERVABLE */ | ||
| } | ||
| } | ||
|
|
||
| oc_ri_free_resource_properties(resource); | ||
| oc_ri_dealloc_resource(resource); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring ri_delete_resource for improved maintainability
The ri_delete_resource function is comprehensive but complex. Consider the following improvements:
- Extract the collection-related operations into a separate function to improve readability.
- Add error handling for the observer removal process.
- Consider using a more structured approach for handling the different compilation configurations.
Here's a sketch of how you might refactor part of this function:
#ifdef OC_COLLECTIONS
static void remove_resource_from_collections(oc_resource_t *resource, bool notify) {
// ... (existing collection removal code)
}
#endif
static void ri_delete_resource(oc_resource_t *resource, bool notify) {
// ... (existing code)
#ifdef OC_COLLECTIONS
remove_resource_from_collections(resource, notify);
#endif
// ... (rest of the function)
if (resource->num_observers > 0) {
int removed_num = coap_remove_observers_by_resource(resource);
if (removed_num != resource->num_observers) {
OC_WRN("Mismatch in observer count: removed %d, expected %d",
removed_num, resource->num_observers);
}
}
// ... (rest of the function)
}This refactoring improves readability and maintainability while also adding better error checking.
| static bool | ||
| ri_add_observation(const coap_packet_t *request, const coap_packet_t *response, | ||
| oc_resource_t *resource, bool resource_is_collection, | ||
| uint16_t block2_size, const oc_endpoint_t *endpoint, | ||
| oc_interface_mask_t iface_query) | ||
| { | ||
| if (ri_observe_handler(request, response, resource, block2_size, endpoint, | ||
| iface_query) >= 0 && | ||
| /* If the resource is marked as periodic observable it means it must be | ||
| * polled internally for updates (which would lead to notifications being | ||
| * sent). If so, add the resource to a list of periodic GET callbacks to | ||
| * utilize the framework's internal polling mechanism. | ||
| */ | ||
| ((resource->properties & OC_PERIODIC) != 0 && | ||
| !oc_periodic_observe_callback_add(resource))) { | ||
| return false; | ||
| } | ||
| #ifdef OC_COLLECTIONS | ||
| if (resource_is_collection) { | ||
| oc_collection_t *collection = (oc_collection_t *)resource; | ||
| if (!ri_add_collection_observation(collection, endpoint, | ||
| iface_query == OC_IF_B)) { | ||
| // TODO: shouldn't we remove the periodic observe callback here? | ||
| return false; | ||
| } | ||
| } | ||
| #else /* !OC_COLLECTIONS */ | ||
| (void)resource_is_collection; | ||
| #endif /* OC_COLLECTIONS */ | ||
| return true; | ||
| } | ||
|
|
||
| static void | ||
| ri_remove_observation(const coap_packet_t *request, | ||
| const coap_packet_t *response, oc_resource_t *resource, | ||
| bool resource_is_collection, uint16_t block2_size, | ||
| const oc_endpoint_t *endpoint, | ||
| oc_interface_mask_t iface_query) | ||
| { | ||
| if (ri_observe_handler(request, response, resource, block2_size, endpoint, | ||
| iface_query) <= 0) { | ||
| return; | ||
| } | ||
| if ((resource->properties & OC_PERIODIC) != 0) { | ||
| oc_periodic_observe_callback_remove(resource); | ||
| } | ||
| #if defined(OC_COLLECTIONS) | ||
| if (resource_is_collection) { | ||
| oc_collection_t *collection = (oc_collection_t *)resource; | ||
| oc_link_t *links = (oc_link_t *)oc_list_head(collection->links); | ||
| for (; links != NULL; links = links->next) { | ||
| if (links->resource != NULL && | ||
| (links->resource->properties & OC_PERIODIC) != 0) { | ||
| oc_periodic_observe_callback_remove(links->resource); | ||
| } | ||
| } | ||
| } | ||
| #else /* !OC_COLLECTIONS */ | ||
| (void)resource_is_collection; | ||
| #endif /* OC_COLLECTIONS */ | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Address TODO comment and consider refactoring observation handling
In the ri_add_observation function, there's a TODO comment that needs to be addressed:
// TODO: shouldn't we remove the periodic observe of links added by this call?This is an important consideration. If an error occurs after adding some periodic observations but before completing the process, we should indeed clean up the partially added observations to prevent resource leaks or inconsistent state.
Consider implementing this cleanup logic. Here's a possible approach:
- Keep track of the links for which we've added periodic observations.
- If an error occurs, iterate through this list and remove the periodic observations.
- Clear the tracking list at the end of the function, whether successful or not.
Additionally, consider extracting the collection-specific logic into a separate function to improve readability and maintainability. This would also make it easier to handle the cleanup process mentioned above.
Here's a sketch of how this refactoring might look:
#ifdef OC_COLLECTIONS
static bool add_collection_observation(oc_collection_t *collection,
const oc_endpoint_t *endpoint,
bool is_batch,
oc_link_t **added_links,
size_t *added_links_count)
{
// ... (existing collection observation logic)
// Keep track of added links in added_links array
// Update added_links_count as links are added
// Return false if any step fails
}
static void remove_collection_observation(oc_link_t **added_links,
size_t added_links_count)
{
for (size_t i = 0; i < added_links_count; i++) {
oc_periodic_observe_callback_remove(added_links[i]->resource);
}
}
#endif
static bool
ri_add_observation(/* ... existing parameters ... */)
{
// ... (existing code)
#ifdef OC_COLLECTIONS
if (resource_is_collection) {
oc_collection_t *collection = (oc_collection_t *)resource;
oc_link_t *added_links[MAX_LINKS]; // Adjust size as needed
size_t added_links_count = 0;
if (!add_collection_observation(collection, endpoint,
iface_query == OC_IF_B,
added_links, &added_links_count)) {
remove_collection_observation(added_links, added_links_count);
return false;
}
}
#endif
return true;
}This refactoring addresses the TODO comment and improves the overall structure of the observation handling code.
| int | ||
| oc_ri_handle_observation(const coap_packet_t *request, coap_packet_t *response, | ||
| oc_resource_t *resource, bool resource_is_collection, | ||
| uint16_t block2_size, const oc_endpoint_t *endpoint, | ||
| oc_interface_mask_t iface_query) | ||
| { | ||
|
|
||
| /* If a GET request was successfully processed, then check if the resource is | ||
| * OBSERVABLE and check its observe option. | ||
| */ | ||
| int32_t observe = OC_COAP_OPTION_OBSERVE_NOT_SET; | ||
| if ((resource->properties & OC_OBSERVABLE) == 0 || | ||
| !coap_options_get_observe(request, &observe)) { | ||
| return OC_COAP_OPTION_OBSERVE_NOT_SET; | ||
| } | ||
|
|
||
| /* If the observe option is set to 0, make an attempt to add the requesting | ||
| * client as an observer. | ||
| */ | ||
| if (observe == OC_COAP_OPTION_OBSERVE_REGISTER) { | ||
| if (!ri_add_observation(request, response, resource, resource_is_collection, | ||
| block2_size, endpoint, iface_query)) { | ||
| coap_remove_observer_by_token(endpoint, request->token, | ||
| request->token_len); | ||
| return OC_COAP_OPTION_OBSERVE_NOT_SET; | ||
| } | ||
| coap_options_set_observe(response, OC_COAP_OPTION_OBSERVE_REGISTER); | ||
| return OC_COAP_OPTION_OBSERVE_REGISTER; | ||
| } | ||
|
|
||
| /* If the observe option is set to 1, make an attempt to remove the | ||
| * requesting client from the list of observers. In addition, remove the | ||
| * resource from the list periodic GET callbacks if it is periodic observable. | ||
| */ | ||
| if (observe == OC_COAP_OPTION_OBSERVE_UNREGISTER) { | ||
| ri_remove_observation(request, response, resource, resource_is_collection, | ||
| block2_size, endpoint, iface_query); | ||
| return OC_COAP_OPTION_OBSERVE_UNREGISTER; | ||
| } | ||
|
|
||
| // if the observe option is >= 2 then we a have a notification | ||
| return observe; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance clarity and error handling in oc_ri_handle_observation
The oc_ri_handle_observation function is well-structured, but consider the following improvements to enhance clarity and error handling:
- Use early returns to simplify the logic flow.
- Add more detailed error logging.
- Consider using an enum for observation actions instead of magic numbers.
Here's an example of how you might refactor this function:
typedef enum {
OC_OBSERVE_REGISTER = 0,
OC_OBSERVE_UNREGISTER = 1,
OC_OBSERVE_NOTIFICATION = 2
} oc_observe_action_t;
int
oc_ri_handle_observation(const coap_packet_t *request, coap_packet_t *response,
oc_resource_t *resource, bool resource_is_collection,
uint16_t block2_size, const oc_endpoint_t *endpoint,
oc_interface_mask_t iface_query)
{
if ((resource->properties & OC_OBSERVABLE) == 0) {
OC_DBG("Resource is not observable");
return OC_COAP_OPTION_OBSERVE_NOT_SET;
}
int32_t observe;
if (!coap_options_get_observe(request, &observe)) {
OC_DBG("Observe option not present in request");
return OC_COAP_OPTION_OBSERVE_NOT_SET;
}
switch ((oc_observe_action_t)observe) {
case OC_OBSERVE_REGISTER:
if (!ri_add_observation(request, response, resource, resource_is_collection,
block2_size, endpoint, iface_query)) {
OC_ERR("Failed to add observation");
coap_remove_observer_by_token(endpoint, request->token, request->token_len);
return OC_COAP_OPTION_OBSERVE_NOT_SET;
}
coap_options_set_observe(response, OC_COAP_OPTION_OBSERVE_REGISTER);
return OC_COAP_OPTION_OBSERVE_REGISTER;
case OC_OBSERVE_UNREGISTER:
ri_remove_observation(request, response, resource, resource_is_collection,
block2_size, endpoint, iface_query);
return OC_COAP_OPTION_OBSERVE_UNREGISTER;
default:
if (observe >= OC_OBSERVE_NOTIFICATION) {
OC_DBG("Received notification with observe option: %d", observe);
return observe;
}
OC_ERR("Invalid observe option: %d", observe);
return OC_COAP_OPTION_OBSERVE_NOT_SET;
}
}This refactored version improves readability, adds more detailed logging, and uses an enum for better type safety and clarity.
| void | ||
| oc_ri_notify_resource_observers(oc_resource_t *resource, | ||
| oc_interface_mask_t iface_mask) | ||
| { | ||
| if ((iface_mask == OC_IF_STARTUP) || (iface_mask == OC_IF_STARTUP_REVERT)) { | ||
| oc_resource_defaults_data_t *resource_defaults_data = | ||
| oc_ri_alloc_resource_defaults(); | ||
| resource_defaults_data->resource = resource; | ||
| resource_defaults_data->iface_mask = iface_mask; | ||
| oc_ri_add_timed_event_callback_ticks( | ||
| resource_defaults_data, | ||
| &ri_observe_notification_resource_defaults_delayed, 0); | ||
| } else { | ||
| oc_notify_resource_changed_delayed_ms(resource, 0); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding configurability to notification delay
The oc_ri_notify_resource_observers function uses a fixed delay of 0 ms for non-startup notifications. Consider making this delay configurable, which could be useful for different use cases or for testing purposes.
You could introduce a new parameter or use a global configuration value. For example:
void
oc_ri_notify_resource_observers(oc_resource_t *resource,
- oc_interface_mask_t iface_mask)
+ oc_interface_mask_t iface_mask,
+ uint32_t delay_ms)
{
// ... (existing code for startup notifications)
} else {
- oc_notify_resource_changed_delayed_ms(resource, 0);
+ oc_notify_resource_changed_delayed_ms(resource, delay_ms);
}
}This change would allow more flexibility in how notifications are scheduled.
Committable suggestion was skipped due to low confidence.
| void | ||
| oc_ri_init(void) | ||
| { | ||
| ri_set_status_codes(); | ||
|
|
||
| #ifdef OC_SERVER | ||
| oc_list_init(g_app_resources); | ||
| oc_list_init(g_app_resources_to_be_deleted); | ||
| oc_ri_server_init(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for server initialization
The addition of oc_ri_server_init(); is good for initializing server-specific components. However, it's recommended to add error handling to manage potential initialization failures.
Consider applying this change:
#ifdef OC_SERVER
- oc_ri_server_init();
+ if (oc_ri_server_init() != 0) {
+ OC_ERR("Failed to initialize OCF Resource Interface server");
+ // Consider appropriate error handling or early return
+ }
#endif /* OC_SERVER */This change will help detect and handle any initialization errors, improving the robustness of the server setup.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| oc_ri_server_init(); | |
| #ifdef OC_SERVER | |
| if (oc_ri_server_init() != 0) { | |
| OC_ERR("Failed to initialize OCF Resource Interface server"); | |
| // Consider appropriate error handling or early return | |
| } | |
| #endif /* OC_SERVER */ |
No description provided.