Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FlexMeter: Add FlexMeter functionality #1571

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

FlexMeter: Add FlexMeter functionality #1571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be2fbbc
FlexMeter: Add FlexMeter functionality
bogdanovs Jan 6, 2025
c337785
Update all Platforms to support FlexMeter
bogdanovs Jan 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CRT.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ static int CRT_colorSchemes[LAST_COLORSCHEME][LAST_COLORELEMENT] = {
[FAILED_READ] = A_BOLD | ColorPair(Red, Black),
[PAUSED] = A_BOLD | ColorPair(Yellow, Cyan),
[UPTIME] = A_BOLD | ColorPair(Cyan, Black),
[FLEX] = A_BOLD | ColorPair(Cyan, Black),
[BATTERY] = A_BOLD | ColorPair(Cyan, Black),
[LARGE_NUMBER] = A_BOLD | ColorPair(Red, Black),
[METER_SHADOW] = A_BOLD | ColorPairGrayBlack,
Expand Down Expand Up @@ -247,6 +248,7 @@ static int CRT_colorSchemes[LAST_COLORSCHEME][LAST_COLORELEMENT] = {
[FAILED_READ] = A_BOLD,
[PAUSED] = A_BOLD | A_REVERSE,
[UPTIME] = A_BOLD,
[FLEX] = A_BOLD,
[BATTERY] = A_BOLD,
[LARGE_NUMBER] = A_BOLD,
[METER_SHADOW] = A_DIM,
Expand Down Expand Up @@ -365,6 +367,7 @@ static int CRT_colorSchemes[LAST_COLORSCHEME][LAST_COLORELEMENT] = {
[FAILED_READ] = ColorPair(Red, White),
[PAUSED] = A_BOLD | ColorPair(Yellow, Cyan),
[UPTIME] = ColorPair(Yellow, White),
[FLEX] = ColorPair(Yellow, White),
[BATTERY] = ColorPair(Yellow, White),
[LARGE_NUMBER] = ColorPair(Red, White),
[METER_SHADOW] = ColorPair(Blue, White),
Expand Down Expand Up @@ -483,6 +486,7 @@ static int CRT_colorSchemes[LAST_COLORSCHEME][LAST_COLORELEMENT] = {
[FAILED_READ] = ColorPair(Red, Black),
[PAUSED] = A_BOLD | ColorPair(Yellow, Cyan),
[UPTIME] = ColorPair(Yellow, Black),
[FLEX] = ColorPair(Yellow, Black),
[BATTERY] = ColorPair(Yellow, Black),
[LARGE_NUMBER] = ColorPair(Red, Black),
[METER_SHADOW] = A_BOLD | ColorPairGrayBlack,
Expand Down Expand Up @@ -601,6 +605,7 @@ static int CRT_colorSchemes[LAST_COLORSCHEME][LAST_COLORELEMENT] = {
[FAILED_READ] = A_BOLD | ColorPair(Red, Blue),
[PAUSED] = A_BOLD | ColorPair(Yellow, Cyan),
[UPTIME] = A_BOLD | ColorPair(Yellow, Blue),
[FLEX] = A_BOLD | ColorPair(Yellow, Blue),
[BATTERY] = A_BOLD | ColorPair(Yellow, Blue),
[LARGE_NUMBER] = A_BOLD | ColorPair(Red, Blue),
[METER_SHADOW] = ColorPair(Cyan, Blue),
Expand Down Expand Up @@ -719,6 +724,7 @@ static int CRT_colorSchemes[LAST_COLORSCHEME][LAST_COLORELEMENT] = {
[FAILED_READ] = A_BOLD | ColorPair(Red, Black),
[PAUSED] = A_BOLD | ColorPair(Yellow, Green),
[UPTIME] = ColorPair(Green, Black),
[FLEX] = ColorPair(Green, Black),
[BATTERY] = ColorPair(Green, Black),
[LARGE_NUMBER] = A_BOLD | ColorPair(Red, Black),
[METER_SHADOW] = A_BOLD | ColorPairGrayBlack,
Expand Down
1 change: 1 addition & 0 deletions CRT.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@ typedef enum ColorElements_ {
DYNAMIC_MAGENTA,
DYNAMIC_YELLOW,
DYNAMIC_WHITE,
FLEX,
LAST_COLORELEMENT
} ColorElements;

Expand Down
217 changes: 217 additions & 0 deletions FlexMeter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,217 @@
/*
htop - FlexMeter.c
(C) 2024 Stoyan Bogdanov
Released under the GNU GPLv2+, see the COPYING file
in the source distribution for its full text.
*/

#include <dirent.h>
#include <pwd.h>
#include <sys/time.h>
#include <sys/types.h>
#include <time.h>
#include <unistd.h>

#include "CRT.h"
#include "config.h"
#include "FlexMeter.h"
#include "Object.h"


#define FLEX_CFG_FOLDER ".config/htop/FlexMeter"

typedef struct {
char* name;
char* command;
char* type;
char* caption;
char* uiName;
} _flex_meter;

_flex_meter meter_list[METERS_LIST_SIZE];

static int meters_count = 0;

static const int DateMeter_attributes[] = {
FLEX
};

MeterClass* FlexMeter_class = NULL;

static int check_for_meters(void);

static bool parse_input(_flex_meter *meter, char* line)
{
bogdanovs marked this conversation as resolved.
Show resolved Hide resolved
switch(line[0])
{
case 'n':
if (String_startsWith(line, "name=")) {
xAsprintf(&meter->uiName, "Flex: %s", line + 5);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memory leak, if multiple lines with name= are provided.

Similar below.

Have a look at free_and_xStrdup for the cases below.
For the xAsprintf call site calling free on meter->uiName should suffice.

}
break;
case 'c':
if (String_startsWith(line, "command=")) {
meter->command = xStrdup(line + 8);
} else if (String_startsWith(line, "caption=")) {
meter->caption = xStrdup(line + 8);
}
break;
case 't':
if (String_startsWith(line, "type=")) {
meter->type = xStrdup(line + 6);
}
break;
default:
return false;
}

return true;
}

static bool load_config(_flex_meter *meter, char* file)
{
bool ret = false;
FILE* fp = fopen(file, "r");

if (fp != NULL) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, that fclose(NULL); is undefined behaviour, usually characterized to crash your program. ;-)

Suggested change
if (fp != NULL) {
if (!fp) {
return false;
}

Doing it that way around also allows to lower the indentation level.

char* buff;
while ((buff = String_readLine(fp)) != NULL) {
ret = parse_input(meter, buff);
if (!ret) {
break;
}
}
free(buff);
buff = NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly necessary. should best be removed.

For stack variables, calling free suffices. Only when the variable or its value is exposed to other scopes it's necessary to explicitly set the value to NULL.

}

fclose(fp);
return ret;
}

static int check_for_meters(void)
{
char* path;
struct dirent* dir;
struct passwd* pw = getpwuid(getuid());
const char* homedir = pw->pw_dir;
Comment on lines +96 to +97
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note the comment by @Explorer09 regarding the XDG_HOME_DIR variable and fallbacks …

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is current XDG_CONFIG_HOME and fallbacks enough ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getpwuid has some bad side effects (calls to libpam and potentially causes network traffic; thus can potentially hang). Furthermore, when building htop with --enable-static this may cause issues, thus call sites should be minimized.

Not to mention the call can fail and is lacking error checks.

Please compare with Settings_new in Settings.c. Possibly, we should refactor this code block to be a separate function. @Explorer09?

char* home = NULL;
bool ret;

const char* xdgConfigHome = getenv("XDG_CONFIG_HOME");
const char* homedirEnv = getenv("HOME");

if (xdgConfigHome) {
xAsprintf(&home, "%s/%s", xdgConfigHome, "htop/FlexMeter");
} else if (homedirEnv) {
xAsprintf(&home, "%s/%s", homedirEnv, FLEX_CFG_FOLDER);
} else {
xAsprintf(&home, "%s/%s", homedir, FLEX_CFG_FOLDER);
}

struct stat fileStat;

if (stat(home, &fileStat) < 0) {
return -1;
}

uint32_t uid = getuid();

if ((fileStat.st_uid == uid) && S_ISDIR(fileStat.st_mode) &&
((fileStat.st_mode & 0777) == 0700)) {
DIR* d = opendir(home);
if (d) {
while ((dir = readdir(d)) != NULL) {
if ( dir->d_name[0] == '.') {
/* We are ignoring all files starting with . like ".Template"
* and "." ".." directories
*/
continue;
}

meter_list[meters_count].name = xStrdup(dir->d_name);
xAsprintf(&path, "%s/%s", home, dir->d_name);

if (stat(path, &fileStat) < 0) {
return -1;
}

if ((fileStat.st_uid == uid) && ((fileStat.st_mode & 0777) == 0700)) {
ret = load_config(&meter_list[meters_count], path);

if (ret && (meters_count < MAX_METERS_COUNT)) {
meters_count++;
}
}

free(path);
path=NULL;

}
closedir(d);
}
}

free(home);
home = NULL;

return meters_count;
}

static void FlexMeter_updateValues(Meter* this)
{
for (size_t i = 0 ; i < (size_t)meters_count; i++) {
if (this->m_ptr == &FlexMeter_class[i] ) {
char* buff = NULL;
int ret = -1;
if (meter_list[i].command) {
FILE* fd = popen(meter_list[i].command, "r");
if (fd) {
buff = String_readLine(fd);
ret = pclose(fd);
}
}

if (buff && !ret) {
xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "%s", buff);
} else {
// Once fail, free command pointer and every time print Error message
if (meter_list[i].command != NULL) {
free(meter_list[i].command);
meter_list[i].command = NULL;
}
xSnprintf(this->txtBuffer, sizeof(this->txtBuffer), "%s", "[ERR] Check command");
}
}
}
}

const MeterClass FlexMeter_class_template = {
.super = {
.extends = Class(Meter),
.delete = Meter_delete
},
.updateValues = FlexMeter_updateValues,
.defaultMode = TEXT_METERMODE,
.maxItems = 1,
.total = 100,
.attributes = DateMeter_attributes,
.name = NULL,
.uiName = NULL,
.caption = NULL,
};

int load_flex_modules(void)
{
size_t meters_num = check_for_meters();
if (!FlexMeter_class && meters_num > 0) {
FlexMeter_class = (MeterClass*) xCalloc(meters_num, sizeof(MeterClass));
for (size_t i = 0 ; i < meters_num; i++) {
memcpy(&FlexMeter_class[i], &FlexMeter_class_template, sizeof(MeterClass));
FlexMeter_class[i].name = (const char*) xStrdup(meter_list[i].name);
FlexMeter_class[i].uiName = (const char*) xStrdup(meter_list[i].uiName);
FlexMeter_class[i].caption = (const char*) xStrdup(meter_list[i].caption);
Comment on lines +211 to +213
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows for easy meter impersonation in the UI. IDK if this is a good thing to have …

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BenBE can you please elaborate ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the unrestricted nature of how the uiName and caption are set, a FlexMeter could just configure to use the same strings as another (built-in) meter and thus impersonate another (e.g. built-in) meter. This is not a security issue directly, but more a point for discussion, if we want to allow the user to create FlexMeters that are indistinguishable from built-in meters.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point from @BenBE about meter impersonation. But since we have PCPDynamicMeter already, what would PCPDynamicMeter handle this situation?

I think @natoscott can come in and look at this whole PR and give a review.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think could be overcome with prefix or suffix to the uiName and for the caption it is matter of user decision what caption should be selected. It could be literally everything

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Good suggestion about a meter prefix. And it could avoid name collisions with any meter added into htop built-in in the future. (BTW, did this issue ever come up with PCPDynamicMeter too?)
  2. For "captions" I would suggest a laxer filter. Honestly I would like to see Unicode support for custom meter captions, because why not. The only kind of characters worth filtering out is non-printable characters (including control characters). Otherwise I don't like the things to be too strict when it comes to custom meter captions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Good suggestion about a meter prefix. And it could avoid name collisions with any meter added into htop built-in in the future. (BTW, did this issue ever come up with PCPDynamicMeter too?)

No, AFAIR not. But there is some code in PCPDynamicColumn_validateColumnName that does minimalistic filtering. Also there are checks to detect duplicate names.

  1. For "captions" I would suggest a laxer filter. Honestly I would like to see Unicode support for custom meter captions, because why not. The only kind of characters worth filtering out is non-printable characters (including control characters). Otherwise I don't like the things to be too strict when it comes to custom meter captions.

ACK.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Flex: prefix - ok
  2. lexer is good but too powerful tool for that task. I think simple implementation will do the job. Valid range from 26 to 126, for caption is open enough and without non printable chars

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For caption= all* >=32 is fine. With just 26<=c && c<=126 you cause two issues:

  1. You allow for inserting ECMA48 escape sequences (which can rebind keys -> security issue)
  2. You break Unicode support for things like caption=運勢 (when executing command=LANG=ja_JA fortune)

*Unicode filtering has its whole other can of worms … But that would apply to the output of command too …

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't expect I would comment on Unicode filtering here. Do you guys plan to revise or improve the Unicode filtering code in RichString class?
Here is a draft code of mine, just for reference.

}
}
return meters_num;
}
22 changes: 22 additions & 0 deletions FlexMeter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
#ifndef HEADER_FlexMeter
#define HEADER_FlexMeter
/*
htop - FlexMeter.c
(C) 2024 Stoyan Bogdanov
Released under the GNU GPLv2+, see the COPYING file
in the source distribution for its full text.
*/
#include <stdbool.h>

#include "Meter.h"


#define METERS_LIST_SIZE 30

#define MAX_METERS_COUNT METERS_LIST_SIZE-1
Comment on lines +14 to +16
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one of these (METERS_LIST_SIZE, MAX_METERS_COUNT) should exist.

Also, I'd suggest a slight rename:

Suggested change
#define METERS_LIST_SIZE 30
#define MAX_METERS_COUNT METERS_LIST_SIZE-1
#define MAX_FLEXMETERS_COUNT 30


extern MeterClass *FlexMeter_class ;

int load_flex_modules(void);

#endif
10 changes: 9 additions & 1 deletion Header.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,14 @@ in the source distribution for its full text.
#include "CRT.h"
#include "CPUMeter.h"
#include "DynamicMeter.h"
#include "FlexMeter.h"
#include "Macros.h"
#include "Object.h"
#include "Platform.h"
#include "ProvideCurses.h"
#include "Settings.h"
#include "XUtils.h"


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep the second blank after the list of includes.

Header* Header_new(Machine* host, HeaderLayout hLayout) {
Header* this = xCalloc(1, sizeof(Header));
this->columns = xMallocArray(HeaderLayout_getColumns(hLayout), sizeof(Vector*));
Expand Down Expand Up @@ -121,6 +121,14 @@ void Header_populateFromSettings(Header* this) {
const Settings* settings = this->host->settings;
Header_setLayout(this, settings->hLayout);

int num = load_flex_modules();
int platform_size = 0;

for (platform_size = 0; Platform_meterTypes[platform_size] != NULL; platform_size++);
for (int i = 0; i < num; i++) Platform_meterTypes[platform_size+i]=FlexMeter_class+i;

Platform_meterTypes[platform_size+num]=NULL;
Comment on lines +127 to +130
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space around binary operators:

Suggested change
for (platform_size = 0; Platform_meterTypes[platform_size] != NULL; platform_size++);
for (int i = 0; i < num; i++) Platform_meterTypes[platform_size+i]=FlexMeter_class+i;
Platform_meterTypes[platform_size+num]=NULL;
for (platform_size = 0; Platform_meterTypes[platform_size] != NULL; platform_size++) {
// Count meter types for the platform
}
for (int i = 0; i < num; i++) {
Platform_meterTypes[platform_size + i] = FlexMeter_class + i;
}
Platform_meterTypes[platform_size + num] = NULL;

Furthermore, for loops always should have a body; and be it empty.


Header_forEachColumn(this, col) {
const MeterColumnSetting* colSettings = &settings->hColumns[col];
Vector_prune(this->columns[col]);
Expand Down
4 changes: 4 additions & 0 deletions Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ myhtopsources = \
DynamicScreen.c \
EnvScreen.c \
FileDescriptorMeter.c \
FlexMeter.c \
FunctionBar.c \
Hashtable.c \
Header.c \
Expand Down Expand Up @@ -93,6 +94,7 @@ myhtopsources = \
Vector.c \
XUtils.c


myhtopheaders = \
Action.h \
Affinity.h \
Expand All @@ -118,6 +120,7 @@ myhtopheaders = \
DynamicScreen.h \
EnvScreen.h \
FileDescriptorMeter.h \
FlexMeter.h \
FunctionBar.h \
Hashtable.h \
Header.h \
Expand Down Expand Up @@ -165,6 +168,7 @@ myhtopheaders = \
Vector.h \
XUtils.h


# Linux
# -----

Expand Down
1 change: 1 addition & 0 deletions Meter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,7 @@ Meter* Meter_new(const Machine* host, unsigned int param, const MeterClass* type
this->values = type->maxItems ? xCalloc(type->maxItems, sizeof(double)) : NULL;
this->total = type->total;
this->caption = xStrdup(type->caption);
this->m_ptr = type;
if (Meter_initFn(this)) {
Meter_init(this);
}
Expand Down
9 changes: 5 additions & 4 deletions Meter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,9 @@ typedef struct MeterClass_ {
const MeterModeId defaultMode;
const double total;
const int* const attributes;
const char* const name; /* internal name of the meter, must not contain any space */
const char* const uiName; /* display name in header setup menu */
const char* const caption; /* prefix in the actual header */
const char* name; /* internal name of the meter, must not contain any space */
const char* uiName; /* display name in header setup menu */
const char* caption; /* prefix in the actual header */
Comment on lines +70 to +72
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const char* name; /* internal name of the meter, must not contain any space */
const char* uiName; /* display name in header setup menu */
const char* caption; /* prefix in the actual header */
const char* name; /* internal name of the meter, must not contain any space */
const char* uiName; /* display name in header setup menu */
const char* caption; /* prefix in the actual header */

const char* const description; /* optional meter description in header setup menu */
const uint8_t maxItems;
const bool isMultiColumn; /* whether the meter draws multiple sub-columns (defaults to false) */
Expand Down Expand Up @@ -102,8 +102,9 @@ typedef struct GraphData_ {
struct Meter_ {
Object super;
Meter_Draw draw;
const Machine* host;

const Machine* host;
const MeterClass* m_ptr;
char* caption;
MeterModeId mode;
unsigned int param;
Expand Down
1 change: 1 addition & 0 deletions configure.ac
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ AM_INIT_AUTOMAKE([-Wall std-options subdir-objects])

# ----------------------------------------------------------------------

AC_DEFINE([MAX_PLATFORM_METERS], [100], [Set max meters for number])

# ----------------------------------------------------------------------
# Checks for platform.
Expand Down
Loading