Repository navigation
Conversation
+ Added new net monitoring tab for tcp 4/6 monitoring
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe pull request adds CMake build configuration, including platform detection, dependency probes, optional features, executable targets, and install rules. It also adds Linux TCP traffic-rate monitoring, per-process upload and download fields, and a Network Monitor screen. Platform cleanup functions now accept a machine pointer, and Linux cleanup releases network-monitoring resources. Suggested reviewers: Priority: ➖ Normal Change: Feature Merge Risk: 🟡 Moderate · up to Linux refreshes now perform extra process-descriptor scans even when the network view is unused, and long sessions can accumulate monitoring state; the monitor can also display stale rates. Address or explicitly accept these risks before merging. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to Linux monitoring now runs on every rescan, even outside the Network Monitor screen. Socket history has no eviction bound, and some process-rate updates can allocate objects without retaining an owner. Local socket activity can therefore increase the monitor's memory consumption over time. The demonstrated exposure is limited to monitoring instances that can observe those processes; privilege escalation or cross-service compromise was not established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. TCP currents cross the screen Comment |
There was a problem hiding this comment.
Actionable comments posted: 20
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
1e2636ec-e8cd-4ec6-88cb-b06c4ec930b7
📒 Files selected for processing (33)
CMakeLists.txtCMakeLists3.txtCommandLine.cConfig.h.inMachine.hMakefile.amdarwin/Platform.cdarwin/Platform.hdragonflybsd/Platform.cdragonflybsd/Platform.hfreebsd/Platform.cfreebsd/Platform.hlinux/LinuxMachine.clinux/LinuxMachine.hlinux/LinuxProcess.clinux/LinuxProcess.hlinux/LinuxProcessTable.clinux/NetSpeed.clinux/NetSpeed.hlinux/Platform.clinux/Platform.hlinux/ProcessField.hnetbsd/Platform.cnetbsd/Platform.hnetmonitor.copenbsd/Platform.copenbsd/Platform.hpcp/Platform.cpcp/Platform.hsolaris/Platform.csolaris/Platform.hunsupported/Platform.cunsupported/Platform.h
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| #define TRAFFIC_HASH_SIZE 4096 | ||
| #define HASH_SIZE 4096 | ||
| #define CACHE_TTL_SEC 10 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Rename the generic HASH_SIZE and CACHE_TTL_SEC macros.
LinuxMachine.h includes this header, so many translation units see these macros. HASH_SIZE is a common name, and a collision with another header causes a redefinition or silent behavior change. CACHE_TTL_SEC is not used, because the TTL check in NetSpeed.c is commented out. Rename the macros with a NETSPEED_ prefix, or move them into NetSpeed.c.
| LinuxMachine* linuxMachine = (LinuxMachine*)host; | ||
| NetSpeed_cleanup(linuxMachine->netMonitoringData); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Free the NetMonitoringData allocation. NetSpeed_monitor allocates netMonitoringData. NetSpeed_cleanup frees only the struct's members, and no code frees the struct.
linux/Platform.c#L1184-L1185: afterNetSpeed_cleanup, callfree(linuxMachine->netMonitoringData)and set the pointer to NULL.linux/LinuxMachine.h#L105-L105: document that theLinuxMachineowner frees this pointer during platform cleanup.
📍 Affects 2 files
linux/Platform.c#L1184-L1185(this comment)linux/LinuxMachine.h#L105-L105
|
This duplicates functionality already present in #2105 … Also, please take a very close look at the style guide and contribution documents. Your code style is all over the place. |
|
Closing as the already existing #2105 PR is more advanced |
This pull request adds supports for displaying network (TCP) speed statistics per process. Precisely, the following have be added: