Conversation
2 changes to make htop compatible with OSX 10.4 Tiger 1. `mach_host_self()` and `vm_page_size` are defined in `mach_init.h`. 2. `<libproc.h>` API is not available, related functionality is disabled. htop builds and works but cannot display per-process CPU% and task%. This can hopefully be improved in the future but at least it builds for now.
BenBE
added
code quality ♻️
|
Disabling huge chunks of the existing code may have very adverse effects. Please refine this patch in a way that it works more localized. |
|
I have no idea how to make it more localized, considering that the entire |
BenBE
left a comment
BenBE
left a comment
There was a problem hiding this comment.
That's what I mean by "too broad" …
| #if MAC_OS_X_VERSION_MIN_REQUIRED >= 1050 | ||
| char path[PROC_PIDPATHINFO_MAXSIZE]; | ||
|
|
||
| int r = proc_pidpath(pid, path, sizeof(path)); | ||
| if (r <= 0) | ||
| return; | ||
|
|
||
| Process_updateExe(proc, path); | ||
| #endif |
There was a problem hiding this comment.
This leaves Process_updateExe uninitialized; at least call this explicitly as NULL, so things can be set up correctly as "unavailable".
| free_and_xStrdup(&proc->procCwd, vpi.pvi_cdir.vip_path); | ||
| #endif |
There was a problem hiding this comment.
Same here. at least call free_and_xStrdup(&proc->procCwd, NULL); so things are defined.
| #else | ||
| proc->taskAccess = false; | ||
| #endif |
There was a problem hiding this comment.
Try to fill all the other fields here too; and be it with sensible "defaults" like "no time passed" for "one task"/"none running". Mark a todo to implement these with older APIs as necessary.
|
|
||
| vm_deallocate(mach_task_self(), (vm_address_t) thread_list, sizeof(thread_port_array_t) * thread_count); | ||
| mach_port_deallocate(mach_task_self(), task); | ||
| #endif |
There was a problem hiding this comment.
Same here. If there's not even essential information filled in we can also just declare things fully incompatible with such old versions of MacOS …
| #include <net/if_types.h> | ||
| #include <net/route.h> | ||
| #include <sys/socket.h> | ||
| #include <mach/mach_init.h> |
There was a problem hiding this comment.
Why is this here, when it's not used through new code? Or is was this missing before; and by which code?
| #if MAC_OS_X_VERSION_MIN_REQUIRED >= 1050 | ||
| #include <libproc.h> | ||
| #endif |
There was a problem hiding this comment.
See docs/styleguide.md. Conditional includes should be below headers that are always included.
|
Thank you for the review, I'll get back to it next weekend. In the meantime, I figured out how to get some of the info on Tiger (though it requires root for privileged processes). |
|
4 participants
2 changes to make htop compatible with OSX 10.4 Tiger
mach_host_self()andvm_page_sizeare defined inmach_init.h.<libproc.h>API is not available, related functionality is disabled.htop builds and works but cannot display per-process CPU% and task%. This can hopefully be improved in the future but at least it builds for now.
/cc @barracuda156