From af0515d05d9b8ec4f9cb0adf94f3f0c74586eedf Mon Sep 17 00:00:00 2001 From: Felipe Cardoso Date: Wed, 25 Mar 2026 20:19:44 +0100 Subject: [PATCH] fix: address code review findings (HIGH + MEDIUM) - Replace GNU \b with portable word-boundary sed patterns in kernel-params - Warn on unknown CLI arguments instead of silently swallowing - Add floor check on recommended_gttsize_mib to prevent negative values - Fix Python operator precedence in benchmark log parser - Add root checks to tuned-profile.sh and rollback.sh - Remove redundant sudo calls (scripts already require root at entry) Co-Authored-By: Claude Opus 4.6 (1M context) --- lib/detect.sh | 2 ++ scripts/benchmark/run-baseline.sh | 2 +- scripts/benchmark/run-suite.sh | 4 ++-- scripts/monitor/dashboard.sh | 2 +- scripts/optimize/kernel-params.sh | 10 +++++----- scripts/optimize/rollback.sh | 9 +++++++-- scripts/optimize/tuned-profile.sh | 7 ++++++- 7 files changed, 24 insertions(+), 12 deletions(-) diff --git a/lib/detect.sh b/lib/detect.sh index 4087952..e48f697 100644 --- a/lib/detect.sh +++ b/lib/detect.sh @@ -186,6 +186,8 @@ recommended_gttsize_mib() { total_kb="$(detect_total_physical_ram_kb)" local total_gib=$(( total_kb / 1024 / 1024 )) local gtt_gib=$(( total_gib - 4 )) + # Floor at 1 GiB to avoid negative or zero values + (( gtt_gib < 1 )) && gtt_gib=1 echo $(( gtt_gib * 1024 )) } diff --git a/scripts/benchmark/run-baseline.sh b/scripts/benchmark/run-baseline.sh index 75d6531..5e93efc 100644 --- a/scripts/benchmark/run-baseline.sh +++ b/scripts/benchmark/run-baseline.sh @@ -155,7 +155,7 @@ for logfile in sorted(result_dir.glob("*.log")): # Parse the pipe-delimited llama-bench table for line in content.splitlines(): line = line.strip() - if not line.startswith("|") or "model" in line.lower() and "size" in line.lower(): + if not line.startswith("|") or ("model" in line.lower() and "size" in line.lower()): continue if "---" in line: continue diff --git a/scripts/benchmark/run-suite.sh b/scripts/benchmark/run-suite.sh index e996cb8..0a385fe 100644 --- a/scripts/benchmark/run-suite.sh +++ b/scripts/benchmark/run-suite.sh @@ -17,7 +17,7 @@ while [[ $# -gt 0 ]]; do --tag|-t) TAG="$2"; shift 2 ;; --backends|-b) BACKENDS_FILTER="$2"; shift 2 ;; --models|-m) MODELS_FILTER="$2"; shift 2 ;; - *) shift ;; + *) log_warn "Unknown argument: $1"; shift ;; esac done @@ -147,7 +147,7 @@ for logfile in sorted(result_dir.glob("*.log")): continue for line in content.splitlines(): line = line.strip() - if not line.startswith("|") or "model" in line.lower() and "size" in line.lower(): + if not line.startswith("|") or ("model" in line.lower() and "size" in line.lower()): continue if "---" in line: continue diff --git a/scripts/monitor/dashboard.sh b/scripts/monitor/dashboard.sh index 06c1c96..220f6f1 100644 --- a/scripts/monitor/dashboard.sh +++ b/scripts/monitor/dashboard.sh @@ -13,7 +13,7 @@ while [[ $# -gt 0 ]]; do case "$1" in --simple|-s) SIMPLE=true; shift ;; --with-logging|-l) WITH_LOG=true; shift ;; - *) shift ;; + *) log_warn "Unknown argument: $1"; shift ;; esac done diff --git a/scripts/optimize/kernel-params.sh b/scripts/optimize/kernel-params.sh index e81e79a..6bd517e 100644 --- a/scripts/optimize/kernel-params.sh +++ b/scripts/optimize/kernel-params.sh @@ -93,12 +93,12 @@ with open(os.environ["GRUB_PATH"]) as f: print("") ')" -# Remove any existing values of these params +# Remove any existing values of these params (portable, no GNU \b) new_cmdline="$current_cmdline" -new_cmdline="$(echo "$new_cmdline" | sed -E 's/\biommu=[^ ]*//g')" -new_cmdline="$(echo "$new_cmdline" | sed -E 's/\bamd_iommu=[^ ]*//g')" -new_cmdline="$(echo "$new_cmdline" | sed -E 's/\bamdgpu\.gttsize=[^ ]*//g')" -new_cmdline="$(echo "$new_cmdline" | sed -E 's/\bttm\.pages_limit=[^ ]*//g')" +new_cmdline="$(echo " $new_cmdline " | sed -E 's/ iommu=[^ ]*/ /g')" +new_cmdline="$(echo " $new_cmdline " | sed -E 's/ amd_iommu=[^ ]*/ /g')" +new_cmdline="$(echo " $new_cmdline " | sed -E 's/ amdgpu\.gttsize=[^ ]*/ /g')" +new_cmdline="$(echo " $new_cmdline " | sed -E 's/ ttm\.pages_limit=[^ ]*/ /g')" # Clean up extra spaces new_cmdline="$(echo "$new_cmdline" | xargs)" diff --git a/scripts/optimize/rollback.sh b/scripts/optimize/rollback.sh index 5eeb758..65e7659 100644 --- a/scripts/optimize/rollback.sh +++ b/scripts/optimize/rollback.sh @@ -10,6 +10,11 @@ BACKUP_DIR="$(data_dir backups)" log_header "Rollback Optimizations" +if [[ $EUID -ne 0 ]]; then + log_error "This script requires root. Re-run with: sudo make rollback" + exit 1 +fi + # ── 1. GRUB rollback ──────────────────────────────────── log_info "GRUB backups:" mapfile -t grub_backups < <(find "$BACKUP_DIR" -name 'grub-*.bak' -print 2>/dev/null | sort -r) @@ -23,7 +28,7 @@ else echo "" if confirm "Restore most recent GRUB backup?"; then - require_root + # root already checked at script start backup="${grub_backups[0]}" cp "$backup" "$GRUB_FILE" log_success "GRUB restored from: $backup" @@ -54,7 +59,7 @@ if [[ -f "$prev_profile_file" ]]; then log_info "Tuned profile: $current (previous: $prev_profile)" if [[ "$current" != "$prev_profile" ]] && confirm "Restore tuned profile to $prev_profile?"; then - sudo tuned-adm profile "$prev_profile" + tuned-adm profile "$prev_profile" log_success "Tuned profile restored to: $prev_profile" fi else diff --git a/scripts/optimize/tuned-profile.sh b/scripts/optimize/tuned-profile.sh index 21a5f3f..48d51a7 100644 --- a/scripts/optimize/tuned-profile.sh +++ b/scripts/optimize/tuned-profile.sh @@ -10,6 +10,11 @@ RECOMMENDED="accelerator-performance" log_header "Tuned Profile Optimization" +if [[ $EUID -ne 0 ]]; then + log_error "This script requires root. Re-run with: sudo make optimize-tuned" + exit 1 +fi + if ! is_cmd tuned-adm; then log_error "tuned is not installed. Install with: sudo dnf install tuned" exit 1 @@ -46,7 +51,7 @@ fi # Save current for rollback echo "$current" > "$(data_dir backups)/tuned-previous-profile.txt" -sudo tuned-adm profile "$RECOMMENDED" +tuned-adm profile "$RECOMMENDED" new_profile="$(detect_tuned_profile)" if [[ "$new_profile" == "$RECOMMENDED" ]]; then