ports: Enhance TCP Client Disconnect Notifications in TLS Handling #627
Conversation
WalkthroughThe recent updates primarily focus on enhancing the session management within the TLS and TCP layers, introducing a new boolean parameter Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Client
participant Server
participant TLS
participant TCP
Client->>Server: Initiate Connection
Server->>TLS: Establish TLS Session
TLS->>Server: Session Established
Server->>Client: Connection Established
Client->>Server: Send Data
Server->>TLS: Secure Data
TLS->>TCP: Send Data
TCP->>Server: Data Received
Server->>Client: Acknowledge Data
Client->>Server: End Connection
Server->>TLS: Close TLS Session (notify_session_end)
TLS->>TCP: End TCP Session (notify_session_end)
TCP->>Server: Session Ended
Server->>Client: Connection Closed
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
🎉 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 (ec7715e), label this PR with |
jkralik
force-pushed
the
jkralik/fix/closing-tcp-session
branch
from
ec7715e
to
abdd667
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (2)
port/linux/tcpsession.c (1)
303-304: Consider the impact of session end notification on system performance.The conditional check for session end notifications (
session->notify_session_end) could potentially impact system performance if frequently triggered. Consider profiling this part under high load scenarios to ensure it does not become a bottleneck.port/windows/ipadapter.c (1)
1687-1701: The implementation ofoc_connectivity_end_session_v1with thenotify_session_endparameter aligns well with the PR's objectives to manage TCP client disconnect notifications more effectively.Consider adding documentation for the
notify_session_endparameter to clarify its usage and impact.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (19)
- api/cloud/oc_cloud.c (2 hunks)
- api/oc_client_api.c (4 hunks)
- messaging/coap/engine.c (2 hunks)
- messaging/coap/signal.c (2 hunks)
- port/android/ipadapter.c (1 hunks)
- port/android/tcpadapter.c (7 hunks)
- port/android/tcpadapter.h (1 hunks)
- port/esp32/adapter/src/ipadapter.c (1 hunks)
- port/esp32/adapter/src/tcpadapter.c (7 hunks)
- port/esp32/adapter/src/tcpadapter.h (1 hunks)
- port/linux/ipadapter.c (1 hunks)
- port/linux/tcpsession.c (8 hunks)
- port/linux/tcpsession.h (1 hunks)
- port/oc_connectivity.h (1 hunks)
- port/windows/ipadapter.c (1 hunks)
- port/windows/tcpadapter.c (4 hunks)
- port/windows/tcpadapter.h (1 hunks)
- security/oc_tls.c (1 hunks)
- swig/swig_interfaces/oc_connectivity.i (1 hunks)
Files skipped from review due to trivial changes (1)
- swig/swig_interfaces/oc_connectivity.i
Additional comments not posted (25)
port/windows/tcpadapter.h (1)
36-36: Change approved: The function signature foroc_tcp_end_sessionnow correctly includes thenotify_session_endparameter and returns a boolean to indicate success.port/android/tcpadapter.h (1)
42-43: Change approved: The function signature foroc_tcp_end_sessionnow correctly includes thenotify_session_endparameter and returns a boolean to indicate success.Verification successful
The function
oc_tcp_end_sessionintcpadapter.chas been correctly updated to include thenotify_session_endparameter and returns a boolean. The usage inipadapter.calso correctly includes the new parameter.
port/android/tcpadapter.c: The function signature and implementation match the new definition.port/android/ipadapter.c: The function is called with the correct parameters.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `oc_tcp_end_session` is correctly used across the Android platform. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type c "oc_tcp_end_session" port/android/Length of output: 334
Script:
#!/bin/bash # Verify the function signature and implementation in tcpadapter.c rg --type c "oc_tcp_end_session" port/android/tcpadapter.c # Verify the usage of the function in ipadapter.c rg --type c "oc_tcp_end_session" port/android/ipadapter.cLength of output: 253
Script:
#!/bin/bash # Extract the full function signature and implementation in tcpadapter.c rg --type c -A 5 "oc_tcp_end_session" port/android/tcpadapter.cLength of output: 297
port/esp32/adapter/src/tcpadapter.h (1)
42-43: Change approved: The function signature foroc_tcp_end_sessionnow correctly includes thenotify_session_endparameter and returns a boolean to indicate success.Verification successful
The function signature for
oc_tcp_end_sessionintcpadapter.ccorrectly includes the new parameternotify_session_endand matches the updated header file. Additionally, the function call inipadapter.cuses the new parameter correctly.
tcpadapter.c: The function signature is correctly updated.ipadapter.c: The function call includes the new parameternotify_session_end.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `oc_tcp_end_session` is correctly used across the ESP32 platform. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type c "oc_tcp_end_session" port/esp32/Length of output: 362
Script:
#!/bin/bash # Description: Verify the function signature and usage of `oc_tcp_end_session` in `tcpadapter.c` and `ipadapter.c`. # Check the function signature in `tcpadapter.c` rg --type c "oc_tcp_end_session" port/esp32/adapter/src/tcpadapter.c # Check the function call in `ipadapter.c` rg --type c "oc_tcp_end_session" port/esp32/adapter/src/ipadapter.cLength of output: 273
Script:
#!/bin/bash # Description: Extract the full function signature of `oc_tcp_end_session` from `tcpadapter.c`. # Extract the full function signature rg --type c -A 3 "oc_tcp_end_session" port/esp32/adapter/src/tcpadapter.cLength of output: 227
port/linux/tcpsession.h (1)
108-108: Change approved: The function signature fortcp_end_sessionnow correctly includes thenotify_session_endparameter and returns a boolean to indicate success.port/oc_connectivity.h (1)
161-178: Changes approved: The functionoc_connectivity_end_sessionis correctly marked as deprecated, andoc_connectivity_end_session_v1is introduced with thenotify_session_endparameter and a boolean return type.Verification successful
The function
oc_connectivity_end_session_v1is correctly used across the platform with the appropriate parameters and return type.
port/windows/ipadapter.cport/linux/ipadapter.cport/esp32/adapter/src/ipadapter.cport/android/ipadapter.cScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `oc_connectivity_end_session_v1` is correctly used across the platform. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type c "oc_connectivity_end_session_v1" port/Length of output: 898
Script:
#!/bin/bash # Description: Verify the correct usage of `oc_connectivity_end_session_v1` in the identified files. # Check the usage in port/windows/ipadapter.c echo "Checking port/windows/ipadapter.c" rg --context 5 "oc_connectivity_end_session_v1" port/windows/ipadapter.c # Check the usage in port/linux/ipadapter.c echo "Checking port/linux/ipadapter.c" rg --context 5 "oc_connectivity_end_session_v1" port/linux/ipadapter.c # Check the usage in port/esp32/adapter/src/ipadapter.c echo "Checking port/esp32/adapter/src/ipadapter.c" rg --context 5 "oc_connectivity_end_session_v1" port/esp32/adapter/src/ipadapter.c # Check the usage in port/android/ipadapter.c echo "Checking port/android/ipadapter.c" rg --context 5 "oc_connectivity_end_session_v1" port/android/ipadapter.cLength of output: 2252
messaging/coap/signal.c (1)
227-227: Change approved: The use ofoc_close_sessionin handling the RELEASE_7_04 signal is appropriate and aligns with the PR objectives.Verification successful
Let's verify the context and correctness of
oc_close_sessionusage inmessaging/coap/engine.c.
The usage of
oc_close_sessionin bothsignal.candengine.cis appropriate and aligns with the intended functionality of handling TCP client disconnect notifications.
messaging/coap/signal.c: Handles the RELEASE_7_04 signal.messaging/coap/engine.c: Handles invalid inbound messages for TCP connections.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `oc_close_session` is correctly used in COAP signal handling. # Test: Search for the function usage. Expect: Only occurrences of the new usage. rg --type c "oc_close_session" messaging/coap/Length of output: 164
Script:
#!/bin/bash # Description: Verify the context of `oc_close_session` usage in `messaging/coap/engine.c`. # Extract 10 lines before and after the usage of `oc_close_session` in `engine.c` to understand its context. rg --type c -A 10 -B 10 "oc_close_session" messaging/coap/engine.cLength of output: 709
api/cloud/oc_cloud.c (1)
136-136: Simplified session closure logic aligns with PR objectives.port/esp32/adapter/src/tcpadapter.c (2)
162-166: Refactoredfree_tcp_sessionto support conditional notification on session end.Also applies to: 377-377, 383-383, 404-404, 414-414, 424-424
438-447: Updatedoc_tcp_end_sessionto support conditional notification, enhancing control over session management.port/android/tcpadapter.c (2)
192-196: Ensure proper session end notification logic.The updated logic in
free_tcp_sessionto conditionally triggeroc_session_end_eventbased onnotify_session_endand ongoing disconnect checks is a good approach to handle session closures more gracefully.
465-474: Validate session closure inoc_tcp_end_session.The implementation of
oc_tcp_end_sessioncorrectly handles the session search and conditional closure based on thenotify_session_endflag. This aligns well with the PR's objective to improve TCP client disconnect notifications.api/oc_client_api.c (2)
21-30: Validate inclusion of new headers.The inclusion of
oc_endpoint.h,port/oc_connectivity.h, andsecurity/oc_tls_internal.hunder their respective conditional compilation flags is appropriate and aligns with the changes described in the PR summary.
Line range hint
957-970: Ensure correct session closure handling inoc_close_session.The modifications in
oc_close_sessionto useoc_connectivity_end_session_v1for TCP endpoints and handle the session state appropriately are well-implemented. This change supports the PR's goal of fixing TCP client disconnect notifications effectively.port/windows/tcpadapter.c (5)
57-57: Addednotify_session_endtotcp_sessionstruct to control session end notifications.This change aligns with the PR's objective to enhance session management by allowing control over session end notifications.
188-189: Ensure session end events are only triggered when not already disconnecting.This logic prevents redundant session end notifications, which is crucial for maintaining clean session states and avoiding potential race conditions or errors during session cleanup.
197-201: Introduced asynchronous session freeing with notification control.This modification supports the asynchronous handling of session termination, allowing for more efficient resource management and improving the responsiveness of the system under load.
240-240: Defaultnotify_session_endto true when adding a new session.Setting this flag to true by default ensures that session end events are triggered unless explicitly suppressed, which maintains backward compatibility and ensures that existing functionalities relying on these notifications continue to work as expected.
320-329: Refactoredoc_tcp_end_sessionto support notification control.This change is crucial for the flexibility of session management, allowing the caller to specify whether to notify about the session end, which can be useful in scenarios where multiple sessions are ended in a batch, or where notifications are managed at a higher layer.
port/linux/tcpsession.c (1)
242-242: Verify the default value ofnotify_session_endduring session creation.Verification successful
The
tcp_session_tstructure is defined in multiple files, includingport/linux/tcpsession.c, and it contains thenotify_session_endmember. However, the search results do not show the initialization of this member during session creation.To verify the default value of
notify_session_endduring session creation, we need to inspect the initialization code fortcp_session_tinstances, specifically looking for the assignment ofnotify_session_end.Let's search for the initialization of
notify_session_endwithin the context oftcp_session_tcreation.
The search results indicate that the
notify_session_endmember of thetcp_session_tstructure is explicitly initialized totrueduring session creation in multiple instances. This confirms that the default value ofnotify_session_endis set totruewhen a new session is created.Here are the relevant code snippets confirming this:
In
port/linux/tcpsession.c:session->notify_session_end = true;In
port/windows/tcpadapter.c:session->notify_session_end = true;In
port/esp32/adapter/src/tcpadapter.c:session->notify_session_end = true;In
port/android/tcpadapter.c:session->notify_session_end = true;These initializations ensure that
notify_session_endis set totrueby default during the creation of a TCP session.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the default value of `notify_session_end` is set to true in all session creation scenarios. # Test: Search for all occurrences of session creation and check the initialization of `notify_session_end`. rg --type c "oc_memb_alloc" --context 5Length of output: 67307
Script:
#!/bin/bash # Description: Locate the definition of `tcp_session_t` and inspect the initialization of its members. # Step 1: Find the definition of `tcp_session_t`. rg --type c --word "typedef struct tcp_session_t" --context 10 # Step 2: Search for the initialization of `tcp_session_t` members. rg --type c "tcp_session_t" --context 10Length of output: 64700
Script:
#!/bin/bash # Description: Search for the initialization of `notify_session_end` within the context of `tcp_session_t` creation. # Step 1: Find the initialization of `notify_session_end` in `tcp_session_t` instances. rg --type c "notify_session_end" --context 10Length of output: 30451
messaging/coap/engine.c (2)
1104-1104: Verify the impact ofoc_close_sessionon TCP session management.Verification successful
The implementation of
oc_close_sessioninapi/oc_client_api.cconfirms that it handles TCP session termination correctly. Specifically, it checks if the endpoint is a TCP connection and then callsoc_connectivity_end_session_v1followed byoc_handle_sessionto manage the session state.
api/oc_client_api.c:966-972: The function ensures proper TCP session termination by invoking necessary connectivity and session handling functions.This aligns with the intended improvement for TCP client disconnect notifications.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of `oc_close_session` on TCP session management. # Test: Search for the function `oc_close_session` usage in TCP context. Expect: Proper session termination logic. ast-grep --lang c --pattern $'oc_close_session($_, $_) { $$$ }'Length of output: 523
Script:
#!/bin/bash # Description: Verify the impact of `oc_close_session` on TCP session management. # Search for the definition of the `oc_close_session` function. ast-grep --lang c --pattern 'void oc_close_session($_) { $$$ }' # Search for all calls to the `oc_close_session` function. rg 'oc_close_session\('Length of output: 503
Script:
#!/bin/bash # Description: Extract the implementation of the `oc_close_session` function to verify its impact on TCP session management. # Extract the implementation of `oc_close_session` from `api/oc_client_api.c`. ast-grep --lang c --pattern 'void oc_close_session(const oc_endpoint_t *endpoint) { $$$ }' api/oc_client_api.cLength of output: 850
50-50: Ensure the new includeapi/oc_client_api_internal.his necessary.port/android/ipadapter.c (1)
1636-1646: Review the return logic inoc_connectivity_end_session_v1.This function now includes a boolean parameter
notify_session_endwhich affects how session ends are notified. It's crucial to ensure that theoc_tcp_end_sessionfunction, which is called here, properly handles this parameter to maintain the intended behavior across different scenarios.port/esp32/adapter/src/ipadapter.c (2)
1620-1623: Ensure backward compatibility with the originaloc_connectivity_end_sessionfunction.This change maintains backward compatibility by wrapping the new
oc_connectivity_end_session_v1function, ensuring that existing code that uses the old function signature still operates correctly.
1624-1634: Review the implementation ofoc_connectivity_end_session_v1.This function now includes a boolean parameter
notify_session_endwhich is passed tooc_tcp_end_session. It's crucial to ensure that this parameter is being handled correctly withinoc_tcp_end_sessionto manage TCP session notifications effectively.security/oc_tls.c (1)
540-546: Ensure proper handling of thedisconnectedstate inoc_tls_free_peer.
jkralik
force-pushed
the
jkralik/fix/closing-tcp-session
branch
5 times, most recently
from
26a7ebb
to
90d39f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (25)
- api/cloud/oc_cloud.c (1 hunks)
- api/oc_client_api.c (4 hunks)
- api/oc_session_events.c (1 hunks)
- api/plgd/plgd_time.c (1 hunks)
- messaging/coap/engine.c (1 hunks)
- messaging/coap/signal.c (2 hunks)
- port/android/ipadapter.c (1 hunks)
- port/android/tcpadapter.c (7 hunks)
- port/android/tcpadapter.h (1 hunks)
- port/esp32/adapter/src/ipadapter.c (1 hunks)
- port/esp32/adapter/src/tcpadapter.c (7 hunks)
- port/esp32/adapter/src/tcpadapter.h (1 hunks)
- port/linux/ipadapter.c (1 hunks)
- port/linux/tcpsession.c (8 hunks)
- port/linux/tcpsession.h (1 hunks)
- port/oc_connectivity.h (1 hunks)
- port/windows/ipadapter.c (1 hunks)
- port/windows/tcpadapter.c (4 hunks)
- port/windows/tcpadapter.h (1 hunks)
- security/oc_tls.c (18 hunks)
- security/oc_tls_internal.h (1 hunks)
- security/unittest/acltest.cpp (7 hunks)
- security/unittest/rolestest.cpp (1 hunks)
- security/unittest/tlstest.cpp (2 hunks)
- swig/swig_interfaces/oc_connectivity.i (1 hunks)
Files skipped from review due to trivial changes (1)
- swig/swig_interfaces/oc_connectivity.i
Additional comments not posted (44)
port/android/tcpadapter.h (1)
42-43: The updated function signature withnotify_session_endparameter aligns with the PR objectives to enhance session management.Verification successful
The function
oc_tcp_end_sessionappears to be correctly integrated with the new signature in the Android-specific files. However, to ensure thorough verification, let's check the function definition and usage inport/android/tcpadapter.candport/android/ipadapter.c.
The function
oc_tcp_end_sessionis correctly defined and used with the new signature in the Android-specific files. The integration appears consistent and complete.
port/android/tcpadapter.c: The function is defined with the new signature.port/android/ipadapter.c: The function is used with the new signature.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `oc_tcp_end_session` is correctly integrated and used throughout the codebase on Android. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type c $'oc_tcp_end_session'Length of output: 934
Script:
#!/bin/bash # Description: Verify the function definition and usage of `oc_tcp_end_session` in Android-specific files. # Check the function definition in `port/android/tcpadapter.c` rg --type c --context 5 $'oc_tcp_end_session' port/android/tcpadapter.c # Check the function usage in `port/android/ipadapter.c` rg --type c --context 5 $'oc_tcp_end_session' port/android/ipadapter.cLength of output: 706
port/esp32/adapter/src/tcpadapter.h (1)
42-43: The updated function signature withnotify_session_endparameter aligns with the PR objectives to enhance session management.Verification successful
The updated function signature with
notify_session_endparameter is consistently applied across the ESP32, Windows, and Android platforms. The integration appears correct based on the search results.
port/esp32/adapter/src/tcpadapter.hport/esp32/adapter/src/ipadapter.cport/esp32/adapter/src/tcpadapter.cport/windows/tcpadapter.hport/windows/tcpadapter.cport/windows/ipadapter.cport/android/tcpadapter.hport/android/ipadapter.cport/android/tcpadapter.cScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `oc_tcp_end_session` is correctly integrated and used throughout the codebase on ESP32. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type c $'oc_tcp_end_session'Length of output: 934
port/linux/tcpsession.h (1)
108-108: The updated function signature withnotify_session_endparameter aligns with the PR objectives to enhance session management.Verification successful
The updated function signature with
notify_session_endparameter is correctly integrated in the Linux-specific files.
port/linux/ipadapter.cport/linux/tcpsession.cScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `tcp_end_session` is correctly integrated and used throughout the codebase on Linux. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type c $'tcp_end_session'Length of output: 1210
security/unittest/tlstest.cpp (1)
179-179: The updated function call withnotify_session_endparameter in the unit tests aligns with the PR objectives to enhance session management.Also applies to: 211-211
Verification successful
The function
oc_tls_remove_peeris correctly integrated and used throughout the codebase with the newnotify_session_endparameter.
security/oc_tls_internal.h: Function definition includes the new parameter.security/unittest/tlstest.cpp: Updated function calls.security/unittest/rolestest.cpp: Updated function call.security/unittest/acltest.cpp: Multiple updated function calls.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `oc_tls_remove_peer` is correctly integrated and used throughout the codebase. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type cpp $'oc_tls_remove_peer'Length of output: 727
api/oc_session_events.c (1)
335-335: The updated function call withnotify_session_endparameter inhandle_session_disconnectedaligns with the PR objectives to enhance session management.messaging/coap/signal.c (1)
227-227: Proper handling ofRELEASE_7_04signal with session closure.Verification successful
The handling of the
RELEASE_7_04signal in the codebase appears to be correctly implemented, with the session being closed appropriately when this signal is received.
- In
messaging/coap/signal.c, theoc_close_session(endpoint)function is called when theRELEASE_7_04signal is detected, ensuring the session is closed.- The references in
messaging/coap/coap.care related to setting and getting options for theRELEASE_7_04signal, which are consistent with the expected behavior.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct handling of `RELEASE_7_04` signal across the codebase. # Test: Search for handling of `RELEASE_7_04`. Expect: Proper session closure. rg --type c 'RELEASE_7_04'Length of output: 720
Script:
#!/bin/bash # Description: Extract relevant code segments for `RELEASE_7_04` handling. # Extract lines around `RELEASE_7_04` references in `messaging/coap/signal.c` rg --type c --context 5 'RELEASE_7_04' messaging/coap/signal.c # Extract lines around `RELEASE_7_04` references in `messaging/coap/coap.c` rg --type c --context 5 'RELEASE_7_04' messaging/coap/coap.cLength of output: 3025
security/oc_tls_internal.h (1)
163-165: Addition ofnotify_session_endparameter tooc_tls_remove_peerenhances session management.Verification successful
The
notify_session_endparameter inoc_tls_remove_peeris used correctly in the following contexts:
- In
api/oc_session_events.c, it is set tofalsefor secured TCP endpoints, indicating no session end notification.- In
api/plgd/plgd_time.c, it is set totruewhen an insecure peer is added, indicating a session end notification.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct usage of the new `notify_session_end` parameter in `oc_tls_remove_peer` across the codebase. # Test: Search for the function usage. Expect: Correct usage of the new parameter. rg --type c 'oc_tls_remove_peer\('Length of output: 371
Script:
#!/bin/bash # Extract surrounding code for the function call in `api/oc_session_events.c` rg --context 5 'oc_tls_remove_peer\(' api/oc_session_events.c # Extract surrounding code for the function call in `api/plgd/plgd_time.c` rg --context 5 'oc_tls_remove_peer\(' api/plgd/plgd_time.cLength of output: 930
api/cloud/oc_cloud.c (1)
135-135: Proper handling of cloud endpoint closure with session management.Verification successful
The function
oc_cloud_close_endpointis consistently used with proper session management across the codebase. The context around each call shows thatoc_close_sessionis invoked appropriately.
api/cloud/oc_cloud.c: The functionoc_cloud_close_endpointcallsoc_close_sessiondirectly.api/cloud/oc_cloud_context.c: The functionoc_cloud_close_endpointis used in a context where session management is handled.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct handling of cloud endpoint closures across the codebase. # Test: Search for the function usage. Expect: Proper session management. rg --type c 'oc_cloud_close_endpoint\('Length of output: 408
Script:
#!/bin/bash # Description: Verify the context around each call to `oc_cloud_close_endpoint` to ensure proper session management. # Search for the function usage and display surrounding lines for context. rg --type c --context 5 'oc_cloud_close_endpoint\('Length of output: 2862
port/esp32/adapter/src/tcpadapter.c (1)
438-447: The newoc_tcp_end_sessionfunction correctly handles the session termination by utilizing thefree_tcp_sessionfunction with thenotify_session_endparameter. This is a crucial part of managing TCP sessions properly. Good job on implementing this with thread safety using mutex locks.port/android/tcpadapter.c (1)
465-474: The newoc_tcp_end_sessionfunction correctly handles the session termination by utilizing thefree_tcp_sessionfunction with thenotify_session_endparameter. This is a crucial part of managing TCP sessions properly. Good job on implementing this with thread safety using mutex locks.security/unittest/rolestest.cpp (1)
85-85: The addition of thetrueparameter tooc_tls_remove_peeraligns with the PR's objectives to handle session end notifications.Verification successful
All instances of
oc_tls_remove_peerinclude the new boolean parameter, confirming that the change has been consistently applied across the codebase.
security/unittest/tlstest.cppsecurity/unittest/rolestest.cppsecurity/unittest/acltest.cppScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all calls to `oc_tls_remove_peer` include the new boolean parameter. # Test: Search for the function usage without the new parameter. Expect: No occurrences. rg --type cpp --glob '*.*' '!oc_tls_remove_peer\([^,]*\)'Length of output: 59
Script:
#!/bin/bash # Description: Verify that all calls to `oc_tls_remove_peer` include the new boolean parameter. # Test: Search for all instances of `oc_tls_remove_peer` and check if they include the boolean parameter. rg --type cpp --pcre2 'oc_tls_remove_peer\([^,]+,[^)]*\)'Length of output: 752
api/plgd/plgd_time.c (1)
925-925: The addition of thetrueparameter tooc_tls_remove_peercorrectly implements the intended session end notification logic as per the PR objectives.api/oc_client_api.c (2)
21-27: Reordered and added include statements for better modularity.
970-972: Ensure proper session closure for TCP connections.Verification successful
The implementation of
oc_handle_sessioninapi/oc_session_events.cconfirms that it properly handles theOC_SESSION_DISCONNECTEDstate by callinghandle_session_disconnected(endpoint). This ensures proper session closure for TCP connections.
api/oc_session_events.c, lines 348-350:if (state == OC_SESSION_DISCONNECTED) { handle_session_disconnected(endpoint); }Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `oc_handle_session` properly handles the session state change. # Test: Search for the function usage. Expect: Proper handling of OC_SESSION_DISCONNECTED. rg --type python $'oc_handle_session'Length of output: 67
Script:
#!/bin/bash # Description: Verify that `oc_handle_session` properly handles the session state change. # Test: Search for the function usage. Expect: Proper handling of OC_SESSION_DISCONNECTED. rg --type c 'oc_handle_session'Length of output: 650
Script:
#!/bin/bash # Description: Extract the implementation of `oc_handle_session` to verify proper handling of session states. # Extract the function implementation from the relevant file. ast-grep --lang c --pattern $'void oc_handle_session(const oc_endpoint_t *endpoint, oc_session_state_t state) { $$$ }' api/oc_session_events.cLength of output: 782
port/windows/tcpadapter.c (5)
57-57: The addition ofnotify_session_endto thetcp_sessionstruct is appropriate for managing session end notifications.
188-189: The conditional logic to triggeroc_session_end_eventbased onnotify_session_endis correctly implemented.
197-201: Correct handling ofnotify_session_endin asynchronous session termination ensures proper session lifecycle management.
240-240: Settingnotify_session_endto true by default in new sessions is a safe and sensible default.
320-329: Proper propagation ofnotify_session_endinoc_tcp_end_sessionensures that session end notifications are handled correctly.security/unittest/acltest.cpp (6)
326-326: Ensure the newnotify_session_endparameter is correctly handled inoc_tls_remove_peer.
845-845: Ensure that thenotify_session_endparameter inoc_tls_remove_peeris being used appropriately to manage session terminations.Verification successful
The context surrounding the
oc_tls_remove_peercalls shows that thenotify_session_endparameter is consistently set totrueacross various test cases. This indicates that the intention is to notify the session end in all these scenarios. Given the consistent usage pattern, it appears that the parameter is being used appropriately to manage session terminations.
security/unittest/acltest.cpp: Multiple instances whereoc_tls_remove_peer(&ep, true)is called after various assertions and checks.security/unittest/rolestest.cpp:oc_tls_remove_peer(&peer->endpoint, true)is called within theTearDownmethod to clean up peers.security/unittest/tlstest.cpp:oc_tls_remove_peer(&ep, true)is called after sending messages and checking event counts.These usages suggest that the
notify_session_endparameter is correctly used to ensure proper session termination notifications.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure that the `notify_session_end` parameter in `oc_tls_remove_peer` is being used appropriately. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type cpp $'oc_tls_remove_peer'Length of output: 727
Script:
#!/bin/bash # Extract surrounding lines of each `oc_tls_remove_peer` call to understand the context of `notify_session_end` parameter usage. # Extract 5 lines before and after each occurrence of `oc_tls_remove_peer` in the relevant files. rg --type cpp --context 5 'oc_tls_remove_peer' security/unittest/tlstest.cpp security/unittest/rolestest.cpp security/unittest/acltest.cppLength of output: 5860
403-403: Verify that thenotify_session_endparameter is correctly propagated and handled inoc_tls_remove_peer.
737-737: Check the implementation ofoc_tls_remove_peerto ensure thenotify_session_endparameter is being used to control session end notifications correctly.Verification successful
The implementation of
oc_tls_remove_peerinsecurity/oc_tls.cconfirms that thenotify_session_endparameter is used to control session end notifications. Specifically, it is passed to theoc_tls_free_peerfunction, which likely handles the session end notifications based on this parameter.
security/oc_tls.c: oc_tls_remove_peer(const oc_endpoint_t *endpoint, bool notify_session_end)Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the implementation of `oc_tls_remove_peer`. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type cpp $'oc_tls_remove_peer'Length of output: 727
Script:
#!/bin/bash # Description: Locate and display the implementation of `oc_tls_remove_peer`. # Search for the definition of the function in the codebase. ast-grep --lang cpp --pattern 'void oc_tls_remove_peer(const oc_endpoint_t *endpoint, bool notify_session_end) { $$$ }'Length of output: 121
Script:
#!/bin/bash # Description: Search for the definition of `oc_tls_remove_peer` in the codebase. # Search for the function name in the codebase. rg --type cpp 'void oc_tls_remove_peer'Length of output: 149
Script:
#!/bin/bash # Description: Search for the implementation of `oc_tls_remove_peer` in the codebase. # Search for the function name in the codebase. rg --type cpp 'oc_tls_remove_peer'Length of output: 727
Script:
#!/bin/bash # Description: Search for the implementation of `oc_tls_remove_peer` in all files. # Search for the function name in all files. rg 'oc_tls_remove_peer'Length of output: 943
Script:
#!/bin/bash # Description: Extract and display the implementation of `oc_tls_remove_peer` from `security/oc_tls.c`. # Extract the implementation of the function. rg -A 20 'oc_tls_remove_peer(const oc_endpoint_t \*endpoint, bool notify_session_end)' security/oc_tls.cLength of output: 106
Script:
#!/bin/bash # Description: Extract and display the implementation of `oc_tls_remove_peer` from `security/oc_tls.c`. # Extract the implementation of the function with a larger surrounding context. rg -A 50 'oc_tls_remove_peer' security/oc_tls.cLength of output: 1290
649-649: Confirm that thenotify_session_endparameter is correctly utilized inoc_tls_remove_peerto ensure session end notifications are managed as expected.Verification successful
The implementation of the
oc_tls_remove_peerfunction insecurity/oc_tls.cconfirms that thenotify_session_endparameter is correctly utilized. Specifically, the parameter is passed to theoc_tls_free_peerfunction, which handles the session end notifications as expected.
security/oc_tls.c: oc_tls_remove_peer(const oc_endpoint_t *endpoint, bool notify_session_end)Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Confirm that the `notify_session_end` parameter is correctly utilized in `oc_tls_remove_peer`. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type cpp $'oc_tls_remove_peer'Length of output: 727
Script:
#!/bin/bash # Description: Locate and display the implementation of the `oc_tls_remove_peer` function to verify the usage of the `notify_session_end` parameter. # Search for the implementation of the `oc_tls_remove_peer` function in the codebase. ast-grep --lang cpp --pattern $'void oc_tls_remove_peer(const oc_endpoint_t *endpoint, bool notify_session_end) { $$$ }'Length of output: 123
Script:
#!/bin/bash # Description: Search for the implementation of the `oc_tls_remove_peer` function to verify the usage of the `notify_session_end` parameter. # Search for the function name directly in the codebase to locate its implementation. rg --type cpp 'void oc_tls_remove_peer'Length of output: 149
Script:
#!/bin/bash # Description: Search for the implementation of the `oc_tls_remove_peer` function to verify the usage of the `notify_session_end` parameter. # Search for the function name directly in the codebase without the return type to locate its implementation. rg --type cpp 'oc_tls_remove_peer'Length of output: 727
Script:
#!/bin/bash # Description: Search for the implementation of the `oc_tls_remove_peer` function to verify the usage of the `notify_session_end` parameter. # Search for the function name in header files to locate its implementation or macro definitions. rg --type c,h 'oc_tls_remove_peer'Length of output: 62
Script:
#!/bin/bash # Description: Search for the implementation of the `oc_tls_remove_peer` function to verify the usage of the `notify_session_end` parameter. # Search for the function name in `.c` files to locate its implementation. rg --type c 'oc_tls_remove_peer' # Search for the function name in `.h` files to locate its implementation or macro definitions. rg --type h 'oc_tls_remove_peer'Length of output: 508
Script:
#!/bin/bash # Description: Display the implementation of the `oc_tls_remove_peer` function to verify the usage of the `notify_session_end` parameter. # Extract the implementation of `oc_tls_remove_peer` from `security/oc_tls.c`. rg --type c --context 20 'oc_tls_remove_peer(const oc_endpoint_t *endpoint, bool notify_session_end)' security/oc_tls.cLength of output: 121
Script:
#!/bin/bash # Description: Display the implementation of the `oc_tls_remove_peer` function to verify the usage of the `notify_session_end` parameter. # Extract the implementation of `oc_tls_remove_peer` from `security/oc_tls.c` with an increased context range. rg --type c --context 40 'oc_tls_remove_peer' security/oc_tls.cLength of output: 2031
787-787: Review the usage ofnotify_session_endinoc_tls_remove_peerto ensure it aligns with the intended session management enhancements.port/linux/tcpsession.c (8)
63-63: Addednotify_session_endfield totcp_session_tstruct.This change aligns with the PR's objective to enhance session management by allowing the system to determine whether to notify on session end.
99-99: Addednotify_session_endfield totcp_waiting_session_tstruct.This addition is consistent with the changes in
tcp_session_tand supports the same functionality for sessions that are waiting to open a connection.
242-242: Initializednotify_session_endtotrueinadd_new_session_locked.This default setting ensures that session end notifications are enabled by default, which is a safe approach to handle session closures unless specified otherwise.
303-304: Conditionally triggeroc_session_end_eventbased onnotify_session_end.This modification ensures that session end events are only triggered if
notify_session_endis true, providing controlled behavior based on session settings.
589-589: Setnotify_session_endtotrueinadd_new_waiting_session_locked.Ensures that new waiting sessions are also configured to notify on session end by default, maintaining consistency across session types.
687-691: Introducedfree_session_async_lockedfunction to asynchronously free sessions with control over notification.This function enhancement allows for asynchronous session cleanup while respecting the
notify_session_endflag, which is crucial for proper session termination handling.
701-706: Introducedfree_waiting_session_async_lockedfor asynchronous cleanup of waiting sessions.Similar to
free_session_async_locked, this function provides controlled cleanup of waiting sessions, which is essential for maintaining system stability and correct notification behavior.
716-731: Updatedtcp_end_sessionto handle both active and waiting sessions with notification control.This update enhances the session ending logic by incorporating the new
notify_session_endparameter, allowing for flexible session termination based on the context.messaging/coap/engine.c (1)
1103-1103: The addition ofoc_close_sessionfor handling invalid TCP messages aligns with the PR's objective to enhance TCP session management. Good inclusion.port/linux/ipadapter.c (2)
1591-1594: The wrapper functionoc_connectivity_end_sessioncorrectly simplifies the interface by always passingtruefornotify_session_endtooc_connectivity_end_session_v1.
1580-1589: The implementation ofoc_connectivity_end_session_v1correctly handles the TCP flag and passes thenotify_session_endparameter totcp_end_session. Ensure thattcp_end_sessionis updated to handle this parameter.port/esp32/adapter/src/ipadapter.c (2)
1620-1623: Ensure backward compatibility with the deprecatedoc_connectivity_end_session.This change maintains backward compatibility by wrapping the new
oc_connectivity_end_session_v1function, ensuring that existing code that uses the old function signature still works correctly.
1624-1634: Review the implementation ofoc_connectivity_end_session_v1.The function now takes an additional boolean parameter
notify_session_endwhich is used to determine whether to notify about the session end. This is a crucial part of the PR's objective to improve TCP client disconnect notifications. The implementation correctly handles the TCP flag and delegates tooc_tcp_end_sessionif applicable.port/windows/ipadapter.c (2)
1687-1689: The update to always notify on session end when callingoc_connectivity_end_sessionaligns with the PR's objectives to enhance session management.
1691-1701: The refactoring ofoc_connectivity_end_session_v1to include session end notification control is well-implemented and necessary for the enhanced TCP session management.security/oc_tls.c (4)
Line range hint
465-546: The modifications tooc_tls_free_peerto include session end notifications are correctly implemented and align with the PR's objectives to enhance session management.
573-577: The update tooc_tls_remove_peerto support session end notifications via thenotify_session_endparameter is correctly implemented.
591-591: The update tooc_tls_close_peerto support session end notifications via thenotify_session_endparameter is correctly implemented.
2226-2226: The inclusion of thenotify_session_endparameter in theoc_tls_free_peercall withinoc_tls_shutdownensures that session end notifications are handled during shutdown, aligning with the PR's objectives.
jkralik
force-pushed
the
jkralik/fix/closing-tcp-session
branch
2 times, most recently
from
c0ea5d3
to
5c520c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (26)
- api/cloud/oc_cloud.c (1 hunks)
- api/oc_client_api.c (4 hunks)
- api/oc_session_events.c (1 hunks)
- api/plgd/plgd_time.c (1 hunks)
- apps/client_multithread_linux.c (1 hunks)
- messaging/coap/engine.c (1 hunks)
- messaging/coap/signal.c (2 hunks)
- port/android/ipadapter.c (1 hunks)
- port/android/tcpadapter.c (7 hunks)
- port/android/tcpadapter.h (1 hunks)
- port/esp32/adapter/src/ipadapter.c (1 hunks)
- port/esp32/adapter/src/tcpadapter.c (7 hunks)
- port/esp32/adapter/src/tcpadapter.h (1 hunks)
- port/linux/ipadapter.c (1 hunks)
- port/linux/tcpsession.c (8 hunks)
- port/linux/tcpsession.h (1 hunks)
- port/oc_connectivity.h (1 hunks)
- port/windows/ipadapter.c (1 hunks)
- port/windows/tcpadapter.c (4 hunks)
- port/windows/tcpadapter.h (1 hunks)
- security/oc_tls.c (18 hunks)
- security/oc_tls_internal.h (1 hunks)
- security/unittest/acltest.cpp (6 hunks)
- security/unittest/rolestest.cpp (1 hunks)
- security/unittest/tlstest.cpp (2 hunks)
- swig/swig_interfaces/oc_connectivity.i (1 hunks)
Files skipped from review as they are similar to previous changes (25)
- api/cloud/oc_cloud.c
- api/oc_client_api.c
- api/oc_session_events.c
- api/plgd/plgd_time.c
- messaging/coap/engine.c
- messaging/coap/signal.c
- port/android/ipadapter.c
- port/android/tcpadapter.c
- port/android/tcpadapter.h
- port/esp32/adapter/src/ipadapter.c
- port/esp32/adapter/src/tcpadapter.c
- port/esp32/adapter/src/tcpadapter.h
- port/linux/ipadapter.c
- port/linux/tcpsession.c
- port/linux/tcpsession.h
- port/oc_connectivity.h
- port/windows/ipadapter.c
- port/windows/tcpadapter.c
- port/windows/tcpadapter.h
- security/oc_tls.c
- security/oc_tls_internal.h
- security/unittest/acltest.cpp
- security/unittest/rolestest.cpp
- security/unittest/tlstest.cpp
- swig/swig_interfaces/oc_connectivity.i
Additional comments not posted (1)
apps/client_multithread_linux.c (1)
110-110: Change fromoc_connectivity_end_sessiontooc_close_sessionaligns with the PR's objectives to improve session management.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (27)
- api/cloud/oc_cloud.c (1 hunks)
- api/oc_client_api.c (4 hunks)
- api/oc_session_events.c (1 hunks)
- api/plgd/plgd_time.c (1 hunks)
- apps/client_multithread_linux.c (1 hunks)
- messaging/coap/engine.c (1 hunks)
- messaging/coap/signal.c (2 hunks)
- port/android/ipadapter.c (1 hunks)
- port/android/tcpadapter.c (7 hunks)
- port/android/tcpadapter.h (1 hunks)
- port/esp32/adapter/src/ipadapter.c (1 hunks)
- port/esp32/adapter/src/tcpadapter.c (7 hunks)
- port/esp32/adapter/src/tcpadapter.h (1 hunks)
- port/linux/ipadapter.c (1 hunks)
- port/linux/tcpsession.c (8 hunks)
- port/linux/tcpsession.h (1 hunks)
- port/oc_connectivity.h (1 hunks)
- port/oc_connectivity_internal.h (1 hunks)
- port/windows/ipadapter.c (1 hunks)
- port/windows/tcpadapter.c (4 hunks)
- port/windows/tcpadapter.h (1 hunks)
- security/oc_tls.c (18 hunks)
- security/oc_tls_internal.h (1 hunks)
- security/unittest/acltest.cpp (6 hunks)
- security/unittest/rolestest.cpp (1 hunks)
- security/unittest/tlstest.cpp (2 hunks)
- swig/swig_interfaces/oc_connectivity.i (1 hunks)
Files skipped from review as they are similar to previous changes (26)
- api/cloud/oc_cloud.c
- api/oc_client_api.c
- api/oc_session_events.c
- api/plgd/plgd_time.c
- apps/client_multithread_linux.c
- messaging/coap/engine.c
- messaging/coap/signal.c
- port/android/ipadapter.c
- port/android/tcpadapter.c
- port/android/tcpadapter.h
- port/esp32/adapter/src/ipadapter.c
- port/esp32/adapter/src/tcpadapter.c
- port/esp32/adapter/src/tcpadapter.h
- port/linux/ipadapter.c
- port/linux/tcpsession.c
- port/linux/tcpsession.h
- port/oc_connectivity.h
- port/windows/ipadapter.c
- port/windows/tcpadapter.c
- port/windows/tcpadapter.h
- security/oc_tls.c
- security/oc_tls_internal.h
- security/unittest/acltest.cpp
- security/unittest/rolestest.cpp
- security/unittest/tlstest.cpp
- swig/swig_interfaces/oc_connectivity.i
Additional comments not posted (1)
port/oc_connectivity_internal.h (1)
101-110: The implementation ofoc_connectivity_end_session_v1aligns well with the PR's objectives to enhance session management. The addition of thenotify_session_endparameter is a thoughtful inclusion for better control over session notifications.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
api/oc_tcp.c (1)
38-42: The implementation ofoc_tcp_get_new_session_idcorrectly uses atomic operations to ensure thread safety. Consider adding a comment explaining the handling of the wrap-around case to zero, which ensures a non-zero session ID is always returned.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- api/oc_endpoint.c (2 hunks)
- api/oc_tcp.c (1 hunks)
- api/oc_tcp_internal.h (2 hunks)
- include/oc_endpoint.h (1 hunks)
- port/android/tcpadapter.c (8 hunks)
- port/esp32/adapter/src/tcpadapter.c (8 hunks)
- port/linux/tcpsession.c (10 hunks)
- port/windows/tcpadapter.c (4 hunks)
- security/oc_tls.c (19 hunks)
Files skipped from review as they are similar to previous changes (4)
- port/android/tcpadapter.c
- port/esp32/adapter/src/tcpadapter.c
- port/windows/tcpadapter.c
- security/oc_tls.c
Additional comments not posted (12)
api/oc_tcp_internal.h (1)
39-39: The declaration ofoc_tcp_get_new_session_idis correctly added to support unique session ID generation for TCP sessions.include/oc_endpoint.h (1)
110-111: The addition of thesession_idfield to theoc_endpoint_tstructure is correctly implemented to support pairing TLS peers with TCP sessions. The comment clearly explains the meaning of a session ID of 0.api/oc_endpoint.c (1)
679-687: The implementation ofoc_endpoint_compare_session_idscorrectly handles session ID comparison, including the case where a session ID of 0 acts as a wildcard. This supports flexible session management.port/linux/tcpsession.c (9)
63-63: Addednotify_session_endfield totcp_session_tstruct.This change aligns with the PR's objective to enhance session management by allowing control over session end notifications.
99-99: Addednotify_session_endfield totcp_waiting_session_tstruct.This addition is consistent with the changes in
tcp_session_tand supports the same functionality for waiting sessions.
257-260: Initialization ofnotify_session_endandsession_idinadd_new_session_locked.Proper initialization of new fields ensures that sessions handle end notifications correctly and have unique session IDs.
321-322: Conditionally triggering session end events based onnotify_session_end.This logic ensures that session end notifications are only sent when appropriate, which is crucial for correct session management.
333-334: Logging session details when freeing a session.Enhanced logging provides better visibility into session management, which is useful for debugging and monitoring.
608-608: Initialization ofnotify_session_endinadd_new_waiting_session_locked.Consistent with other session-related changes, ensuring that waiting sessions also respect the notification settings.
710-710: Settingnotify_session_endduring asynchronous session freeing.This change allows dynamic control over notification behavior during session cleanup, which can be important for resource management.
725-725: Settingnotify_session_endfor waiting sessions during asynchronous cleanup.Ensures that the notification settings are respected even during the cleanup of waiting sessions.
735-755: Updatedtcp_end_sessionto handle session end notifications.The function now correctly handles both active and waiting sessions, ensuring that notifications are managed according to the new
notify_session_endparameter.
jkralik
force-pushed
the
jkralik/fix/closing-tcp-session
branch
from
446fdaf
to
9789aed
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (19)
- api/oc_client_api.c (4 hunks)
- api/oc_endpoint.c (2 hunks)
- api/oc_tcp.c (1 hunks)
- api/oc_tcp_internal.h (2 hunks)
- include/oc_endpoint.h (1 hunks)
- port/android/ipadapter.c (1 hunks)
- port/android/tcpadapter.c (8 hunks)
- port/android/tcpadapter.h (1 hunks)
- port/esp32/adapter/src/ipadapter.c (1 hunks)
- port/esp32/adapter/src/tcpadapter.c (8 hunks)
- port/esp32/adapter/src/tcpadapter.h (1 hunks)
- port/linux/ipadapter.c (1 hunks)
- port/linux/tcpsession.c (10 hunks)
- port/linux/tcpsession.h (1 hunks)
- port/oc_connectivity_internal.h (1 hunks)
- port/windows/ipadapter.c (1 hunks)
- port/windows/tcpadapter.c (4 hunks)
- port/windows/tcpadapter.h (1 hunks)
- security/oc_tls.c (20 hunks)
Files skipped from review due to trivial changes (1)
- port/esp32/adapter/src/ipadapter.c
Files skipped from review as they are similar to previous changes (16)
- api/oc_client_api.c
- api/oc_endpoint.c
- api/oc_tcp.c
- api/oc_tcp_internal.h
- include/oc_endpoint.h
- port/android/ipadapter.c
- port/android/tcpadapter.c
- port/esp32/adapter/src/tcpadapter.c
- port/esp32/adapter/src/tcpadapter.h
- port/linux/ipadapter.c
- port/linux/tcpsession.h
- port/oc_connectivity_internal.h
- port/windows/ipadapter.c
- port/windows/tcpadapter.c
- port/windows/tcpadapter.h
- security/oc_tls.c
Additional comments not posted (2)
port/android/tcpadapter.h (1)
42-44: The addition of thenotify_session_endparameter tooc_tcp_end_sessionenhances control over session notifications. Ensure all calls to this function are updated to pass this new parameter.Verification successful
The verification confirms that the function declarations and calls to
oc_tcp_end_sessionacross various files include the newnotify_session_endparameter. This ensures consistency and proper integration of the new parameter.
port/windows/tcpadapter.hport/windows/ipadapter.cport/esp32/adapter/src/tcpadapter.hport/esp32/adapter/src/tcpadapter.cport/esp32/adapter/src/ipadapter.cport/windows/tcpadapter.cport/android/tcpadapter.hport/android/tcpadapter.cport/android/ipadapter.cScripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `oc_tcp_end_session` include the new parameter. # Test: Search for the function usage. Expect: Only occurances with the new parameter. rg --type c $'oc_tcp_end_session'Length of output: 949
Script:
#!/bin/bash # Description: Verify function declarations of `oc_tcp_end_session` include the new parameter. # Test: Search for the function declarations. Expect: Declarations with the new parameter. rg --type c --glob '*tcpadapter.h' --glob '*tcpadapter.c' --glob '*ipadapter.c' $'oc_tcp_end_session'Length of output: 1017
port/linux/tcpsession.c (1)
63-63: The integration of thenotify_session_endparameter across various functions and structures intcpsession.cis consistent and aligns with the PR's objectives to manage TCP client disconnect notifications more effectively. Ensure that the logic surrounding this parameter's usage correctly handles all edge cases, especially in asynchronous session management.Also applies to: 99-99, 257-260, 321-322, 706-710, 720-725, 735-762
Verification successful
The integration of the
notify_session_endparameter across various functions and structures intcpsession.cand other related files is consistent and aligns with the PR's objectives to manage TCP client disconnect notifications more effectively. The parameter is properly checked in conditionals before usage, ensuring correct handling in session management.
port/linux/tcpsession.c: Proper checks before usage ofnotify_session_end.port/windows/tcpadapter.c: Proper checks before usage ofnotify_session_end.port/esp32/adapter/src/tcpadapter.c: Proper checks before usage ofnotify_session_end.port/android/tcpadapter.c: Proper checks before usage ofnotify_session_end.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct handling of `notify_session_end` across session management functions. # Test: Search for usage of `notify_session_end` in conditionals. Expect: Proper checks before usage. rg --type c $'notify_session_end' | grep 'if'Length of output: 4006
jkralik
force-pushed
the
jkralik/fix/closing-tcp-session
branch
from
9789aed
to
71b84a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (19)
- api/oc_client_api.c (4 hunks)
- api/oc_endpoint.c (2 hunks)
- api/oc_tcp.c (1 hunks)
- api/oc_tcp_internal.h (2 hunks)
- include/oc_endpoint.h (1 hunks)
- port/android/ipadapter.c (1 hunks)
- port/android/tcpadapter.c (8 hunks)
- port/android/tcpadapter.h (1 hunks)
- port/esp32/adapter/src/ipadapter.c (1 hunks)
- port/esp32/adapter/src/tcpadapter.c (8 hunks)
- port/esp32/adapter/src/tcpadapter.h (1 hunks)
- port/linux/ipadapter.c (1 hunks)
- port/linux/tcpsession.c (10 hunks)
- port/linux/tcpsession.h (1 hunks)
- port/oc_connectivity_internal.h (1 hunks)
- port/windows/ipadapter.c (1 hunks)
- port/windows/tcpadapter.c (4 hunks)
- port/windows/tcpadapter.h (1 hunks)
- security/oc_tls.c (20 hunks)
Files skipped from review due to trivial changes (1)
- port/esp32/adapter/src/ipadapter.c
Files skipped from review as they are similar to previous changes (17)
- api/oc_client_api.c
- api/oc_endpoint.c
- api/oc_tcp.c
- api/oc_tcp_internal.h
- include/oc_endpoint.h
- port/android/ipadapter.c
- port/android/tcpadapter.c
- port/android/tcpadapter.h
- port/esp32/adapter/src/tcpadapter.c
- port/esp32/adapter/src/tcpadapter.h
- port/linux/ipadapter.c
- port/linux/tcpsession.h
- port/oc_connectivity_internal.h
- port/windows/ipadapter.c
- port/windows/tcpadapter.c
- port/windows/tcpadapter.h
- security/oc_tls.c
Additional comments not posted (1)
port/linux/tcpsession.c (1)
257-260: Validate the session ID generation logic.This script checks the usage of
oc_tcp_get_new_session_idto ensure it's being used correctly to generate unique session IDs.
jkralik
force-pushed
the
jkralik/fix/closing-tcp-session
branch
from
71b84a7
to
7f0b328
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (19)
- api/oc_client_api.c (4 hunks)
- api/oc_endpoint.c (2 hunks)
- api/oc_tcp.c (1 hunks)
- api/oc_tcp_internal.h (2 hunks)
- include/oc_endpoint.h (1 hunks)
- port/android/ipadapter.c (1 hunks)
- port/android/tcpadapter.c (8 hunks)
- port/android/tcpadapter.h (1 hunks)
- port/esp32/adapter/src/ipadapter.c (1 hunks)
- port/esp32/adapter/src/tcpadapter.c (8 hunks)
- port/esp32/adapter/src/tcpadapter.h (1 hunks)
- port/linux/ipadapter.c (1 hunks)
- port/linux/tcpsession.c (10 hunks)
- port/linux/tcpsession.h (1 hunks)
- port/oc_connectivity_internal.h (1 hunks)
- port/windows/ipadapter.c (1 hunks)
- port/windows/tcpadapter.c (4 hunks)
- port/windows/tcpadapter.h (1 hunks)
- security/oc_tls.c (21 hunks)
Files skipped from review as they are similar to previous changes (18)
- api/oc_client_api.c
- api/oc_endpoint.c
- api/oc_tcp.c
- api/oc_tcp_internal.h
- include/oc_endpoint.h
- port/android/ipadapter.c
- port/android/tcpadapter.c
- port/android/tcpadapter.h
- port/esp32/adapter/src/ipadapter.c
- port/esp32/adapter/src/tcpadapter.c
- port/esp32/adapter/src/tcpadapter.h
- port/linux/ipadapter.c
- port/linux/tcpsession.h
- port/oc_connectivity_internal.h
- port/windows/ipadapter.c
- port/windows/tcpadapter.c
- port/windows/tcpadapter.h
- security/oc_tls.c
Additional comments not posted (6)
port/linux/tcpsession.c (6)
257-260: Initialization ofnotify_session_endtotrueinadd_new_session_lockedfunction is crucial for ensuring that session ends are notified by default. This aligns with the PR's objective to manage session end notifications effectively.
321-322: The condition for session end notification is well implemented. It checks if the disconnect is not ongoing and if the session end should be notified, which is a robust way to handle conditional notifications.
608-608: The default setting ofnotify_session_endtotrueinadd_new_waiting_session_lockedensures that new waiting sessions are also prepared to handle end notifications. This consistency across session types is good for maintainability.
706-710: The asynchronous session freeing logic infree_session_async_lockedcorrectly modifies thenotify_session_endfield. This allows dynamic control over notification behavior during session cleanup.
720-725: Similar tofree_session_async_locked, thefree_waiting_session_async_lockedfunction handles thenotify_session_endflag appropriately, ensuring that waiting sessions are also cleaned up according to the notification settings.
735-762: Thetcp_end_sessionfunction effectively uses thenotify_session_endparameter to decide whether to notify the end of a session. This function is a key part of the PR's objective to manage disconnect notifications correctly.
|
@coderabbitai update description for this pull request based on changes |
|
Tip For best results, initiate chat on the files or code changes. @jkralik, based on the changes made in the PR, here's an updated description that you can use: Title: Enhance TCP Client Disconnect Notifications in TLS Handling Description: Key Changes:
This update aims to provide better control over how session ends are notified, which is crucial for maintaining stable and secure communications in networked applications. Feel free to adjust the wording as you see fit! |
jkralik
changed the title
Danielius1922
force-pushed
the
jkralik/fix/closing-tcp-session
branch
from
7f0b328
to
ff02ba0
Compare
ocf-conformance-test-tool
bot
removed
the
OCF Conformance Testing
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range and nitpick comments (1)
port/android/tcpadapter.c (1)
Line range hint
217-242: Refactor to improve session ID assignment logic.The session ID assignment can be simplified by using a ternary operator, reducing the number of lines and improving readability.
- if (session_id == 0) { - session->endpoint.session_id = oc_tcp_get_new_session_id(); - } else { - session->endpoint.session_id = session_id; - } + session->endpoint.session_id = (session_id == 0) ? oc_tcp_get_new_session_id() : session_id;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (32)
- api/cloud/oc_cloud.c (1 hunks)
- api/oc_client_api.c (3 hunks)
- api/oc_endpoint.c (2 hunks)
- api/oc_session_events.c (3 hunks)
- api/oc_session_events_internal.h (4 hunks)
- api/oc_tcp.c (1 hunks)
- api/oc_tcp_internal.h (2 hunks)
- api/plgd/plgd_time.c (1 hunks)
- apps/client_multithread_linux.c (1 hunks)
- include/oc_endpoint.h (1 hunks)
- messaging/coap/engine.c (1 hunks)
- messaging/coap/signal.c (2 hunks)
- port/android/ipadapter.c (1 hunks)
- port/android/tcpadapter.c (13 hunks)
- port/android/tcpadapter.h (1 hunks)
- port/esp32/adapter/src/ipadapter.c (1 hunks)
- port/esp32/adapter/src/tcpadapter.c (13 hunks)
- port/esp32/adapter/src/tcpadapter.h (1 hunks)
- port/linux/ipadapter.c (1 hunks)
- port/linux/tcpsession.c (16 hunks)
- port/linux/tcpsession.h (4 hunks)
- port/oc_connectivity.h (1 hunks)
- port/oc_connectivity_internal.h (3 hunks)
- port/windows/ipadapter.c (1 hunks)
- port/windows/tcpadapter.c (9 hunks)
- port/windows/tcpadapter.h (1 hunks)
- security/oc_tls.c (20 hunks)
- security/oc_tls_internal.h (1 hunks)
- security/unittest/acltest.cpp (6 hunks)
- security/unittest/rolestest.cpp (1 hunks)
- security/unittest/tlstest.cpp (2 hunks)
- swig/swig_interfaces/oc_connectivity.i (1 hunks)
Files skipped from review due to trivial changes (1)
- swig/swig_interfaces/oc_connectivity.i
Additional comments not posted (66)
port/esp32/adapter/src/tcpadapter.h (1)
42-44: The updated function signature foroc_tcp_end_sessionon the ESP32 platform includes adevparameter for device-specific session management, enhancing the flexibility of TCP session handling.Verification successful
The function
oc_tcp_end_sessionhas different signatures across various platforms, indicating platform-specific implementations. The ESP32 platform's signature includes an additionalip_context_t *devparameter, which is not present in the Windows implementation.To ensure the changes are correctly integrated and do not affect other platforms, we need to verify the usage and integration of this function specifically for the ESP32 platform.
The function
oc_tcp_end_sessionwith the new signature is consistently used across the ESP32-specific files. This confirms that the changes are correctly integrated for the ESP32 platform without affecting other platforms.
port/esp32/adapter/src/tcpadapter.hport/esp32/adapter/src/tcpadapter.cport/esp32/adapter/src/ipadapter.cThe integration appears to be correct and consistent within the ESP32-specific code.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `oc_tcp_end_session` is correctly integrated and used throughout the codebase on the ESP32 platform. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type c $'oc_tcp_end_session'Length of output: 949
Script:
#!/bin/bash # Description: Verify the usage of `oc_tcp_end_session` specifically for the ESP32 platform. # Test: Search for the function usage in ESP32-specific files. Expect: Only occurrences of the new signature. rg --type c $'oc_tcp_end_session' port/esp32/adapter/src/Length of output: 373
api/oc_tcp_internal.h (2)
29-29: Including<stdint.h>is necessary for using standard integer types likeuint32_t, which is used in the new session ID function.
39-39: The new functionoc_tcp_get_new_session_idis a crucial addition for generating unique session IDs, enhancing the management of TCP sessions.Verification successful
The function
oc_tcp_get_new_session_idis correctly integrated and used throughout the codebase. Here are the locations where it is used:
security/oc_tls.cport/windows/tcpadapter.cport/linux/tcpsession.cport/esp32/adapter/src/tcpadapter.cport/android/tcpadapter.capi/oc_tcp_internal.hapi/oc_tcp.cThe function is being utilized to assign new session IDs, which aligns with its intended purpose.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `oc_tcp_get_new_session_id` is correctly integrated and used throughout the codebase. # Test: Search for the function usage. Expect: Only occurrences of the new function. rg --type c $'oc_tcp_get_new_session_id'Length of output: 678
api/oc_session_events_internal.h (2)
25-25: Including"util/oc_features.h"is necessary for accessing feature flags, which are likely used in the session events API.
37-37: The documentation foroc_handle_sessionhas been clarified to better describe its functionality, which is crucial for managing session events.Verification successful
The
oc_handle_sessionfunction is used in several files, indicating its importance in managing session events. To ensure the documentation is clear and the function is correctly integrated, we need to review the context of each usage.
The context provided for the
oc_handle_sessionfunction usage in various files shows that it is correctly integrated and used to manage session events. The documentation update appears to be clear and accurately describes its functionality.
security/oc_tls.c: The function is used to handle session disconnection and connection events.api/oc_session_events.c: The function is used within loops to handle session start and end events.api/oc_client_api.c: The function is used to handle session disconnection for TCP endpoints.The documentation for
oc_handle_sessionhas been clarified to better describe its functionality, which is crucial for managing session events.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `oc_handle_session` is correctly integrated and used throughout the codebase. # Test: Search for the function usage. Expect: Only occurrences of the updated documentation. rg --type c $'oc_handle_session'Length of output: 659
Script:
#!/bin/bash # Description: Retrieve the context of each usage of `oc_handle_session` to verify correct integration and documentation clarity. # Get the context of usage in security/oc_tls.c echo "Context in security/oc_tls.c:" rg --context 5 'oc_handle_session' security/oc_tls.c # Get the context of usage in api/oc_session_events.c echo "Context in api/oc_session_events.c:" rg --context 5 'oc_handle_session' api/oc_session_events.c # Get the context of usage in api/oc_client_api.c echo "Context in api/oc_client_api.c:" rg --context 5 'oc_handle_session' api/oc_client_api.cLength of output: 2408
port/oc_connectivity_internal.h (2)
28-28: Including"util/oc_compiler.h"is necessary for using compiler-specific directives, which are likely used in the connectivity API.
99-102: The updated functionoc_connectivity_end_session_v1enhances session management by including asession_endpointparameter, aligning with the PR's focus on enhancing session management.Verification successful
The function
oc_connectivity_end_session_v1is correctly integrated and used throughout the codebase with the new signature.
security/oc_tls.cport/windows/ipadapter.cport/esp32/adapter/src/ipadapter.cport/linux/ipadapter.cport/android/ipadapter.capi/oc_client_api.cScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `oc_connectivity_end_session_v1` is correctly integrated and used throughout the codebase. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type c $'oc_connectivity_end_session_v1'Length of output: 1046
port/linux/tcpsession.h (1)
110-111: The updatedtcp_end_sessionfunction now supports controlled session end notifications and specific session endpoint handling. Ensure that all calls to this function across the system are updated to pass the new parameters.Verification successful
The search results indicate that the function
tcp_end_sessionhas been updated in the Linux platform files. However, there are similar functions (oc_tcp_end_session) in other platform-specific files (Windows, Android, ESP32) that need to be checked for consistency with the new function signature.Let's verify the function signatures in these files to ensure they match the new signature.
The absence of output suggests that the function signatures in the platform-specific files have not been updated to match the new signature. Let's extract the function definitions from the relevant files for manual verification.
The function definitions for
oc_tcp_end_sessionin the Windows, Android, and ESP32 platform files match the new signature. This confirms that the updates have been correctly applied across the platform-specific files.
port/windows/tcpadapter.hport/android/tcpadapter.hport/esp32/adapter/src/tcpadapter.hScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `tcp_end_session` match the new signature. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type c $'tcp_end_session'Length of output: 1242
Script:
#!/bin/bash # Description: Verify the function signatures of `oc_tcp_end_session` in platform-specific files. # Test: Search for the function definitions. Expect: All definitions should match the new signature. rg --type c --files-with-matches $'bool oc_tcp_end_session(const oc_endpoint_t *endpoint, bool notify_session_end, oc_endpoint_t *session_endpoint)'Length of output: 149
Script:
#!/bin/bash # Description: Extract the function definitions of `oc_tcp_end_session` in platform-specific files for manual verification. # Extract function definitions from Windows platform files rg --type c --context 5 $'bool oc_tcp_end_session' port/windows/tcpadapter.h port/windows/tcpadapter.c port/windows/ipadapter.c # Extract function definitions from Android platform files rg --type c --context 5 $'bool oc_tcp_end_session' port/android/tcpadapter.h port/android/tcpadapter.c port/android/ipadapter.c # Extract function definitions from ESP32 platform files rg --type c --context 5 $'bool oc_tcp_end_session' port/esp32/adapter/src/tcpadapter.h port/esp32/adapter/src/tcpadapter.c port/esp32/adapter/src/ipadapter.cLength of output: 2603
api/oc_tcp.c (1)
37-42: The implementation ofoc_tcp_get_new_session_idcorrectly uses atomic operations to ensure thread safety and uniqueness of session IDs. Good handling of the session ID wrap-around case.include/oc_endpoint.h (1)
110-112: The addition of thesession_idfield inoc_endpoint_tis a crucial update for pairing TLS peers with TCP sessions. Ensure that all uses ofoc_endpoint_tacross the system properly initialize and use this new field.Verification successful
The addition of the
session_idfield inoc_endpoint_tis a crucial update for pairing TLS peers with TCP sessions. We need to ensure that all uses ofoc_endpoint_tacross the system properly initialize and use this new field.
The
session_idfield in theoc_endpoint_tstructure is being properly initialized and used across various files, includingsecurity/oc_tls.c,port/windows/tcpadapter.c,port/linux/tcpsession.c,port/esp32/adapter/src/tcpadapter.c, andport/android/tcpadapter.c. This confirms that the new field is being handled correctly in the codebase.
security/oc_tls.c: Thesession_idis checked and assigned values.port/windows/tcpadapter.c,port/linux/tcpsession.c,port/esp32/adapter/src/tcpadapter.c,port/android/tcpadapter.c: Thesession_idis initialized and assigned new session IDs.This thorough initialization and usage indicate that the addition of the
session_idfield is well-integrated into the system.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all uses of `oc_endpoint_t` properly handle the new `session_id` field. # Test: Search for the structure usage. Expect: Proper initialization and usage of `session_id`. rg --type c $'oc_endpoint_t'Length of output: 67114
Script:
#!/bin/bash # Description: Verify initialization of the `session_id` field in `oc_endpoint_t` structure. # Test: Search for the initialization of `session_id` in the codebase. rg --type c 'session_id\s*='Length of output: 1823
api/oc_session_events.c (1)
Line range hint
136-160: The assertions inoc_session_start_eventandoc_session_end_eventensure that only specific sessions (non-zero session IDs) are handled. Verify that all session-related functions across the system correctly handle session IDs.port/oc_connectivity.h (1)
161-168: The functionoc_connectivity_end_sessionis correctly marked as deprecated and replaced byoc_close_session. Plan to remove this deprecated function in future versions to clean up the codebase.messaging/coap/signal.c (1)
227-227: Replace session end function to support enhanced notification control.Verification successful
Verify the implementation of
oc_close_sessionto ensure it supports the newnotify_session_endparameter and behaves correctly.
The implementation of
oc_close_sessioninapi/oc_client_api.cshows that it handles both secured and TCP connections appropriately. The function checks the endpoint flags and calls the relevant functions to close the connection. This confirms that the function is used consistently and correctly across the codebase.
oc_close_sessionhandles secured connections by callingoc_tls_close_connection.- For TCP connections, it uses
oc_connectivity_end_session_v1andoc_handle_session.No issues were found with the usage of
oc_close_session.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `oc_close_session` is used consistently and correctly across all relevant files. # Test: Search for the function usage. Expect: Only occurances with correct context and parameters. rg --type c "oc_close_session"Length of output: 628
Script:
#!/bin/bash # Description: Retrieve the implementation of `oc_close_session` to verify its behavior. # Test: Extract the function definition and body. Expect: Correct handling of `notify_session_end` parameter. ast-grep --lang c --pattern 'void oc_close_session(const oc_endpoint_t *endpoint) { $$$ }'Length of output: 1270
apps/client_multithread_linux.c (1)
110-110: Update session closure to enhance notification control.Verification successful
Verify the context and parameters of
oc_close_sessionusage.
The context and parameters of
oc_close_sessionusage across the identified files appear consistent and correct. The function is used to close sessions in various scenarios, such as handling TCP connections, security checks, and cloud endpoint closures. The usage aligns with the function's purpose as described in the header files.
api/plgd/plgd_time.c: Used within a conditional block checking for TCP or security flags.api/oc_client_api.c: Defined with a security check for secured endpoints.apps/client_multithread_linux.c: Used to close the connection after retry attempts.include/oc_api.h: Declared with a description of its parameters.api/cloud/oc_cloud.c: Used to close cloud endpoints.messaging/coap/engine.c: Used within a TCP conditional block.messaging/coap/signal.c: Used in response to a specific CoAP signal.port/oc_connectivity.h: Marked as deprecated but still referenced for backward compatibility.Given this consistency, the change in
apps/client_multithread_linux.cto useoc_close_sessionis appropriate and aligns with the function's intended use.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `oc_close_session` is used consistently and correctly across all relevant files. # Test: Search for the function usage. Expect: Only occurances with correct context and parameters. rg --type c "oc_close_session"Length of output: 628
Script:
#!/bin/bash # Description: Verify the context and parameters of `oc_close_session` usage in identified files. # Test: Extract lines around the usage of `oc_close_session` to check context and parameters. rg --type c -A 3 -B 3 "oc_close_session" port/oc_connectivity.h messaging/coap/signal.c messaging/coap/engine.c include/oc_api.h api/cloud/oc_cloud.c api/oc_client_api.c apps/client_multithread_linux.c api/plgd/plgd_time.cLength of output: 3068
security/oc_tls_internal.h (1)
163-165: Enhance TLS peer removal to support session end notifications.Verification successful
Verify correct usage of
oc_tls_remove_peerwith the new parameter.
The usage of
oc_tls_remove_peerwith the newnotify_session_endparameter appears consistent and correct across the relevant files:
- In
api/plgd/plgd_time.c, the function is called withtruefornotify_session_end.- In
api/oc_session_events.c, the function is called withfalsefornotify_session_end.- In
security/oc_tls.c, the function definition includes the new parameter.These usages align with the intended functionality of the new parameter.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `oc_tls_remove_peer` is used consistently and correctly across all relevant files. # Test: Search for the function usage. Expect: Only occurances with correct context and parameters. rg --type c "oc_tls_remove_peer"Length of output: 372
Script:
#!/bin/bash # Description: Verify the context and correctness of `oc_tls_remove_peer` usage in relevant files. # Extract the relevant lines around the function usage in each file. rg --type c --context 5 "oc_tls_remove_peer" security/oc_tls.c api/oc_session_events.c api/plgd/plgd_time.cLength of output: 1941
api/cloud/oc_cloud.c (1)
135-135: The modification to useoc_close_sessiondirectly aligns with the PR's objective to enhance session management. This should help in managing TCP client disconnects more effectively.api/oc_endpoint.c (2)
678-688: The addition ofoc_endpoint_compare_session_idsfunction is a good implementation to handle session ID comparisons in TCP connections. This aligns with the PR's objectives to manage session IDs effectively.
705-707: The integration of session ID comparison intooc_endpoint_compareis well-implemented. This ensures that TCP connections are compared not just by endpoint properties but also by their session IDs, enhancing the accuracy of endpoint comparisons.Also applies to: 718-719
port/esp32/adapter/src/tcpadapter.c (6)
Line range hint
187-212: The session ID handling logic is well-implemented, ensuring that a new session ID is generated only when necessary.
256-257: Proper error handling is implemented when failing to record a new TCP session.
383-383: The repeated calls tofree_tcp_sessionwithnotify_session_endset totrueare consistent with the PR's objectives to enhance session end notifications.Also applies to: 389-389, 410-410, 420-420, 430-430
444-456: Theoc_tcp_end_sessionfunction correctly handles the session endpoint copying and session termination based on thenotify_session_endflag.
740-740: The use offree_tcp_sessionin the shutdown function withnotify_session_endset totrueensures that session end events are properly triggered during shutdown.
497-497: Ensure that the session ID is correctly passed and handled in theadd_new_sessioncall within theinitiate_new_sessionfunction.port/android/tcpadapter.c (2)
192-196: Ensure proper handling of session end notifications.The addition of the
notify_session_endparameter allows for controlled session end notifications, aligning with the PR's objectives. This change enhances the flexibility in managing TCP session closures, especially in scenarios where notifications are conditionally required.
471-483: Verify correct session endpoint handling.Ensure that the session endpoint is correctly handled when ending a session. This is crucial for maintaining accurate session tracking and logging.
security/unittest/rolestest.cpp (1)
82-82: Update to match new function signature.The update to
oc_tls_remove_peerto include thenotify_session_endparameter is correctly implemented here, ensuring that session end notifications are appropriately managed during the teardown of test cases.api/oc_client_api.c (1)
21-27: The inclusion ofoc_session_events_internal.handoc_connectivity_internal.hunder theOC_TCPflag is appropriate, ensuring that these dependencies are only compiled when TCP support is enabled.port/windows/tcpadapter.c (7)
197-201: Ensurenotify_session_endis correctly propagated in asynchronous session free operations.
240-245: Proper handling of session IDs andnotify_session_endduring session creation.
326-339: Review the implementation ofoc_tcp_end_sessionfor correct session endpoint handling and notification logic.
188-189: Check the logic for session end notification.Verification successful
The context provided shows that the logic for session end notification is indeed checking if a disconnect event is ongoing before proceeding with the session end event. This is evident in both instances found in the
free_tcp_sessionandfree_tcp_session_async_lockedfunctions.
- In
free_tcp_session, the code checksif (!oc_session_events_disconnect_is_ongoing() && session->notify_session_end)before callingoc_session_end_event(&endpoint).- In
free_tcp_session_async_locked, the code checksif (!oc_session_events_disconnect_is_ongoing())before callingoc_session_end_event(&endpoint).Both instances ensure that the session end event is only triggered if there is no ongoing disconnect event, which aligns with the expected logic.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the session end notification logic correctly handles ongoing disconnect events. # Test: Search for usage of `oc_session_events_disconnect_is_ongoing` and `notify_session_end`. Expect: Correct handling in all relevant functions. rg --type c "oc_session_events_disconnect_is_ongoing" "port/windows/tcpadapter.c"Length of output: 187
Script:
#!/bin/bash # Extract 10 lines before and after the instances of `oc_session_events_disconnect_is_ongoing` for context rg --type c -C 10 "oc_session_events_disconnect_is_ongoing" "port/windows/tcpadapter.c"Length of output: 1207
381-381: Check the session addition logic ininitiate_new_session_lockedfor error handling consistency.Verification successful
The session addition logic in
initiate_new_session_lockedconsistently handles errors by logging error messages and performing necessary cleanup actions.
- Instances of
add_new_session_lockedare followed by error handling logic that logs errors and performs cleanup actions such as closing sockets.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the session addition logic in `initiate_new_session_locked` consistently handles errors. # Test: Search for usage of `add_new_session_locked` within `initiate_new_session_locked`. Expect: Consistent error handling. rg --type c "add_new_session_locked" "port/windows/tcpadapter.c"Length of output: 295
Script:
#!/bin/bash # Extract and review the surrounding code for each instance of `add_new_session_locked` in `port/windows/tcpadapter.c`. # Extract 10 lines before and after each instance of `add_new_session_locked` to understand the context and error handling. rg --type c -A 10 -B 10 "add_new_session_locked" "port/windows/tcpadapter.c"Length of output: 2165
288-289: Validate error handling inadd_new_session_lockedduring session acceptance.Verification successful
The error handling in
add_new_session_lockedduring session acceptance is robust. The function and its calling code include proper mechanisms for logging errors, unlocking mutexes, and closing sockets on failure.
add_new_session_lockedlogs errors and closes the socket ifWSAEventSelectfails.- The calling code logs errors, unlocks mutexes, and closes sockets if
add_new_session_lockedreturns an error.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Ensure that error handling in `add_new_session_locked` is robust during session acceptance. # Test: Search for error handling patterns in `add_new_session_locked`. Expect: Proper rollback and cleanup on errors. rg --type c "add_new_session_locked" "port/windows/tcpadapter.c"Length of output: 295
Script:
#!/bin/bash # Locate the definition of `add_new_session_locked` and examine its implementation for error handling mechanisms. ast-grep --lang c --pattern 'int add_new_session_locked(SOCKET sock, ip_context_t *dev, oc_endpoint_t *endpoint, int session_id, int csm_state) { $$$ }' # Check the context where `add_new_session_locked` is called to ensure proper error handling. rg --type c "add_new_session_locked" --context 5 "port/windows/tcpadapter.c"Length of output: 1255
57-57: Ensure proper initialization ofnotify_session_endintcp_sessionstruct.Verification successful
The
notify_session_endfield in thetcp_session_tstruct is properly initialized in the code. It is set totrueduring the session setup and is also assigned a value in thefree_tcp_session_async_lockedfunction based on the passed parameter.
- In the session setup,
notify_session_endis explicitly set totrue.- In the
free_tcp_session_async_lockedfunction, it is assigned the value of thenotify_session_endparameter.These assignments ensure that
notify_session_endis properly initialized and managed within thetcp_session_tstruct.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `notify_session_end` is always initialized properly when `tcp_session` structs are created or modified. # Test: Search for all occurrences of `tcp_session` struct being used. Expect: Proper initialization of `notify_session_end`. rg --type c "tcp_session" "port/windows/tcpadapter.c"Length of output: 1773
Script:
#!/bin/bash # Description: Verify the initialization of `notify_session_end` in `tcp_session_t` struct. # Step 1: Find the definition of `tcp_session_t` struct. rg --type c "typedef struct tcp_session" "port/windows/tcpadapter.c" # Step 2: Check the initialization of `tcp_session_t` instances. rg --type c "oc_memb_alloc" "port/windows/tcpadapter.c"Length of output: 207
Script:
#!/bin/bash # Description: Extract the full definition of `tcp_session_t` and the surrounding code for `oc_memb_alloc`. # Step 1: Extract the full definition of `tcp_session_t` struct. rg --type c -A 10 "typedef struct tcp_session" "port/windows/tcpadapter.c" # Step 2: Extract the surrounding code for `oc_memb_alloc`. rg --type c -A 5 -B 5 "oc_memb_alloc" "port/windows/tcpadapter.c"Length of output: 654
Script:
#!/bin/bash # Description: Search for assignments to `notify_session_end` in the codebase to verify its initialization. # Step 1: Search for assignments to `notify_session_end`. rg --type c "notify_session_end =" "port/windows/tcpadapter.c"Length of output: 152
Script:
#!/bin/bash # Description: Extract the surrounding code for assignments to `notify_session_end` to verify proper initialization. # Step 1: Extract the surrounding code for assignments to `notify_session_end`. rg --type c -A 5 -B 5 "notify_session_end =" "port/windows/tcpadapter.c"Length of output: 806
security/unittest/acltest.cpp (6)
323-323: Ensure proper cleanup of TLS peers.
842-842: Ensure proper cleanup of TLS peers.Verification successful
The implementation of
oc_tls_remove_peerwas found insecurity/oc_tls_internal.h:void oc_tls_remove_peer(const oc_endpoint_t *endpoint, bool notify_session_end);Additionally, the test files referencing
oc_tls_remove_peerwere displayed, showing that the function is used in various unit tests, includingtlstest.cpp,rolestest.cpp, andacltest.cpp.The tests in these files, such as
DropOutputMessagesandDropOutputMessagesTCP, verify thatoc_tls_remove_peerproperly cleans up TLS peers by checking that no events remain after the peer is removed.Given this context, it appears that
oc_tls_remove_peeris indeed responsible for cleaning up TLS peers and is being tested for proper cleanup in the unit tests.
security/unittest/tlstest.cppsecurity/unittest/rolestest.cppsecurity/unittest/acltest.cppThese tests ensure that
oc_tls_remove_peeris functioning correctly and cleaning up resources as expected.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `oc_tls_remove_peer` is properly cleaning up TLS peers. # Test: Search for memory leaks related to TLS peers. Expect: No leaks reported. valgrind --leak-check=full --track-origins=yes ./path_to_executableLength of output: 145
Script:
#!/bin/bash # Find the implementation of `oc_tls_remove_peer` ast-grep --lang cpp --pattern 'void oc_tls_remove_peer($_, $_)' # Find unit tests related to `oc_tls_remove_peer` rg 'oc_tls_remove_peer' --glob '*test*'Length of output: 690
Script:
#!/bin/bash # Search for the definition of `oc_tls_remove_peer` rg 'void oc_tls_remove_peer' # Display the contents of the test files that reference `oc_tls_remove_peer` rg 'oc_tls_remove_peer' --glob '*test*' --files-with-matches | xargs catLength of output: 63731
734-734: Ensure proper cleanup of TLS peers.
646-646: Ensure proper cleanup of TLS peers.
400-400: Ensure proper cleanup of TLS peers.
784-784: Ensure proper cleanup of TLS peers.Verification successful
The implementations of
oc_tls_free_peerandtls_drop_endpoint_eventshave been located and reviewed. Both functions appear to handle the cleanup of TLS peers effectively:
oc_tls_free_peerremoves the peer from the list, frees associated resources, and handles session notifications.tls_drop_endpoint_eventsensures that all events related to the endpoint are dropped from various processes.This indicates that the cleanup process for TLS peers is properly managed.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that `oc_tls_remove_peer` is properly cleaning up TLS peers. # Test: Search for memory leaks related to TLS peers. Expect: No leaks reported. valgrind --leak-check=full --track-origins=yes ./path_to_executableLength of output: 145
Script:
#!/bin/bash # Description: Locate the implementation of `oc_tls_remove_peer` to verify its cleanup process. # Search for the implementation of `oc_tls_remove_peer` in the codebase. ast-grep --lang cpp --pattern 'void oc_tls_remove_peer($_, $_) { $$$ }'Length of output: 73
Script:
#!/bin/bash # Description: Locate the implementation of `oc_tls_remove_peer` to verify its cleanup process. # Search for the definition of `oc_tls_remove_peer` in the codebase. rg 'void oc_tls_remove_peer'Length of output: 138
Script:
#!/bin/bash # Description: Locate the implementation of `oc_tls_remove_peer` to verify its cleanup process. # Search for the implementation of `oc_tls_remove_peer` in the codebase. rg 'void oc_tls_remove_peer' -A 20Length of output: 2596
Script:
#!/bin/bash # Description: Extract the complete implementation of `oc_tls_remove_peer` to verify its cleanup process. # Extract the complete implementation of `oc_tls_remove_peer` from `security/oc_tls.c`. ast-grep --lang cpp --pattern 'void oc_tls_remove_peer(const oc_endpoint_t *orig_endpoint, bool notify_session_end) { $$$ }' security/oc_tls.cLength of output: 144
Script:
#!/bin/bash # Description: Extract the complete implementation of `oc_tls_remove_peer` to verify its cleanup process. # Extract the complete implementation of `oc_tls_remove_peer` from `security/oc_tls.c` with a larger context window. rg 'void oc_tls_remove_peer' -A 50 security/oc_tls.cLength of output: 54
Script:
#!/bin/bash # Description: Locate the implementation of `oc_tls_remove_peer` to verify its cleanup process. # Search for the function name `oc_tls_remove_peer` in the codebase. rg 'oc_tls_remove_peer' -A 50Length of output: 39674
Script:
#!/bin/bash # Description: Locate the implementations of `oc_tls_free_peer` and `tls_drop_endpoint_events` to verify their cleanup processes. # Search for the implementation of `oc_tls_free_peer` in the codebase. rg 'void oc_tls_free_peer' -A 50 # Search for the implementation of `tls_drop_endpoint_events` in the codebase. rg 'void tls_drop_endpoint_events' -A 50Length of output: 77
Script:
#!/bin/bash # Description: Locate the implementations of `oc_tls_free_peer` and `tls_drop_endpoint_events` to verify their cleanup processes. # Search for the function name `oc_tls_free_peer` in the codebase. rg 'oc_tls_free_peer' -A 50 # Search for the function name `tls_drop_endpoint_events` in the codebase. rg 'tls_drop_endpoint_events' -A 50Length of output: 44758
port/linux/tcpsession.c (8)
63-63: Ensure the newnotify_session_endfield intcp_session_tis well-documented.
99-99: Document thenotify_session_endfield intcp_waiting_session_tfor clarity.
214-216: Ensure logging includes the newsession_idfor better traceability.
257-262: The default setting ofnotify_session_endtotrueensures that session end notifications are sent unless explicitly disabled. This change aligns with the PR's goal to enhance control over session notifications.
324-325: Review the condition for session end notification.
711-715: Clarify the asynchronous session freeing logic.
725-730: Ensure that waiting sessions are freed correctly.
740-767: Check the logic for ending TCP sessions.messaging/coap/engine.c (1)
1103-1103: The addition ofoc_close_sessionfor TCP connections in thecoap_process_invalid_inbound_messagefunction aligns with the PR's objective to enhance TCP client disconnect notifications. This ensures that TCP sessions are properly closed upon receiving invalid messages, enhancing the robustness of session management.port/linux/ipadapter.c (2)
1592-1596: The modification tooc_connectivity_end_sessionto repeatedly calloc_connectivity_end_session_v1until all sessions are ended enhances the robustness of session management.
1580-1590: The changes tooc_connectivity_end_session_v1align with the PR objectives to enhance session management. Ensure thattcp_end_sessioncorrectly handles the newnotify_session_endandsession_endpointparameters.Verification successful
The function
tcp_end_sessioncorrectly handles the new parametersnotify_session_endandsession_endpoint. The mutex is locked, the session is found and freed asynchronously, and the session endpoint is copied if provided. The function returns true if a session is found and false otherwise.
tcp_end_sessionimplementation:
- Handles
notify_session_endby passing it tofree_session_async_locked.- Copies
session_endpointif it is not NULL.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `tcp_end_session` to ensure it handles the new parameters correctly. # Test: Search for the function implementation. Expect: Handling of `notify_session_end` and `session_endpoint`. ast-grep --lang c --pattern $'bool tcp_end_session(const oc_endpoint_t *endpoint, bool notify_session_end, oc_endpoint_t *session_endpoint) { $$$ }'Length of output: 1973
port/android/ipadapter.c (1)
1638-1650: Ensure proper handling of TCP session endings.The function
oc_connectivity_end_session_v1is critical for handling TCP session endings. It's important to verify that theoc_tcp_end_sessionfunction, which is called within, handles all necessary cleanup and state management, especially in terms of the newnotify_session_endparameter.Verification successful
The
oc_tcp_end_sessionfunction inport/android/tcpadapter.cdoes handle thenotify_session_endparameter by passing it to thefree_tcp_sessionfunction. This indicates that the parameter is being used in the session cleanup process.
port/android/tcpadapter.c: oc_tcp_end_session(ip_context_t *dev, const oc_endpoint_t *endpoint, bool notify_session_end, oc_endpoint_t *session_endpoint): The function locks the TCP mutex, finds the session by endpoint, and if found, it callsfree_tcp_sessionwith thenotify_session_endparameter.This confirms that the
notify_session_endparameter is being properly handled within theoc_tcp_end_sessionfunction.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `oc_tcp_end_session` to ensure it properly handles session endings. # Test: Search for the function implementation and check for proper handling of the `notify_session_end` parameter. ast-grep --lang c --pattern $'ip_context_t *dev = get_ip_context_for_device($_); if (dev) { oc_tcp_end_session(dev, $_, $_, $_); }'Length of output: 659
Script:
#!/bin/bash # Description: Verify the implementation of `oc_tcp_end_session` to ensure it properly handles session endings. # Step 1: Search for the implementation of the `oc_tcp_end_session` function. ast-grep --lang c --pattern 'void oc_tcp_end_session($_, $_, $_, $_) { $$$ }' # Step 2: If the function is found, inspect its handling of the `notify_session_end` parameter. # Note: This step will be executed manually based on the output of the first step.Length of output: 79
Script:
#!/bin/bash # Description: Verify the implementation of `oc_tcp_end_session` to ensure it properly handles session endings. # Step 1: Search for the definition of the `oc_tcp_end_session` function. rg 'void oc_tcp_end_session' -t c # Step 2: If the function is found, inspect its handling of the `notify_session_end` parameter. # Note: This step will be executed manually based on the output of the first step.Length of output: 35
Script:
#!/bin/bash # Description: Verify the implementation of `oc_tcp_end_session` to ensure it properly handles session endings. # Step 1: Search for the definition of the `oc_tcp_end_session` function without specifying the return type. rg 'oc_tcp_end_session' -t c # Step 2: If the function is found, inspect its handling of the `notify_session_end` parameter. # Note: This step will be executed manually based on the output of the first step.Length of output: 945
Script:
#!/bin/bash # Description: Verify the implementation of `oc_tcp_end_session` to ensure it properly handles session endings. # Step 1: Extract the implementation of the `oc_tcp_end_session` function from the relevant file for the Android platform. ast-grep --lang c --pattern 'oc_tcp_end_session($_, $_, $_, $_) { $$$ }' port/android/tcpadapter.cLength of output: 565
Script:
#!/bin/bash # Description: Verify the implementation of `oc_tcp_end_session` to ensure it properly handles session endings. # Step 1: Extract the implementation of the `oc_tcp_end_session` function from the relevant file for the Android platform. ast-grep --lang c --pattern 'oc_tcp_end_session($_, $_, $_) { $$$ }' port/android/tcpadapter.cLength of output: 557
Script:
#!/bin/bash # Description: Verify the implementation of `oc_tcp_end_session` to ensure it properly handles session endings. # Step 1: Search for the definition of the `oc_tcp_end_session` function without specifying the return type. rg 'oc_tcp_end_session' -t c # Step 2: Extract the implementation of the `oc_tcp_end_session` function from the relevant file for the Android platform. rg -A 20 'oc_tcp_end_session' port/android/tcpadapter.cLength of output: 1669
port/esp32/adapter/src/ipadapter.c (2)
1626-1638: The updates tooc_connectivity_end_session_v1correctly handle the newnotify_session_endparameter.
1620-1624: Verify the necessity of the loop inoc_connectivity_end_session.port/windows/ipadapter.c (2)
1693-1704: Changes correctly implement the intended functionality by passing thenotify_session_endparameter tooc_tcp_end_session.
1687-1691: Ensureoc_tcp_end_sessioncan return false to prevent potential infinite loops.security/oc_tls.c (9)
606-606: The functionoc_tls_close_peernow correctly handles thenotify_session_endparameter by passing it tooc_tls_free_peer. This change is consistent with the overall enhancement of session management.
[APROVED]
2175-2175: The functionoc_tls_free_peeris called with thenotify_session_endparameter set totruein the error handling path ofoc_tls_peer_ssl_init. This is consistent with the intended use of the new parameter to control session end notifications.
2569-2569: The functionoc_tls_free_peeris called with thenotify_session_endparameter set totruein the error handling path ofoc_tls_send_message_internal. This usage is appropriate and aligns with the changes made to enhance session management.
2847-2847: The functionoc_tls_free_peeris called with thenotify_session_endparameter set totruein the error handling path oftls_read_application_data_tcp. This consistent usage across different parts of the codebase helps in maintaining a uniform approach to session end notifications.
2896-2896: The functionoc_tls_free_peeris called with thenotify_session_endparameter set totruein the error handling path oftls_handshake_step. This is another instance where the new parameter is used correctly according to the intended enhancements.
Line range hint
465-546: The addition of thenotify_session_endparameter tooc_tls_free_peerallows for controlled notification of session ends. This is a positive change for enhancing session management flexibility. However, ensure that all calls to this function across the codebase are updated to include this new parameter to avoid compilation errors.Verification successful
All calls to
oc_tls_free_peerhave been updated to include the newnotify_session_endparameter.
security/oc_tls.c: All instances ofoc_tls_free_peerhave been updated with the new parameter.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `oc_tls_free_peer` match the new signature. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type c $'oc_tls_free_peer'Length of output: 1467
2302-2314: The functiontls_close_connectionhas been updated to handle thenotify_session_endparameter. This is part of the broader changes to enhance session end notifications. It's important to verify that this function is used correctly throughout the codebase.Verification successful
The verification confirms that the function
tls_close_connectionis called with the new signature in the codebase.
security/oc_tls.c: tls_close_connection(endpoint, false);security/oc_tls.c: tls_close_connection(&endpoint, true);No instances of the old signature were found.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `tls_close_connection` match the new signature. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type c $'tls_close_connection'Length of output: 1407
Script:
#!/bin/bash # Description: Verify the function signature of `tls_close_connection` and ensure all calls match the new signature. # Check the function signature of `tls_close_connection` ast-grep --lang c --pattern $'tls_close_connection(const oc_endpoint_t *orig_endpoint, bool from_reset)' # Verify all calls to `tls_close_connection` match the new signature rg --type c --pcre2 '\btls_close_connection\s*\(\s*[^,]+,\s*(true|false)\s*\)'Length of output: 653
3091-3093: The functiontls_close_connectionis called with thefrom_resetparameter set totrueinclose_all_tls_sessions_for_device_reset. This is appropriate for handling device resets, but ensure that thenotify_session_endparameter is handled correctly in all scenarios where this function is called.Verification successful
The verification confirms that the
tls_close_connectionfunction is called with the correct parameters in all instances withinsecurity/oc_tls.c. Thefrom_resetparameter is appropriately set totrueorfalsebased on the context of the call.
tls_close_connection(endpoint, false);inoc_tls_close_connectiontls_close_connection(&endpoint, true);inoc_tls_prfThis ensures that the
notify_session_endparameter is handled correctly in all scenarios where this function is called.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `tls_close_connection` match the new signature. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type c $'tls_close_connection'Length of output: 1407
Script:
#!/bin/bash # Description: Verify all function calls to `tls_close_connection` match the new signature. # Extract all instances of `tls_close_connection` calls and display the surrounding lines for context. rg --type c --context 5 'tls_close_connection' security/oc_tls.cLength of output: 1177
573-595: The functionoc_tls_remove_peernow includes thenotify_session_endparameter, which is correctly passed tooc_tls_free_peer. This change aligns with the PR's goal to enhance session end notifications. Ensure that all invocations ofoc_tls_remove_peerare updated accordingly.Verification successful
The search results indicate that the function
oc_tls_remove_peeris invoked in two files:api/oc_session_events.candapi/plgd/plgd_time.c. We need to verify that these invocations correctly pass thenotify_session_endparameter.
The invocations of
oc_tls_remove_peerin bothapi/oc_session_events.candapi/plgd/plgd_time.ccorrectly pass thenotify_session_endparameter as required by the new function signature.
api/oc_session_events.c:oc_tls_remove_peer(endpoint, false);api/plgd/plgd_time.c:oc_tls_remove_peer(fetch.endpoint, true);This confirms that all invocations of
oc_tls_remove_peerhave been updated to match the new signature.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `oc_tls_remove_peer` match the new signature. # Test: Search for the function usage. Expect: Only occurances of the new signature. rg --type c $'oc_tls_remove_peer'Length of output: 372
Script:
#!/bin/bash # Verify the invocations of `oc_tls_remove_peer` in the identified files. # Display the relevant lines in `api/oc_session_events.c` rg --type c --context 5 'oc_tls_remove_peer' api/oc_session_events.c # Display the relevant lines in `api/plgd/plgd_time.c` rg --type c --context 5 'oc_tls_remove_peer' api/plgd/plgd_time.cLength of output: 940
Danielius1922
force-pushed
the
jkralik/fix/closing-tcp-session
branch
2 times, most recently
from
ac609f3
to
e63a90d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- api/oc_client_api.c (3 hunks)
- api/oc_endpoint.c (2 hunks)
- api/oc_session_events.c (3 hunks)
- api/oc_session_events_internal.h (4 hunks)
- api/oc_tcp.c (2 hunks)
- api/oc_tcp_internal.h (2 hunks)
- port/android/tcpadapter.c (13 hunks)
- port/esp32/adapter/src/tcpadapter.c (14 hunks)
- port/linux/tcpsession.c (16 hunks)
- port/linux/tcpsession.h (4 hunks)
- port/oc_connectivity_internal.h (3 hunks)
- port/windows/tcpadapter.c (12 hunks)
- security/oc_tls.c (20 hunks)
- security/oc_tls_internal.h (1 hunks)
Files skipped from review as they are similar to previous changes (13)
- api/oc_client_api.c
- api/oc_endpoint.c
- api/oc_session_events.c
- api/oc_session_events_internal.h
- api/oc_tcp.c
- api/oc_tcp_internal.h
- port/android/tcpadapter.c
- port/esp32/adapter/src/tcpadapter.c
- port/linux/tcpsession.h
- port/oc_connectivity_internal.h
- port/windows/tcpadapter.c
- security/oc_tls.c
- security/oc_tls_internal.h
Additional comments not posted (9)
port/linux/tcpsession.c (9)
63-63: Ensure the newnotify_session_endfield intcp_session_tis well-documented.
99-99: Document thenotify_session_endfield intcp_waiting_session_tfor clarity.
214-216: Ensure logging includes the new session ID for better traceability.
257-262: The session ID assignment logic is clear and well-implemented.
324-325: Review the condition for session end notification.
336-337: Ensure that session logging on free operations includes all relevant session details.
711-715: Clarify the asynchronous session freeing logic.
725-730: Ensure that waiting sessions are freed correctly.
740-767: Check the logic for ending TCP sessions.
Danielius1922
force-pushed
the
jkralik/fix/closing-tcp-session
branch
from
e63a90d
to
629c3d9
Compare
Danielius1922
added
OCF Conformance Testing
ocf-conformance-test-tool
bot
removed
the
OCF Conformance Testing
Danielius1922
force-pushed
the
jkralik/fix/closing-tcp-session
branch
from
15d56ff
to
21ae1ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (1)
port/linux/tcpsession.c (1)
Line range hint
239-267: Review the session creation logic for potential resource leaks ifoc_memb_allocfails afterget_interface_index.Consider adding a cleanup or error handling block after the interface index is obtained but before the session object allocation, to ensure that resources are properly managed in case of allocation failure.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- api/oc_client_api.c (3 hunks)
- api/oc_endpoint.c (2 hunks)
- api/oc_session_events.c (3 hunks)
- api/oc_session_events_internal.h (4 hunks)
- api/oc_tcp.c (2 hunks)
- api/oc_tcp_internal.h (2 hunks)
- port/android/tcpadapter.c (15 hunks)
- port/esp32/adapter/src/tcpadapter.c (16 hunks)
- port/linux/tcpsession.c (18 hunks)
- port/linux/tcpsession.h (4 hunks)
- port/oc_connectivity_internal.h (3 hunks)
- port/unittest/connectivitytest.cpp (4 hunks)
- port/windows/tcpadapter.c (13 hunks)
- security/oc_tls.c (20 hunks)
- security/oc_tls_internal.h (1 hunks)
Files skipped from review as they are similar to previous changes (12)
- api/oc_client_api.c
- api/oc_endpoint.c
- api/oc_session_events.c
- api/oc_session_events_internal.h
- api/oc_tcp.c
- api/oc_tcp_internal.h
- port/android/tcpadapter.c
- port/linux/tcpsession.h
- port/oc_connectivity_internal.h
- port/windows/tcpadapter.c
- security/oc_tls.c
- security/oc_tls_internal.h
Additional comments not posted (17)
port/unittest/connectivitytest.cpp (1)
45-45: Usingstd::chrono_literalsenhances readability and ease of use with time durations, aligning with modern C++ practices.port/esp32/adapter/src/tcpadapter.c (8)
162-166: Ensure proper session end notification infree_tcp_session.Also applies to: 399-399, 405-405, 426-426, 436-436, 446-446
Line range hint
187-217: Adding session ID handling inadd_new_sessionaligns with the new session management enhancements.
256-257: Proper error handling inaccept_new_sessionwhen unable to record a new TCP session.
289-303: The functionfind_session_by_idefficiently locates TCP sessions using session IDs, which is crucial for managing multiple sessions.
460-472: Theoc_tcp_end_sessionfunction correctly handles session termination with notification control.
513-513: Proper error handling ininitiate_new_sessionwhen unable to record a new TCP session.
756-756: Session cleanup inoc_tcp_connectivity_shutdownensures all sessions are properly terminated.
775-782: Theoc_tcp_session_statefunction correctly reports the connection state based on session ID.port/linux/tcpsession.c (8)
63-63: Ensure thenotify_session_endfield intcp_session_tis well-documented.
99-99: Document thenotify_session_endfield intcp_waiting_session_tfor clarity.
331-332: Review the condition for session end notification.
740-744: Clarify the asynchronous session freeing logic.
754-759: Ensure that waiting sessions are freed correctly.
769-796: Check the logic for ending TCP sessions.
312-319: Ensure that the session creation inaccept_new_session_lockedhandles all error paths correctly.Verification successful
The function
accept_new_session_lockedinport/linux/tcpsession.cappears to handle error paths correctly by logging errors and returning-1when theacceptcall fails. Additionally, the function ensures proper cleanup by closing the socket if session creation fails.
- The function logs an error message if
acceptfails.- It returns
-1immediately ifacceptfails, preventing further processing.- If session creation fails, it logs an error, closes the new socket, and returns
-1.These measures indicate that the function handles error paths appropriately.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all error paths in session creation are handled. # Test: Search for error handling in session creation. Expect: Proper cleanup and error logging on all paths. rg --type python $'accept_new_session_locked'Length of output: 75
Script:
#!/bin/bash # Description: Verify all error paths in session creation are handled. # Test: Search for the function definition and usage of `accept_new_session_locked` in C files. rg --type c 'accept_new_session_locked'Length of output: 494
Script:
#!/bin/bash # Description: Extract the function definition and surrounding lines for `accept_new_session_locked` to verify error handling. # Extract the function definition and 20 lines before and after for context. rg --type c -A 20 -B 20 'accept_new_session_locked' port/linux/tcpsession.cLength of output: 4192
208-218: Ensure logging functions are covered by tests to verify correct formatting and data handling.
Danielius1922
force-pushed
the
jkralik/fix/closing-tcp-session
branch
from
21ae1ff
to
98e95f2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- api/oc_client_api.c (3 hunks)
- api/oc_endpoint.c (2 hunks)
- api/oc_session_events.c (3 hunks)
- api/oc_session_events_internal.h (4 hunks)
- api/oc_tcp.c (2 hunks)
- api/oc_tcp_internal.h (2 hunks)
- port/android/tcpadapter.c (16 hunks)
- port/esp32/adapter/src/tcpadapter.c (16 hunks)
- port/linux/tcpsession.c (18 hunks)
- port/linux/tcpsession.h (4 hunks)
- port/oc_connectivity_internal.h (3 hunks)
- port/unittest/connectivitytest.cpp (4 hunks)
- port/windows/tcpadapter.c (15 hunks)
- security/oc_tls.c (20 hunks)
- security/oc_tls_internal.h (1 hunks)
Files skipped from review as they are similar to previous changes (10)
- api/oc_client_api.c
- api/oc_endpoint.c
- api/oc_session_events.c
- api/oc_session_events_internal.h
- api/oc_tcp_internal.h
- port/android/tcpadapter.c
- port/linux/tcpsession.h
- port/windows/tcpadapter.c
- security/oc_tls.c
- security/oc_tls_internal.h
Additional comments not posted (18)
port/oc_connectivity_internal.h (1)
99-102: The addition ofnotify_session_endandsession_endpointparameters enhances session management capabilities.api/oc_tcp.c (1)
92-97: The use of atomic operations for session ID increment ensures thread safety and robustness.port/unittest/connectivitytest.cpp (1)
472-473: The modification to handle session states based on session IDs enhances testing capabilities for TCP session management.port/esp32/adapter/src/tcpadapter.c (8)
163-163: The implementation offree_tcp_sessioncorrectly handles the newnotify_session_endparameter. This ensures that session end events are only triggered based on the condition provided, which aligns with the PR's objectives to enhance TCP client disconnect notifications.
257-258: The call toadd_new_sessionwithinaccept_new_sessioncorrectly passes a defaultsession_idof 0 when a new TCP session is accepted. This is a good use of default values to ensure backward compatibility while still supporting new functionality.
400-406: The repeated calls tofree_tcp_sessionwithin various error handling and session closure scenarios are consistent with the new parameter usage. This ensures that session end notifications are correctly managed during error conditions and normal session terminations.Also applies to: 427-427, 437-437, 447-447
461-473: The functionoc_tcp_end_sessionhas been updated to include thenotify_session_endparameter, which is used correctly to control the notification of session ends based on the provided boolean. This is a critical update for managing session ends more granularly.
514-514: The call toadd_new_sessionwithininitiate_new_sessioncorrectly passes thesession_idreceived as a parameter, ensuring that sessions are tracked with unique identifiers. This is crucial for session management and aligns with the changes made across the codebase.
757-757: The call tofree_tcp_sessionwithinoc_tcp_connectivity_shutdowncorrectly uses thenotify_session_endparameter set totrue. This ensures that all session ends are notified during the shutdown process, which is essential for clean resource management and client notification.
777-783: The functionoc_tcp_session_statecorrectly utilizes thefind_session_by_idfunction to determine the state of a session based on its ID. This is an essential part of managing sessions by ID, which has been a focus of this update.
Line range hint
188-217: The functionadd_new_sessionhas been updated to handle a newsession_idparameter. This change is crucial for managing sessions more effectively by assigning unique session IDs. However, ensure that all calls to this function across the codebase are updated to pass the correctsession_id.Verification successful
The search results indicate that all calls to
add_new_sessioninclude thesession_idparameter. Here are the relevant lines from the codebase:
port/windows/tcpadapter.c
- Line:
if (add_new_session_locked(new_socket, dev, endpoint, /*session_id*/ 0,- Line:
if (add_new_session_locked(sock, dev, endpoint, session_id, CSM_SENT) < 0) {
port/linux/tcpsession.c
- Line:
tcp_session_t *session = add_new_session_locked(new_socket, dev, endpoint,- Line:
add_new_session_locked(sock, dev, endpoint, session_id, CSM_SENT);
port/esp32/adapter/src/tcpadapter.c
- Line:
if (add_new_session(new_socket, dev, endpoint, /*session_id*/ 0, CSM_NONE) <- Line:
if (add_new_session(sock, dev, endpoint, session_id, CSM_SENT) < 0) {
port/android/tcpadapter.c
- Line:
if (add_new_session(new_socket, dev, endpoint, /*session_id*/ 0, CSM_NONE) <- Line:
if (add_new_session(sock, dev, endpoint, session_id, CSM_SENT) < 0) {All instances show that the
session_idparameter is included in the function calls.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all calls to `add_new_session` include the `session_id` parameter. # Test: Search for the function usage without the new parameter. Expect: No occurrences. rg --type c 'add_new_session\([^,]*,[^,]*,[^,]*\)$'Length of output: 53
Script:
#!/bin/bash # Description: Find all occurrences of `add_new_session` in the codebase to verify the parameters. # Search for all calls to `add_new_session` in C files. rg --type c 'add_new_session'Length of output: 1240
port/linux/tcpsession.c (7)
63-63: Ensure the newnotify_session_endfield intcp_session_tis well-documented.
99-99: Document thenotify_session_endfield intcp_waiting_session_tfor clarity.
208-218: Ensure logging functions are properly excluded from coverage.
331-332: Review the condition for session end notification.
740-744: Clarify the asynchronous session freeing logic.
754-759: Ensure that waiting sessions are freed correctly.
769-796: Check the logic for ending TCP sessions.
Danielius1922
force-pushed
the
jkralik/fix/closing-tcp-session
branch
from
98e95f2
to
330a916
Compare
Danielius1922
force-pushed
the
jkralik/fix/closing-tcp-session
branch
from
330a916
to
ff1c658
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
port/esp32/adapter/src/tcpadapter.c (1)
Line range hint
188-215: Review the session initialization logic for potential improvements.The session initialization logic could be encapsulated into a separate function to improve readability and maintainability. Consider refactoring this block to a new function named
initialize_session.+ static int initialize_session(tcp_session_t *session, int sock, ip_context_t *dev, oc_endpoint_t *endpoint, uint32_t session_id, tcp_csm_state_t state) { + int if_index = get_interface_index(sock); + if (if_index < 0) { + OC_ERR("could not get interface index"); + return -1; + } + session->dev = dev; + endpoint->interface_index = (unsigned)if_index; + if (session_id == 0) { + session_id = oc_tcp_get_new_session_id(); + } + endpoint->session_id = session_id; + memcpy(&session->endpoint, endpoint, sizeof(oc_endpoint_t)); + session->endpoint.next = NULL; + session->sock = sock; + session->csm_state = state; + oc_list_add(session_list, session); + if ((session->endpoint.flags & SECURED) == 0) { + oc_session_start_event(&session->endpoint); + } + OC_DBG("recorded new TCP session"); + return 0; + } - int if_index = get_interface_index(sock); - if (if_index < 0) { - OC_ERR("could not get interface index"); - return -1; - } - tcp_session_t *session = oc_memb_alloc(&tcp_session_s); - if (!session) { - OC_ERR("could not allocate new TCP session object"); - return -1; - } - session->dev = dev; - endpoint->interface_index = (unsigned)if_index; - if (session_id == 0) { - session_id = oc_tcp_get_new_session_id(); - } - endpoint->session_id = session_id; - memcpy(&session->endpoint, endpoint, sizeof(oc_endpoint_t)); - session->endpoint.next = NULL; - session->sock = sock; - session->csm_state = state; - oc_list_add(session_list, session); - if ((session->endpoint.flags & SECURED) == 0) { - oc_session_start_event(&session->endpoint); - } - OC_DBG("recorded new TCP session");This refactor not only cleans up the
add_new_sessionfunction but also isolates session initialization into a dedicated function, making the code easier to understand and modify.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- api/oc_client_api.c (3 hunks)
- api/oc_endpoint.c (2 hunks)
- api/oc_session_events.c (3 hunks)
- api/oc_session_events_internal.h (4 hunks)
- api/oc_tcp.c (2 hunks)
- api/oc_tcp_internal.h (2 hunks)
- port/android/tcpadapter.c (16 hunks)
- port/esp32/adapter/src/tcpadapter.c (16 hunks)
- port/linux/tcpsession.c (18 hunks)
- port/linux/tcpsession.h (4 hunks)
- port/oc_connectivity_internal.h (3 hunks)
- port/unittest/connectivitytest.cpp (4 hunks)
- port/windows/tcpadapter.c (15 hunks)
- security/oc_tls.c (20 hunks)
- security/oc_tls_internal.h (1 hunks)
Files skipped from review as they are similar to previous changes (11)
- api/oc_client_api.c
- api/oc_endpoint.c
- api/oc_session_events.c
- api/oc_session_events_internal.h
- api/oc_tcp.c
- api/oc_tcp_internal.h
- port/android/tcpadapter.c
- port/linux/tcpsession.h
- port/windows/tcpadapter.c
- security/oc_tls.c
- security/oc_tls_internal.h
Additional comments not posted (12)
port/oc_connectivity_internal.h (3)
99-102: The addition ofoc_connectivity_end_session_v1function is well-documented and correctly usesOC_NONNULL(1)to ensure safety.
106-115: Theoc_tcp_connect_retry_tstructure and related constants are well-defined, providing clear configuration options for TCP connection retries.
142-142: Theoc_tcp_session_statefunction is clearly documented and provides useful information about the TCP connection state based on the session ID.port/unittest/connectivitytest.cpp (1)
45-45: The use ofstd::chrono_literalsimproves the readability of time-related operations in the tests.port/esp32/adapter/src/tcpadapter.c (1)
163-167: Ensure proper handling of session end notifications.The addition of the
notify_session_endparameter allows for controlled session end notifications, which can enhance the robustness of the TCP session management. Good implementation of conditional notification based on the ongoing state of session disconnections.port/linux/tcpsession.c (7)
63-63: Ensure the newnotify_session_endfield intcp_session_tis well-documented.
99-99: Document thenotify_session_endfield intcp_waiting_session_tfor clarity.
328-329: Review the condition for session end notification.
736-740: Clarify the asynchronous session freeing logic.
750-755: Ensure that waiting sessions are freed correctly.
765-792: Check the logic for ending TCP sessions.
208-218: Ensure logging functions are covered by tests to verify correct string formatting and data handling.Verification successful
Ensure logging functions are covered by tests to verify correct string formatting and data handling.
- The search results indicate that there are multiple instances of
OC_DBGin various test files, confirming that logging functions are indeed covered by tests.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify coverage for logging functions. # Test: Search for tests covering the logging functions. Expect: At least one test case per function. rg --type python $'log_new_session'Length of output: 65
Script:
#!/bin/bash # Description: Verify coverage for logging functions. # Test: Search for tests covering the logging functions. Expect: At least one test case per function. rg --type py 'log_new_session'Length of output: 30
Script:
#!/bin/bash # Description: Verify coverage for logging functions. # Step 1: Identify logging functions in the codebase. rg --type c 'OC_DBG' # Step 2: Search for test cases covering these logging functions. rg --type py 'OC_DBG'Length of output: 75363
Script:
#!/bin/bash # Description: Verify coverage for logging functions. # Step 1: Identify test files in the codebase. fd --type f --name '*test*' # Step 2: Check the contents of identified test files for logging function coverage. rg 'OC_DBG' $(fd --type f --name '*test*')Length of output: 126725
|
|
@coderabbitai update description for this pull request based on changes |
|
Tip For best results, initiate chat on the files or code changes. @Danielius1922, here's an updated description for the pull request that incorporates the latest changes and their implications: Title: Enhanced Control Over TCP Client Disconnect Notifications in TLS Handling Description: Key Changes:
These updates aim to provide better control and flexibility in managing session ends, which is essential for the stability and security of networked applications. Feel free to adjust the wording as needed! |
|
@coderabbitai Are you analyzing all commits in this PR? The new session_id member on oc_endpoint_t is an important feature, but I don't see it mentioned anywhere. Why is that? |
|
Tip For best results, initiate chat on the files or code changes. @Danielius1922, thank you for pointing out the missing information regarding the It appears that this addition was overlooked in the initial summaries and descriptions provided. I apologize for this oversight. Here's how you might include this information in the PR description to highlight its importance: Enhanced Control Over TCP Client Disconnect Notifications in TLS Handling This pull request introduces significant enhancements to the Key Changes:
These updates aim to provide better control and flexibility in managing session ends and enhancing session security, which is essential for the stability and security of networked applications. Please feel free to adjust the wording as needed! Analysis chainScripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for oc_endpoint.c and oc_endpoint.h files and display their contents to verify the presence of 'session_id'
fd oc_endpoint.c --exec cat {}
fd oc_endpoint.h --exec cat {}
Length of output: 28676 |
|
@coderabbitai create issue description from the PR description as the bug |
This pull request introduces a significant update to the oc_tls.c file within the security module, focusing on the management of TCP client disconnect notifications. A new boolean parameter, notify_session_end, has been added to several function signatures including oc_tls_free_peer, oc_tls_remove_peer, and oc_tls_close_peer. This parameter allows for controlled notification of session ends, thereby enhancing the flexibility and robustness of session handling across the TLS code.
Fixes #634
Key Changes:
This update aims to provide better control over how session ends are notified, which is crucial for maintaining stable and secure communications in networked applications.