Remove obsolete OpenVZ and VServer support#2010
Conversation
BenBE
added
documentation 📖
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
💤 Files with no reviewable changes (2)
📝 WalkthroughWalkthroughThis PR removes OpenVZ, VServer, and ancient-VServer support from htop. Configure options and their AC_DEFINE macro emissions are deleted from configure.ac. CI jobs (.github/workflows/ci.yml and codeql-analysis.yml) no longer pass those flags to ./configure. Process field enums CTID, VPID, VXID are removed from linux/ProcessField.h and pcp/ProcessField.h. LinuxProcess struct drops the conditional ctid, vpid, vxid members; PROCESS_FLAG_LINUX_OPENVZ and PROCESS_FLAG_LINUX_VSERVER flags are removed and PROCESS_FLAG_LINUX_CGROUP is introduced. LinuxProcessTable parsing no longer initializes or reads VServer/OpenVZ-specific fields from /proc status files; the readOpenVZData() helper is deleted. LinuxProcess rendering and comparison code drops switch cases for CTID/VPID/VXID. README and the man page remove references; cppcheck no longer defines HAVE_OPENVZ. Poem
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. Comment |
|
FYI: Archlinux builds htop with openvz enabled: https://gitlab.archlinux.org/archlinux/packaging/packages/htop/-/blob/main/PKGBUILD?ref_type=heads#L62 |
|
AFAIT that's basically Arch's packaging default. Maybe a quick breakdown:
Given that Arch only ships mainline kernels as far as I'm aware, this means that having these features active on Arch is mostly a no-op. All the code removed in this PR requires ancient kernel-patch based virtualization stuff, that will simply not work with any recent kernel. |
natoscott
left a comment
natoscott
left a comment
There was a problem hiding this comment.
Looks good - there's a spurious left-over CTID definition in pcp/ProcessField.h (never used) - could you remove that as well before merging? Thanks!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
57-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSet explicit minimal GitHub token permissions
These jobs run with implicit default token permissions. Please set explicit least-privilege permissions (for these build-only jobs,
contents: readis typically enough) to avoid over-scopedGITHUB_TOKENaccess.Suggested change
name: CI on: [ push, pull_request ] +permissions: + contents: readAlso applies to: 78-100, 102-121
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b496e1a7-e8d7-4b1e-b23b-96002eefa38f
📒 Files selected for processing (10)
.github/workflows/ci.ymlMakefile.amREADME.mdconfigure.achtop.1.inlinux/LinuxProcess.clinux/LinuxProcess.hlinux/LinuxProcessTable.clinux/ProcessField.hpcp/ProcessField.h
💤 Files with no reviewable changes (8)
- htop.1.in
- linux/ProcessField.h
- linux/LinuxProcess.h
- pcp/ProcessField.h
- configure.ac
- linux/LinuxProcess.c
- linux/LinuxProcessTable.c
- README.md
BenBE
force-pushed
the
remove-openvz
branch
2 times, most recently
from
8ee3b0c to
0c562a9
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/ci.yml (1)
57-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit minimal GitHub token permissions for this workflow.
These jobs still inherit default token permissions because no
permissions:block is set. Please declare least-privilege permissions at workflow level (and override per job only when needed).Proposed hardening patch
name: CI on: [ push, pull_request ] +permissions: + contents: read + env: # Enable format attributes in ncurses headers # Enable fortified memory/string handling CPPFLAGS: -DGCC_PRINTF -DGCC_SCANF -D_FORTIFY_SOURCE=2Also applies to: 78-100, 102-121
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0b6a92e6-0021-4a2a-abb5-c7ef1f3b6b56
📒 Files selected for processing (10)
.github/workflows/ci.ymlMakefile.amREADME.mdconfigure.achtop.1.inlinux/LinuxProcess.clinux/LinuxProcess.hlinux/LinuxProcessTable.clinux/ProcessField.hpcp/ProcessField.h
💤 Files with no reviewable changes (8)
- README.md
- pcp/ProcessField.h
- htop.1.in
- linux/LinuxProcessTable.c
- linux/LinuxProcess.c
- linux/ProcessField.h
- configure.ac
- linux/LinuxProcess.h
This commit fully removes all of the OpenVZ and Linux VServer legacy that has been dormant and unused for the past few years. The code has been disabled by default for several releases now and has not been in use for quite some time. Assisted-By: Claude Haiku 4.5
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d669da07-8ad4-42e0-9038-1000bc40285f
📒 Files selected for processing (11)
.github/workflows/ci.yml.github/workflows/codeql-analysis.ymlMakefile.amREADME.mdconfigure.achtop.1.inlinux/LinuxProcess.clinux/LinuxProcess.hlinux/LinuxProcessTable.clinux/ProcessField.hpcp/ProcessField.h
💤 Files with no reviewable changes (2)
- pcp/ProcessField.h
- README.md
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d669da07-8ad4-42e0-9038-1000bc40285f
📒 Files selected for processing (11)
.github/workflows/ci.yml.github/workflows/codeql-analysis.ymlMakefile.amREADME.mdconfigure.achtop.1.inlinux/LinuxProcess.clinux/LinuxProcess.hlinux/LinuxProcessTable.clinux/ProcessField.hpcp/ProcessField.h
💤 Files with no reviewable changes (2)
- pcp/ProcessField.h
- README.md
🛑 Comments failed to post (2)
.github/workflows/ci.yml (1)
17-17: 🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify all workflow checkout steps are commit-pinned and set persist-credentials:false set -euo pipefail echo "== Unpinned checkout uses ==" rg -n --glob '.github/workflows/*.yml' 'uses:\s*actions/checkout@v[0-9]+' || true echo echo "== Checkout steps missing persist-credentials:false (context view) ==" rg -n -C2 --glob '.github/workflows/*.yml' 'uses:\s*actions/checkout@' || trueRepository: htop-dev/htop
Length of output: 180
🏁 Script executed:
# First, check if the workflow file exists and get its size ls -lh .github/workflows/ci.yml 2>/dev/null || echo "File not found" # Read around line 17 to see the actual checkout step echo "=== Lines 10-25 (around line 17) ===" sed -n '10,25p' .github/workflows/ci.yml # Search for all checkout references more directly echo "" echo "=== All checkout references ===" grep -n "checkout" .github/workflows/ci.yml || echo "No checkout found"Repository: htop-dev/htop
Length of output: 1511
🏁 Script executed:
# Check if any of the checkout steps have persist-credentials configured echo "=== Checkout steps with persist-credentials context ===" grep -B2 -A3 "persist-credentials" .github/workflows/ci.yml || echo "No persist-credentials found" # Get the full content around a few checkout steps to see if there's a 'with:' clause echo "" echo "=== Detailed view of first checkout (lines 17-22) ===" sed -n '17,22p' .github/workflows/ci.yml echo "" echo "=== Detailed view of another checkout example (lines 41-46) ===" sed -n '41,46p' .github/workflows/ci.ymlRepository: htop-dev/htop
Length of output: 954
Pin checkout actions to commit SHA and disable credential persistence.
All 18 checkout steps use
actions/checkout@v7(tag-pinned), which is vulnerable to tag mutation attacks, and none configurepersist-credentials: false. Both issues weaken workflow supply-chain security.Recommended hardening pattern
- - uses: actions/checkout@v7 + - uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 + with: + persist-credentials: falseApply to all 18 occurrences: lines 17, 41, 67, 86, 112, 130, 146, 163, 191, 223, 242, 261, 284, 305, 330, 365, 385, 392.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.- uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 with: persist-credentials: false🧰 Tools
🪛 zizmor (1.26.1)
[warning] 17-17: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 17-17: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
Source: Linters/SAST tools
.github/workflows/codeql-analysis.yml (1)
29-29: 🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n --glob '.github/workflows/codeql-analysis.yml' 'uses:\s*actions/checkout@'Repository: htop-dev/htop
Length of output: 151
🏁 Script executed:
#!/bin/bash # Check if the workflow file exists and list GitHub workflow files find . -path "./.git" -prune -o -type f -name "*.yml" -o -name "*.yaml" | grep -i workflow | head -20Repository: htop-dev/htop
Length of output: 289
🏁 Script executed:
#!/bin/bash # Also check if .github directory exists and list its contents if [ -d .github ]; then echo "=== .github directory structure ===" find .github -type f | sort else echo ".github directory not found" fiRepository: htop-dev/htop
Length of output: 385
🏁 Script executed:
#!/bin/bash # Try reading the specific file if it exists if [ -f ".github/workflows/codeql-analysis.yml" ]; then echo "=== File exists, showing content around line 29 ===" cat -n ".github/workflows/codeql-analysis.yml" | sed -n '20,40p' else echo "File .github/workflows/codeql-analysis.yml not found" fiRepository: htop-dev/htop
Length of output: 956
Pin checkout action to commit SHA and disable credential persistence.
At line 29, replace
actions/checkout@v7with a full commit SHA and addpersist-credentials: falseto reduce token exposure in CI artifacts:- name: Checkout Repository uses: actions/checkout@<full-commit-sha> with: persist-credentials: false🧰 Tools
🪛 zizmor (1.26.1)
[warning] 28-29: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 29-29: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
Source: Linters/SAST tools
|
@coderabbitai These CI issues are unrelated to the original OpenVZ/OpenVServer changes of this PR. Should be addressed in a separate PR. |
|
Tip For best results, initiate chat on the files or code changes.
|
@coderabbitai Sure, but note that fetching by SHA is not the advertised usage pattern of the upstream https://github.com/actions/checkout repo. Also when suggesting security hardening, elaborate the benefits and mitigated risks for better assessment. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cat -n .github/workflows/ci.ymlLength of output: 18978 🏁 Script executed: #!/bin/bash
gh issue create \
--repo htop-dev/htop \
--title "CI: Replace deprecated apt-key and fix shell style in workflow jobs" \
--body "## Summary
This issue tracks CI maintenance items identified in \`.github/workflows/ci.yml\` that are unrelated to any specific feature change. Flagged during review of `#2010`.
---
## 1. Deprecated \`apt-key add\` for LLVM APT repository
**Affected jobs:** \`build-ubuntu-latest-minimal-clang\`, \`build-ubuntu-latest-full-featured-clang\`, \`build-ubuntu-latest-clang-analyzer\`, \`build-ubuntu-latest-clang-sanitizer\`
**Current pattern (all four jobs, e.g. line 45):**
\`\`\`sh
wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key 2>/dev/null | sudo apt-key add -
sudo add-apt-repository \"deb http://apt.llvm.org/\${ubuntu_codename}/ llvm-toolchain-\${ubuntu_codename}-22 main\" -y
\`\`\`
**Problem:**
- \`apt-key\` has been deprecated since Debian Bullseye / Ubuntu 22.04. Running it produces deprecation warnings and may break silently on future runner images.
- More importantly, keys added via \`apt-key\` are trusted globally for *all* repositories, not scoped to the specific source. A compromised or malicious key could therefore be leveraged to sign packages from unrelated repositories and have them accepted silently by \`apt\`.
**Recommended replacement:** Use a signed-by keyrings approach:
\`\`\`sh
wget -qO - https://apt.llvm.org/llvm-snapshot.gpg.key \
| gpg --dearmor \
| sudo tee /etc/apt/keyrings/llvm.gpg > /dev/null
echo \"deb [signed-by=/etc/apt/keyrings/llvm.gpg] http://apt.llvm.org/\${ubuntu_codename}/ llvm-toolchain-\${ubuntu_codename}-22 main\" \
| sudo tee /etc/apt/sources.list.d/llvm.list
\`\`\`
**Security benefit:** The \`signed-by\` field scopes trust to the declared repository only. Even if the LLVM key were ever reused or compromised, it cannot be exploited to verify packages from other apt sources. This follows the current Debian/Ubuntu best practice and eliminates the deprecation warnings.
---
## 2. Backtick command substitution
**Affected jobs:** \`build-ubuntu-latest-minimal-clang\`, \`build-ubuntu-latest-full-featured-clang\`, \`build-ubuntu-latest-clang-analyzer\`, \`build-ubuntu-latest-clang-sanitizer\`
**Current pattern (e.g. line 44):**
\`\`\`sh
ubuntu_codename=\`LC_ALL=C sed '...' /etc/os-release\`
\`\`\`
**Problem:** Backtick syntax is a legacy POSIX-ish form that does not nest cleanly and is harder to read. The POSIX and bash-recommended form is \`\$()\`.
**Recommended replacement:**
\`\`\`sh
ubuntu_codename=\$(LC_ALL=C sed '...' /etc/os-release)
\`\`\`
---
## Acceptance criteria
- [ ] All four Clang CI jobs use the \`signed-by\`-scoped keyrings approach for the LLVM APT key.
- [ ] All backtick command substitutions in the workflow are replaced with \`\$()\`.
- [ ] CI jobs pass after the changes.
---
*Reported by `@coderabbitai` during review of `#2010` (requested by `@BenBE`).*
*Reference comment: https://github.com/htop-dev/htop/pull/2010#issuecomment-2905777696*"Length of output: 3190
✏️ Learnings added
|
3 participants
This PR fully removes all of the OpenVZ and Linux VServer legacy that has been dormant and unused for the past few years.
The code has been disabled by default for several releases now and has not been in use for quite some time.
Assisted-By: Claude Haiku 4.5