Linux: add meter showing the current active user sessions #1364
Conversation
cgzones
added
Linux 🐧
There was a problem hiding this comment.
What re OpenBSD/NetBSD/Solaris?
@fraggerfox Can you help here?
I can check this out in the coming weekend and see if I can make the changes work in NetBSD. |
UserSessionsMeter.c
Outdated
| ret = updateViaUtmp(); | ||
| #else | ||
| ret = updateViaUtmp(); | ||
| #endif /* !BUILD_STATIC || HAVE_LIBSYSTEMD */ |
There was a problem hiding this comment.
- Can the systemd code depend only on
HAVE_LIBSYSTEMD? People who build htop without systemd support would likely not want the systemd code even when it's dynamically loaded and linked. - Code simplification suggestion:
int ret = -1; // Or maybe -ENOSYS for better diagnostic message
#if defined(HAVE_LIBSYSTEMD)
ret = update_via_sd_login();
#endif /* HAVE_LIBSYSTEMD */
if (ret < 0)
ret = updateViaUtmp();There was a problem hiding this comment.
-
There is a systemd meter. Linking should be done accordingly as it's already done there.
-
This looks a bit cleaner, but has a dead store in
ret.
cgzones
force-pushed
the
user_count
branch
3 times, most recently
from
9499ecb
to
e3f970c
Compare
Display the active user sessions either via systemd-logind or utmp. On Linux try to query systemd-login first, and otherwise count only the number of distinct sessions. On other platforms the number of unique sessions can not be determined and the number of all user entries are returned. Suggested in htop-dev#1363.
According to manual pages only OpenBSD does have not support for it. |
| if (!found) { | ||
| ret++; | ||
|
|
||
| if (sessions_size +1 >= sessions_cap) { |
There was a problem hiding this comment.
| if (sessions_size +1 >= sessions_cap) { | |
| if (sessions_size + 1 >= sessions_cap) { |
| #ifndef BUILD_STATIC | ||
| #include <dlfcn.h> | ||
| #include "linux/Platform.h" | ||
| #endif |
There was a problem hiding this comment.
There should not be a platform-specific reference in the platform-independent code section.
| int ret; | ||
| #if (!defined(BUILD_STATIC) && defined(HTOP_LINUX)) || defined(HAVE_LIBSYSTEMD) | ||
| ret = update_via_sd_login(); | ||
| if (ret < 0) | ||
| ret = Generic_utmpx_get_users(); | ||
| #else | ||
| ret = Generic_utmpx_get_users(); | ||
| #endif /* (!BUILD_STATIC && HTOP_LINUX) || HAVE_LIBSYSTEMD */ | ||
|
|
||
| users = ret; |
There was a problem hiding this comment.
Move this code into a platform-specific function. For everything aside from Linux this can be a direct forward to Generic_utmpx_get_users; on Linux that's mostly the above block of code.
| if (users >= 0) { | ||
| xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "%d", ret); |
There was a problem hiding this comment.
Why distinguish here between ret and users?
| int len = xSnprintf(buffer, sizeof(buffer), "%d", users); | ||
| RichString_appendnAscii(out, CRT_colors[METER_VALUE], buffer, len); | ||
| } else { | ||
| RichString_appendAscii(out, CRT_colors[METER_VALUE], "N/A"); |
There was a problem hiding this comment.
Maybe shadow the value if not available?
There was a problem hiding this comment.
For meter text, I don't think shadowing is a good idea. IMHO.
| static void* libsystemd_handle; | ||
| static size_t libsystemd_loaded; | ||
|
|
||
| void* Platform_load_libsystemd(void) { |
There was a problem hiding this comment.
I write some ideas just in case you are planning to refactor this further:
- The LibSystemd init code can become its own C files,
LibSystemd.candLibSystemd.h. Within it theLibSystemd_init()function would perform what you havePlatform_load_libsystemd()here. Look at the existinglinux/LibSensors.cfile for what I mean. Dynamically loaded libraries should have the init and unload functions like that. - When a libsystemd handle has been
dlopen'd, all the symbols htop requires should resolve all at once. No need to have theresolve()code in different places for the same library. - Make the resolved function pointers publicly assessible. That is, put these in
linux/LibSystemd.hwhen needed:
// In "linux/LibSystemd.h"
#ifdef BUILD_STATIC
#define sym_sd_bus_open_system sd_bus_open_system
#define sym_sd_bus_open_user sd_bus_open_user
#define sym_sd_bus_get_property_string sd_bus_get_property_string
#define sym_sd_bus_get_property_trivial sd_bus_get_prope
rty_trivial
#define sym_sd_bus_unref sd_bus_unref
#define sym_sd_get_sessions sd_get_sessions
#else
extern int (*sym_sd_bus_open_system)(sd_bus**);
extern int (*sym_sd_bus_open_user)(sd_bus**);
extern int (*sym_sd_bus_get_property_string)(sd_bus*, const char*, const char*, const char*, const char*, sd_bus_error*, char**);
extern int (*sym_sd_bus_get_property_trivial)(sd_bus*, const char*, const char*, const char*, const char*, sd_bus_error*, char, void*);
extern sd_bus* (*sym_sd_bus_unref)(sd_bus*);
extern int (*sym_sd_get_sessions)(char***);
#endif
cgzones
added
the
needs-rebase
Display the active user sessions either via systemd-logind or utmp.
Suggested in #1363.