Repository navigation
Conversation
|
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 (2)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughAdds Linux fan-speed discovery through Priority: ⬇️ Low Change: Feature Merge Risk: ⚪ Minimal · up to The Linux fan meter is mergeable after normal checks; no demonstrated issue remains that warrants blocking it. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The feature reads fan telemetry from a fixed system location without adding write operations or privilege changes. A conditional compatibility mismatch could prevent builds or terminate the monitor when openat support is unavailable; exposure on supported Linux configurations remains uncertain. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
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. Hwmon paths begin to hum Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: f9b41fef-98a3-450a-8cec-3864c8e4117c
📒 Files selected for processing (5)
FanMeter.cFanMeter.hMakefile.amlinux/Platform.clinux/Platform.h
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| if (dfd == -1) { | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Remove braces from the three single-statement bodies.
Remove the braces at lines 451-453, 464-466, and 514-516. Keep braces around the cleanup-plus-control-flow bodies at lines 469-471 and 502-504.
The repository guidance requires omitting braces around simple single statements.
|
Hi, just adding some context that might be useful here for this PR While working on #2076 I also drafted a follow-up branch to make the hardware sensor code backend-independent and add direct Linux hwmon support as a fallback when libsensors is unavailable: https://github.com/massimomazzariol/htop/tree/feature/hwmon-fallback The draft currently discovers hwmon temperature, fan, PWM and voltage sensors and feeds them through the same generic The main hwmon backend commit is here: I deliberately kept this out of #2076 because I did not want to expand the scope again after the sensor meter implementation had gone through review. Since #2107 is now adding fan support through hwmon, I thought it was worth mentioning in case parts of that draft or the abstraction could be useful, or if the two approaches should be aligned before both implementations diverge. If you want, feel free to adapt the draft depending on which direction you prefer |
| this->values[residueIndex] = residuePercentage; | ||
| } | ||
|
|
||
| static int Platform_getFanSpeedFromHwmonDevice(int parent_dfd, char const* hwmon_device) { |
There was a problem hiding this comment.
Using const char * here is fine and overall more common in the codebase.
| static int Platform_getFanSpeedFromHwmonDevice(int parent_dfd, char const* hwmon_device) { | |
| static int Platform_getFanSpeedFromHwmonDevice(int parent_dfd, const char *hwmon_device) { |
| DIR* dirp; | ||
| int dfd; | ||
|
|
||
| dfd = openat(parent_dfd, hwmon_device, O_RDONLY | O_CLOEXEC | O_DIRECTORY); |
| continue; | ||
| } | ||
|
|
||
| fclose(fp); |
|
|
||
| int fd = openat(dfd, dir->d_name, O_RDONLY | O_CLOEXEC); | ||
| if (fd == -1) | ||
| continue; |
There was a problem hiding this comment.
Some blank line after the if statement doesn't hurt.
| continue; | |
| continue; | |
| /* | ||
| Traverse hwmon sensors, find first fan, read and return its speed. | ||
| Returns -1 on failure. | ||
| */ |
There was a problem hiding this comment.
| /* | |
| Traverse hwmon sensors, find first fan, read and return its speed. | |
| Returns -1 on failure. | |
| */ | |
| /* Traverse hwmon sensors, find first fan, read and return its speed. */ |
| FileDescriptorMeter.h \ | ||
| FunctionBar.h \ | ||
| GPUMeter.h \ | ||
| FanMeter.h \ |
There was a problem hiding this comment.
File order is sorted alphabetically
|
|
||
| linux_platform_sources = \ | ||
| GPUMeter.c \ | ||
| FanMeter.c \ |
There was a problem hiding this comment.
File order is sorted alphabetically
|
@foxpy No problem. Sometimes PRs take a while to ripen. One of my earliest PRs to htop took more then 6 months and it mostly cleaned up some existing code to make it be shared by all platforms. Given the long-term maintainability is important with a huge code-base, having several approaches to a problem to compare what works best is a clear plus. This applies similarly to using synergies with existing code, thus re-using existing infrastructure to query information or realize a feature is much preferred. @coderabbitai Removing braces around single-statement bodies is not required, but (strongly) encouraged. It's totally fine to have them, e.g. if the statements itself are somewhat complicated, but for most simple cases (i.e. just an assignment), the variant without braces is preferred/recommended. Furthermore: blank openat without @massimomazzariol Your PR #2076 is still on my list of PRs to review, but this might still take some time. As noted above: Having a shared infrastructure for accessing hwmon information without duplicating logic unnecessarily sounds good. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
You are interacting with an AI system. |
|
@BenBE thank you for your review. I am afraid @massimomazzariol 's work in PR #2076 is more useful and we both should focus on reviewing it instead of spending extra time on what I did. What do you think? |
|
As noted earlier in this thread, I prefer if we can re-use infrastructure to fetch information from the system, thus going is #2076 is the probably the best move to go forward. The comments in my previous review are for reference, but also what would need to be done if we were to use this PR instead. Unfortunately I'm currently a bit short on spare time for timely reviews, but I try to get around to them eventually as time permits. That said: I still have #2076 marked for reviewing on my todo list … |
BenBE
added
Linux 🐧
|
Alright, I fixed the issues that you mentioned. I don't know how you would like to proceed, but feel free to close this PR and move on with #2076 if you think that's a better way to go. If you would still like to proceed with this PR, I have a question: do you want me to squash all commits here and force push, so we don't have this dangling "review fixes" commit lying around? |
|
Huh… I think the implementation of openat accepts fd as its first argument, but in |
The type of In the header you will thus find this block: #ifdef HAVE_OPENAT
typedef int openat_arg_t;
static inline void Compat_openatArgClose(openat_arg_t dirfd) {
close(dirfd);
}
static inline int Compat_openat(openat_arg_t dirfd, const char* pathname, int flags) {
return openat(dirfd, pathname, flags);
}
#else /* HAVE_OPENAT */
typedef const char* openat_arg_t;
static inline void Compat_openatArgClose(openat_arg_t dirpath) {
(void)dirpath;
}
int Compat_openat(openat_arg_t dirpath, const char* pathname, int flags);
#endif /* HAVE_OPENAT */which forwards the Thus when using #ifdef HAVE_OPENAT
int cpuDirFd = openat(xDirfd(dir), entry->d_name, O_DIRECTORY | O_PATH | O_NOFOLLOW);
if (cpuDirFd < 0)
continue;
#else
char cpuDirFd[4096];
xSnprintf(cpuDirFd, sizeof(cpuDirFd), "/sys/devices/system/cpu/%s", entry->d_name);
#endifHope this helps with your confusion. |
Yes, it helps a lot! And also tells that I've written grossly incorrect code :) |
3 participants
This PR is a replacement for #2009, which implemented the same feature, but using a vendor-specific interface, only supporting Thinkpads running Linux. The new version should support all Linux devices, because it uses hwmon.
It only reports the speed of the first fan it finds, which is not great, but hey, it's something :)
@BenBE sorry for ignoring #2009 for the entire summer, I've been busy with personal stuff and completely forgot about this :D
I remember that we talked about adding support for minimum and maximum fan speed reporting, which I also decided to keep aside for now.