Add debug conditions to ACE_Ping_Socket #2332
Conversation
WalkthroughThe changes introduce conditional debug logging in the Changes
Sequence Diagram(s)sequenceDiagram
participant PS as Ping_Socket
participant D as ACE::debug()
participant L as Logging System
PS->>D: Check if debugging is enabled?
alt Debugging Enabled
D-->>PS: true
PS->>L: Log debug message (constructor / process_incoming_dgram / make_echo_check)
else Debugging Disabled
D-->>PS: false
PS-->>L: Skip logging call
end
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ACE/ace/Ping_Socket.cpp (2)
273-273: Fix typo in class name.There's a typo in the class name - "ACE::Ping_Socket" should be "ACE_Ping_Socket" (underscore instead of double colon).
- ACE_TEXT ("(%P|%t) ACE::Ping_Socket::process_incoming_dgram - ") + ACE_TEXT ("(%P|%t) ACE_Ping_Socket::process_incoming_dgram - ")Also applies to: 285-285
367-371: Improve debug message accuracy.The debug message uses
rval_sendwhich is just the return value (-1 for error, 0 for success) from send_echo_check. For better debugging, it would be more helpful to show the actual number of bytes sent (length_icmp).- ACE_TEXT ("(%P|%t) ACE_Ping_Socket::make_echo_check - sent %d.\n"), - rval_send)); + ACE_TEXT ("(%P|%t) ACE_Ping_Socket::make_echo_check - sent %d bytes.\n"), + ICMP_MIN + ICMP_DATA_LENGTH));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ACE/ace/Ping_Socket.cpp(6 hunks)
🧰 Additional context used
🪛 Cppcheck (2.10-2)
ACE/ace/Ping_Socket.cpp
[error] 230-230: There is an unknown macro here somewhere. Configuration is required. If ACE_TEXT is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: ubuntu-20.04 clang++-11
- GitHub Check: ubuntu-20.04 clang++-10
- GitHub Check: VS2022Release64
- GitHub Check: VS2022Debug64
- GitHub Check: VS2022WChar
- GitHub Check: VS2019Debug64Cxx20
- GitHub Check: VS2019Debug64Cxx17
- GitHub Check: VS2019Release32
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: VS2019Debug32
- GitHub Check: macos-14-C++
- GitHub Check: VS2019Release64
- GitHub Check: macos-13-C++
- GitHub Check: windows-2019 vs2019
- GitHub Check: VS2019Debug64
- GitHub Check: ubuntu-22.04 g++-12
- GitHub Check: VS2019WChar
- GitHub Check: MINGW64-gcc
- GitHub Check: alpine-3.18
- GitHub Check: ubuntu-20.04 g++-10
🔇 Additional comments (3)
ACE/ace/Ping_Socket.cpp (3)
5-5: LGTM!The added include for "ace/ACE.h" is necessary for the ACE::debug() function used in the conditional debug logging.
114-117: LGTM!The debug message is now properly guarded by ACE::debug() check, maintaining the original error reporting while optimizing for non-debug builds.
227-232: LGTM!The debug messages are consistently guarded by ACE::debug() checks, providing valuable diagnostic information while optimizing for non-debug builds.
Also applies to: 242-246, 270-277, 282-286
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 230-230: There is an unknown macro here somewhere. Configuration is required. If ACE_TEXT is a macro then please configure it.
(unknownMacro)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ACE/ace/Ping_Socket.cpp(6 hunks)
🧰 Additional context used
🪛 Cppcheck (2.10-2)
ACE/ace/Ping_Socket.cpp
[error] 230-230: There is an unknown macro here somewhere. Configuration is required. If ACE_TEXT is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: ubuntu-20.04 clang++-11
- GitHub Check: ubuntu-20.04 clang++-10
- GitHub Check: VS2022Release64
- GitHub Check: VS2022Debug64
- GitHub Check: VS2022WChar
- GitHub Check: VS2019Debug64Cxx20
- GitHub Check: VS2019Debug64Cxx17
- GitHub Check: VS2019Release32
- GitHub Check: macos-14-C++
- GitHub Check: VS2019Debug32
- GitHub Check: macos-13-C++
- GitHub Check: VS2019Release64
- GitHub Check: VS2019Debug64
- GitHub Check: VS2019WChar
- GitHub Check: MINGW64-gcc
- GitHub Check: alpine-3.18
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: windows-2019 vs2019
- GitHub Check: ubuntu-20.04 g++-10
- GitHub Check: ubuntu-22.04 g++-12
🔇 Additional comments (6)
ACE/ace/Ping_Socket.cpp (6)
5-5: LGTM!The addition of the
ace/ACE.hheader is necessary for using theACE::debug()function.
114-117: LGTM!The debug check is correctly placed around the debug log, maintaining consistent indentation and preserving the informative error message.
227-232: LGTM!The debug check is correctly placed around the ICMP length diagnostic log, maintaining the error handling flow.
🧰 Tools
🪛 Cppcheck (2.10-2)
[error] 230-230: There is an unknown macro here somewhere. Configuration is required. If ACE_TEXT is a macro then please configure it.
(unknownMacro)
242-246: LGTM!The debug check is correctly placed around the ICMP_ECHOREPLY notification log.
270-277: LGTM!The debug check is correctly placed around the detailed datagram information log, with proper format specifiers.
282-286: LGTM!The debug check is correctly placed around the unexpected datagram type notification log.
likema
force-pushed
the
ping-socket-dbg
branch
from
21f2776 to
4036d98
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ACE/ace/Ping_Socket.cpp (1)
366-370:⚠️ Potential issueFix incorrect debug message.
The debug message is still incorrect. The
rval_sendvariable will be 0 on success or -1 on error, not the actual bytes sent. The actual length sent isICMP_MIN + ICMP_DATA_LENGTH.Apply this diff:
if (ACE::debug()) ACELIB_DEBUG ((LM_DEBUG, - ACE_TEXT ("(%P|%t) ACE_Ping_Socket::make_echo_check - sent %d bytes.\n"), - rval_send)); + ACE_TEXT ("(%P|%t) ACE_Ping_Socket::make_echo_check - sent %d bytes.\n"), + ICMP_MIN + ICMP_DATA_LENGTH));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ACE/ace/Ping_Socket.cpp(6 hunks)
🧰 Additional context used
🪛 Cppcheck (2.10-2)
ACE/ace/Ping_Socket.cpp
[error] 230-230: There is an unknown macro here somewhere. Configuration is required. If ACE_TEXT is a macro then please configure it.
(unknownMacro)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: macos-13-C++
- GitHub Check: ubuntu-20.04 g++-10 ACE for TAO
- GitHub Check: ubuntu-20.04 clang++-12
- GitHub Check: ubuntu-20.04 clang++-11
- GitHub Check: VS2022Release64
- GitHub Check: ubuntu-20.04 clang++-10
- GitHub Check: VS2022Debug64
- GitHub Check: VS2022WChar
- GitHub Check: VS2019Debug64Cxx20
- GitHub Check: VS2019Debug64Cxx17
- GitHub Check: VS2019Release32
- GitHub Check: VS2019Debug32
- GitHub Check: VS2019Release64
- GitHub Check: VS2019Debug64
- GitHub Check: VS2019WChar
- GitHub Check: MINGW64-gcc
- GitHub Check: alpine-3.18
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: windows-2019 vs2019
- GitHub Check: ubuntu-20.04 g++-10
- GitHub Check: ubuntu-22.04 g++-12
🔇 Additional comments (1)
ACE/ace/Ping_Socket.cpp (1)
114-117: LGTM!The debug condition is appropriately added to control error logging in the constructor.
| ACE_Time_Value const * timeout) | ||
| { | ||
| int rval_send = -1; | ||
| const int rval_send = this->send_echo_check (remote_addr, to_connect); |
There was a problem hiding this comment.
We use right side const normally
| ACE_TEXT ("ACE_Ping_Socket::ACE_Ping_Socket: %p\n"), | ||
| ACE_TEXT ("open"))); | ||
| if (ACE::debug()) | ||
| ACELIB_DEBUG ((LM_DEBUG, |
There was a problem hiding this comment.
Use {} around ACE_LIB_DEBUG, all places here
2 participants
Summary by CodeRabbit