Skip to content

Refactor makeCommandStr#388

Merged
BenBE merged 29 commits intohtop-dev:masterfrom
BenBE:command_color_instr
May 23, 2021
Merged

Refactor makeCommandStr#388
BenBE merged 29 commits intohtop-dev:masterfrom
BenBE:command_color_instr

Conversation

@BenBE
Copy link
Copy Markdown
Member

@BenBE BenBE commented Dec 12, 2020

This is some refactoring work for makeCommandStr to make this suitable for platform independent use.

Changes include:

  • Move cmdline, comm and exe to platform-independent Process structure.
  • Move ProcessMergeCommand to platform-independent Process structure.
  • Move procExeDeleted to platform-independent Process structure.
  • Allow for individual activation/deactivation of highlighted components (basename, comm, deleted, separator, …)

The current state is mostly some PoC and still WIP.

@BenBE BenBE added enhancement Extension or improvement to existing feature code quality ♻️ Code quality enhancement needs-discussion 🤔 Changes need to be discussed and require consent labels Dec 12, 2020
@BenBE BenBE requested a review from cgzones December 12, 2020 11:28
@BenBE BenBE self-assigned this Dec 12, 2020
@BenBE BenBE added this to the 3.0.4 milestone Dec 14, 2020
@BenBE BenBE force-pushed the command_color_instr branch from 4f8697c to 8ef5992 Compare December 16, 2020 18:09
@cgzones
Copy link
Copy Markdown
Member

cgzones commented Dec 18, 2020

After switching from merged to standard mode:

linux/LinuxProcess.c:432:10: runtime error: implicit conversion from type 'int' of value -38 (32-bit, signed) to type 'size_t' (aka 'unsigned long') changed the value to 18446744073709551578 (64-bit, unsigned)
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior linux/LinuxProcess.c:432:10 in

@BenBE BenBE modified the milestones: 3.0.4, 3.0.6 Dec 19, 2020
@BenBE
Copy link
Copy Markdown
Member Author

BenBE commented Dec 19, 2020

Moving this item back a few releases, as we'd run into several issues I noticed:

  1. To move PROC_COMM/PROC_EXE to platform-independent grounds I'd have to change user-facing enums, thus breaking existing configurations using these columns. Requires a proper upgrade path, which is out-of-scope of this issue.
  2. Some internal APIs (Process_isKernelThread/Process_isUserlandThread) are named as Platform-Independent, but actually are not (platform-implemented everywhere except MacOS). Repairing this is another issue I don't want to address right now, as this is a general code issue.
  3. To implement some changes I noticed I'd actually need some updates still pending in open PRs (e.g. Process Field Rework #405, New option "Tree view always by PID", replicating htop 2 behavior #411).

Thus even though I'd really like to bring this refactoring in ASAP it's causing too much noise to justify it right now.

@BenBE BenBE force-pushed the command_color_instr branch 2 times, most recently from 1efa15d to 6dd225d Compare December 19, 2020 15:33
@BenBE BenBE force-pushed the command_color_instr branch from 6dd225d to 4d124d9 Compare December 25, 2020 12:20
@BenBE BenBE force-pushed the command_color_instr branch from 4d124d9 to a3f49a4 Compare January 11, 2021 19:37
@BenBE BenBE force-pushed the command_color_instr branch 2 times, most recently from ea60a21 to cf22f73 Compare January 19, 2021 21:49
@BenBE BenBE force-pushed the command_color_instr branch 4 times, most recently from da55a7a to f075725 Compare February 1, 2021 22:29
@cgzones
Copy link
Copy Markdown
Member

cgzones commented Feb 5, 2021

Maybe you could refactor everything process command related into a C++ like class:
A struct with all the relevant data fields:

stuct ProcessCommand {
   char* fullCommand;
   char* comm;   
   char* exe;

   int startIndexOfExeInFullCommand;
   int endIndexOfExeInFullCommand;
   int startIndexOfCommInFullCommand;
   int endIndexOfCommInFullCommand;
   ...

and then have public methods

void ProcessCommand_updateExe(ProcessCommand* pc, const char* exe);
void ProcessCommand_updateComm(ProcessCommand* pc, const char* comm);
void ProcessCommand_updateFullCommand(ProcessCommand* pc, const char* fullCommand);
void ProcessCommand_format(const ProcessCommand* pc, RichString* str);

and no code outside of these public functions (and maybe some private static ones) reads/writes any of the data fields of the ProcessCommand struct directly.
That way it might be easier to ensure/review/maintain all invariants are always guaranteed (indices are either -1 or point to valid memory, indices are -1 if the corresponding data is not-available/NULL, ...).

Another thought: does it make sense to colour common command-path prefixes as gray (like /usr/bin/, /usr/sbin/, /sbin/, /bin/, /lib/systemd/, /usr/libexec/)?

@BenBE
Copy link
Copy Markdown
Member Author

BenBE commented Feb 6, 2021

The first step I'm working on right now is bringing all the required fields into the common code base, i.e. the following fields:

  • comm (cmdline, the process command line)

  • procComm (procComm, command title)

  • procExe (procExe, running executable)

  • basenameOffset (cmdlineBasenameOffset, actually end of first command line token)

  • procCmdlineBasenameOffset (cmdlineBasenameStart, start of command file basename)

  • procCmdlineBasenameEnd -> Removed (overlaps with cmdlineBasenameOffset)

  • procExeBasenameOffset (procExeBasenameStart)

  • procExeLen -> Removed (implicit by strlen(procExe))

  • procExeDeleted (procExeDeleted, true if ProcExe was modified/deleted in the filesystem)

After this I planned to make the common code paths (like extracting the basename from the command line, more homogeneous. Abstracting them away in a pseudo-class might be worth a look, but most implementations will likely use one of two implementations anyway: Extract from NUL-terminated vs. extract from blank-separated, where the first may trigger the second one internally via heuristic (e.g. for Chrome, wine). When setting the command line I more or less already know most of the fields, thus a per-OS split of the "calculate-internal-fields" routine doesn't make much sense. It's extracting this routine and bring it to a common code-base that's currently the hardest part. But to facilitate this I need to have all required fields in the common code-base first, which is ATM still WIP.

@BenBE BenBE force-pushed the command_color_instr branch from f075725 to d7d7441 Compare February 16, 2021 18:22
@BenBE BenBE force-pushed the command_color_instr branch 2 times, most recently from c7f0593 to 1a46ea8 Compare March 17, 2021 21:43
BenBE and others added 22 commits May 23, 2021 02:54
This field held practically the same value as cmdlineBasenameEnd
@BenBE BenBE force-pushed the command_color_instr branch from b69258f to 88eab5f Compare May 23, 2021 00:59
@BenBE BenBE merged commit d9feff1 into htop-dev:master May 23, 2021
fraggerfox added a commit to fraggerfox/htop-dev that referenced this pull request May 23, 2021
fraggerfox added a commit to fraggerfox/htop-dev that referenced this pull request May 24, 2021
fraggerfox added a commit to fraggerfox/htop-dev that referenced this pull request Jun 12, 2021
@BenBE BenBE mentioned this pull request Nov 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code quality ♻️ Code quality enhancement enhancement Extension or improvement to existing feature help wanted Extra attention is needed needs-discussion 🤔 Changes need to be discussed and require consent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants