Open
Conversation
📝 WalkthroughWalkthroughThis pull request extends MoCI to support Lighttpd alongside uhttpd, enabling deployment on TurrisOS and similar distributions. It adds Lighttpd configuration, a JSON-RPC CGI bridge for ubus communication, and ARM VM provisioning scripts for testing. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
Makefile (1)
53-55: Remove duplicateINSTALL_DIRcall.Line 54 duplicates the
$(INSTALL_DIR) $(1)/www/mocicall already made on line 31.Proposed fix
$(INSTALL_CONF) ./files/moci.config $(1)/etc/config/moci - $(INSTALL_DIR) $(1)/www/moci $(INSTALL_BIN) ./files/ubus.cgi $(1)/www/moci/ubus.cgi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 53 - 55, Remove the duplicate directory creation call by deleting the redundant "$(INSTALL_DIR) $(1)/www/moci" invocation (the one adjacent to "$(INSTALL_BIN) ./files/ubus.cgi $(1)/www/moci/ubus.cgi"); keep the original "$(INSTALL_DIR) $(1)/www/moci" earlier in the Makefile and leave "$(INSTALL_BIN) ./files/ubus.cgi $(1)/www/moci/ubus.cgi" untouched so INSTALL_DIR is only called once for the www/moci directory.scripts/setup-qemu-arm.sh (1)
13-16: Script assumes macOS with Homebrew.The QEMU installation step only supports Homebrew. Consider documenting this in the script or README, or adding a fallback message for other platforms.
Suggested improvement
if ! command -v qemu-system-arm &> /dev/null; then - echo "QEMU ARM not found. Installing via Homebrew..." - brew install qemu + if command -v brew &> /dev/null; then + echo "QEMU ARM not found. Installing via Homebrew..." + brew install qemu + else + echo "QEMU ARM not found. Please install qemu-system-arm manually." + echo " macOS: brew install qemu" + echo " Ubuntu/Debian: apt install qemu-system-arm" + exit 1 + fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/setup-qemu-arm.sh` around lines 13 - 16, The script currently checks for qemu-system-arm and unconditionally runs "brew install qemu" which assumes macOS/Homebrew; update the check in scripts/setup-qemu-arm.sh to detect the OS (e.g., via uname) and branch: on macOS use "brew install qemu", on Debian/Ubuntu use "apt-get install -y qemu-system-arm" (or instruct to use apt), on RHEL/CentOS use "yum/dnf install qemu-system-arm", on Arch use "pacman -S qemu" and for unknown platforms print a clear fallback message instructing the user how to install qemu manually; ensure the code path that calls command -v qemu-system-arm still handles failures and outputs the platform-specific guidance instead of always calling brew.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@files/lighttpd-moci.conf`:
- Line 1: The config loads server.modules with "mod_cgi" and "mod_setenv" but
uses alias.url directives, so add "mod_alias" to server.modules to enable
alias.url; update the server.modules array (the line that defines
server.modules) to include "mod_alias" alongside "mod_cgi" and "mod_setenv" so
the alias.url directives will be recognized by lighttpd.
In `@files/ubus.cgi`:
- Around line 96-103: The `service` value extracted from RPC_PARAMS is used
directly in the `ubus -v list "$service"` call, enabling command injection;
update the `list` handler to validate/sanitize the `service` variable (the value
returned by jsonfilter into the service variable) before invoking `ubus -v
list`, using the same whitelist/validation approach as the `call` method (allow
only a safe charset such as [A-Za-z0-9._-] or reject/return an error for invalid
input) and ensure the code returns an error for invalid service names instead of
calling `ubus` with untrusted content.
- Around line 54-58: The current concatenation assumes UBUS_PAYLOAD begins with
"{ " and uses ${UBUS_PAYLOAD#\{ } which fails if there's no space; change the
assembly so you robustly strip only the leading '{' and any amount of whitespace
before appending the rest: compute the payload body by removing the first '{'
character and trimming any leading whitespace from that remainder, then build
UBUS_PAYLOAD with printf '{ "ubus_rpc_session": "%s", %s' "$UBUS_SID"
"$payload_body"; ensure this logic is used where UBUS_PAYLOAD and UBUS_SID are
handled so the resulting JSON is always well-formed regardless of jsonfilter
spacing.
In `@scripts/setup-lighttpd-vm.sh`:
- Around line 20-28: The script uses hardcoded source prefixes "moci/" while the
Makefile uses "./dist/moci/"; update the source paths in the lines that run cat
(those containing "cat moci/... | ${SSH} \"cat > /www/moci/...\"") to point to
the correct build output (e.g., replace "moci/" with "./dist/moci/") or
introduce a single variable (e.g., DIST_SRC) and use it for all source paths so
the deployment sources match the Makefile output.
In `@scripts/setup-qemu-arm.sh`:
- Around line 22-26: The download step can fail but the script will continue to
extraction; update the curl invocation that fetches
"${BASE_URL}/openwrt-${OPENWRT_VERSION}-armsr-armv7-generic-ext4-combined-efi.img.gz"
(writing to "${IMAGE_DIR}/combined.img.gz") to fail on HTTP errors so the script
aborts on download failure (e.g., enable the -f behavior or check curl's exit
status) and ensure any subsequent commands that reference
"${IMAGE_DIR}/combined.img.gz" or "${IMAGE_FILE}" only run when the download
succeeded.
In `@scripts/start-vm-arm.sh`:
- Line 25: The hardcoded BIOS path in scripts/start-vm-arm.sh (the -bios
/opt/homebrew/share/qemu/edk2-arm-code.fd argument) is macOS ARM specific;
change the script to accept a configurable BIOS path (e.g., read BIOS_PATH or
QEMU_BIOS env var) and fall back to auto-detection based on the platform (uname)
by checking common locations (/opt/homebrew/share/qemu/, /usr/local/share/qemu/,
/usr/share/qemu/, /usr/share/AAVMF/) before invoking qemu-system-aarch64; update
the script to use that resolved BIOS path variable for the -bios argument and
emit a clear error if no valid file is found.
---
Nitpick comments:
In `@Makefile`:
- Around line 53-55: Remove the duplicate directory creation call by deleting
the redundant "$(INSTALL_DIR) $(1)/www/moci" invocation (the one adjacent to
"$(INSTALL_BIN) ./files/ubus.cgi $(1)/www/moci/ubus.cgi"); keep the original
"$(INSTALL_DIR) $(1)/www/moci" earlier in the Makefile and leave "$(INSTALL_BIN)
./files/ubus.cgi $(1)/www/moci/ubus.cgi" untouched so INSTALL_DIR is only called
once for the www/moci directory.
In `@scripts/setup-qemu-arm.sh`:
- Around line 13-16: The script currently checks for qemu-system-arm and
unconditionally runs "brew install qemu" which assumes macOS/Homebrew; update
the check in scripts/setup-qemu-arm.sh to detect the OS (e.g., via uname) and
branch: on macOS use "brew install qemu", on Debian/Ubuntu use "apt-get install
-y qemu-system-arm" (or instruct to use apt), on RHEL/CentOS use "yum/dnf
install qemu-system-arm", on Arch use "pacman -S qemu" and for unknown platforms
print a clear fallback message instructing the user how to install qemu
manually; ensure the code path that calls command -v qemu-system-arm still
handles failures and outputs the platform-specific guidance instead of always
calling brew.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7f71d0b-2a75-4b7f-84c0-5f9874e84a71
📒 Files selected for processing (6)
Makefilefiles/lighttpd-moci.conffiles/ubus.cgiscripts/setup-lighttpd-vm.shscripts/setup-qemu-arm.shscripts/start-vm-arm.sh
| @@ -0,0 +1,12 @@ | |||
| server.modules += ( "mod_cgi", "mod_setenv" ) | |||
There was a problem hiding this comment.
Missing mod_alias in server.modules.
The configuration uses alias.url directives (lines 4 and 9) but only loads mod_cgi and mod_setenv. The mod_alias module is required for alias.url to work.
Proposed fix
-server.modules += ( "mod_cgi", "mod_setenv" )
+server.modules += ( "mod_alias", "mod_cgi", "mod_setenv" )📝 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.
Suggested change
| server.modules += ( "mod_cgi", "mod_setenv" ) | |
| server.modules += ( "mod_alias", "mod_cgi", "mod_setenv" ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@files/lighttpd-moci.conf` at line 1, The config loads server.modules with
"mod_cgi" and "mod_setenv" but uses alias.url directives, so add "mod_alias" to
server.modules to enable alias.url; update the server.modules array (the line
that defines server.modules) to include "mod_alias" alongside "mod_cgi" and
"mod_setenv" so the alias.url directives will be recognized by lighttpd.
Comment on lines
+54
to
+58
| if [ -z "$UBUS_PAYLOAD" ] || [ "$UBUS_PAYLOAD" = "{ }" ]; then | ||
| UBUS_PAYLOAD=$(printf '{ "ubus_rpc_session": "%s" }' "$UBUS_SID") | ||
| else | ||
| UBUS_PAYLOAD=$(printf '{ "ubus_rpc_session": "%s", %s' "$UBUS_SID" "${UBUS_PAYLOAD#\{ }") | ||
| fi |
There was a problem hiding this comment.
Fragile JSON string manipulation.
The pattern ${UBUS_PAYLOAD#\{ } assumes jsonfilter outputs { (brace-space), but JSON output formatting may vary. If jsonfilter outputs {"key":"value"} (no space), the substitution won't match and the result will be malformed JSON.
More robust approach
if [ -z "$UBUS_PAYLOAD" ] || [ "$UBUS_PAYLOAD" = "{ }" ]; then
UBUS_PAYLOAD=$(printf '{ "ubus_rpc_session": "%s" }' "$UBUS_SID")
else
- UBUS_PAYLOAD=$(printf '{ "ubus_rpc_session": "%s", %s' "$UBUS_SID" "${UBUS_PAYLOAD#\{ }")
+ # Strip leading { with optional whitespace
+ payload_inner=$(echo "$UBUS_PAYLOAD" | sed 's/^{[[:space:]]*//')
+ UBUS_PAYLOAD=$(printf '{ "ubus_rpc_session": "%s", %s' "$UBUS_SID" "$payload_inner")
fi📝 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.
Suggested change
| if [ -z "$UBUS_PAYLOAD" ] || [ "$UBUS_PAYLOAD" = "{ }" ]; then | |
| UBUS_PAYLOAD=$(printf '{ "ubus_rpc_session": "%s" }' "$UBUS_SID") | |
| else | |
| UBUS_PAYLOAD=$(printf '{ "ubus_rpc_session": "%s", %s' "$UBUS_SID" "${UBUS_PAYLOAD#\{ }") | |
| fi | |
| if [ -z "$UBUS_PAYLOAD" ] || [ "$UBUS_PAYLOAD" = "{ }" ]; then | |
| UBUS_PAYLOAD=$(printf '{ "ubus_rpc_session": "%s" }' "$UBUS_SID") | |
| else | |
| # Strip leading { with optional whitespace | |
| payload_inner=$(echo "$UBUS_PAYLOAD" | sed 's/^{[[:space:]]*//') | |
| UBUS_PAYLOAD=$(printf '{ "ubus_rpc_session": "%s", %s' "$UBUS_SID" "$payload_inner") | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@files/ubus.cgi` around lines 54 - 58, The current concatenation assumes
UBUS_PAYLOAD begins with "{ " and uses ${UBUS_PAYLOAD#\{ } which fails if
there's no space; change the assembly so you robustly strip only the leading '{'
and any amount of whitespace before appending the rest: compute the payload body
by removing the first '{' character and trimming any leading whitespace from
that remainder, then build UBUS_PAYLOAD with printf '{ "ubus_rpc_session": "%s",
%s' "$UBUS_SID" "$payload_body"; ensure this logic is used where UBUS_PAYLOAD
and UBUS_SID are handled so the resulting JSON is always well-formed regardless
of jsonfilter spacing.
Comment on lines
+96
to
+103
| for i in $indexes; do | ||
| service=$(jsonfilter -s "$RPC_PARAMS" -e "@[$i]") | ||
| signature='' | ||
|
|
||
| IFS=$'\n\t' | ||
| for line in $(ubus -v list "$service" | tail -n +2); do | ||
| signature="${signature:+$signature, }$line" | ||
| done |
There was a problem hiding this comment.
Potential command injection in list method: $service is not validated.
The $service variable comes from user-provided RPC_PARAMS (line 97) and is passed directly to ubus -v list "$service" (line 101) without character validation like the call method has on lines 41-43.
A malicious params array like ["$(rm -rf /)"] could execute arbitrary commands.
Proposed fix
for i in $indexes; do
service=$(jsonfilter -s "$RPC_PARAMS" -e "@[$i]")
+
+ case "$service" in
+ *[^a-zA-Z0-9_.-]*) error -32602 "Invalid parameters" ;;
+ esac
+
signature=''📝 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.
Suggested change
| for i in $indexes; do | |
| service=$(jsonfilter -s "$RPC_PARAMS" -e "@[$i]") | |
| signature='' | |
| IFS=$'\n\t' | |
| for line in $(ubus -v list "$service" | tail -n +2); do | |
| signature="${signature:+$signature, }$line" | |
| done | |
| for i in $indexes; do | |
| service=$(jsonfilter -s "$RPC_PARAMS" -e "@[$i]") | |
| case "$service" in | |
| *[^a-zA-Z0-9_.-]*) error -32602 "Invalid parameters" ;; | |
| esac | |
| signature='' | |
| IFS=$'\n\t' | |
| for line in $(ubus -v list "$service" | tail -n +2); do | |
| signature="${signature:+$signature, }$line" | |
| done |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@files/ubus.cgi` around lines 96 - 103, The `service` value extracted from
RPC_PARAMS is used directly in the `ubus -v list "$service"` call, enabling
command injection; update the `list` handler to validate/sanitize the `service`
variable (the value returned by jsonfilter into the service variable) before
invoking `ubus -v list`, using the same whitelist/validation approach as the
`call` method (allow only a safe charset such as [A-Za-z0-9._-] or reject/return
an error for invalid input) and ensure the code returns an error for invalid
service names instead of calling `ubus` with untrusted content.
Comment on lines
+20
to
+28
| cat moci/index.html | ${SSH} "cat > /www/moci/index.html" | ||
| cat moci/app.css | ${SSH} "cat > /www/moci/app.css" | ||
| cat moci/manifest.json | ${SSH} "cat > /www/moci/manifest.json" | ||
| cat moci/icons/icon-192.png | ${SSH} "cat > /www/moci/icons/icon-192.png" | ||
| cat moci/icons/icon-512.png | ${SSH} "cat > /www/moci/icons/icon-512.png" | ||
| cat moci/js/core.js | ${SSH} "cat > /www/moci/js/core.js" | ||
| cat moci/js/modules/dashboard.js | ${SSH} "cat > /www/moci/js/modules/dashboard.js" | ||
| cat moci/js/modules/network.js | ${SSH} "cat > /www/moci/js/modules/network.js" | ||
| cat moci/js/modules/system.js | ${SSH} "cat > /www/moci/js/modules/system.js" |
There was a problem hiding this comment.
Path inconsistency: script uses moci/ but Makefile uses ./dist/moci/.
The file deployment uses paths like moci/index.html, but the Makefile installs from ./dist/moci/. This script won't find files unless:
- A
moci/symlink exists pointing todist/moci/, or - The build output directory structure differs from expectations.
Verify the intended source paths or align with the Makefile's ./dist/moci/ paths.
Proposed fix (if dist/moci is correct)
echo "Deploying MoCI files..."
-cat moci/index.html | ${SSH} "cat > /www/moci/index.html"
-cat moci/app.css | ${SSH} "cat > /www/moci/app.css"
-cat moci/manifest.json | ${SSH} "cat > /www/moci/manifest.json"
-cat moci/icons/icon-192.png | ${SSH} "cat > /www/moci/icons/icon-192.png"
-cat moci/icons/icon-512.png | ${SSH} "cat > /www/moci/icons/icon-512.png"
-cat moci/js/core.js | ${SSH} "cat > /www/moci/js/core.js"
-cat moci/js/modules/dashboard.js | ${SSH} "cat > /www/moci/js/modules/dashboard.js"
-cat moci/js/modules/network.js | ${SSH} "cat > /www/moci/js/modules/network.js"
-cat moci/js/modules/system.js | ${SSH} "cat > /www/moci/js/modules/system.js"
+cat dist/moci/index.html | ${SSH} "cat > /www/moci/index.html"
+cat dist/moci/app.css | ${SSH} "cat > /www/moci/app.css"
+cat dist/moci/manifest.json | ${SSH} "cat > /www/moci/manifest.json"
+cat dist/moci/icons/icon-192.png | ${SSH} "cat > /www/moci/icons/icon-192.png"
+cat dist/moci/icons/icon-512.png | ${SSH} "cat > /www/moci/icons/icon-512.png"
+cat dist/moci/js/core.js | ${SSH} "cat > /www/moci/js/core.js"
+cat dist/moci/js/modules/dashboard.js | ${SSH} "cat > /www/moci/js/modules/dashboard.js"
+cat dist/moci/js/modules/network.js | ${SSH} "cat > /www/moci/js/modules/network.js"
+cat dist/moci/js/modules/system.js | ${SSH} "cat > /www/moci/js/modules/system.js"📝 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.
Suggested change
| cat moci/index.html | ${SSH} "cat > /www/moci/index.html" | |
| cat moci/app.css | ${SSH} "cat > /www/moci/app.css" | |
| cat moci/manifest.json | ${SSH} "cat > /www/moci/manifest.json" | |
| cat moci/icons/icon-192.png | ${SSH} "cat > /www/moci/icons/icon-192.png" | |
| cat moci/icons/icon-512.png | ${SSH} "cat > /www/moci/icons/icon-512.png" | |
| cat moci/js/core.js | ${SSH} "cat > /www/moci/js/core.js" | |
| cat moci/js/modules/dashboard.js | ${SSH} "cat > /www/moci/js/modules/dashboard.js" | |
| cat moci/js/modules/network.js | ${SSH} "cat > /www/moci/js/modules/network.js" | |
| cat moci/js/modules/system.js | ${SSH} "cat > /www/moci/js/modules/system.js" | |
| echo "Deploying MoCI files..." | |
| cat dist/moci/index.html | ${SSH} "cat > /www/moci/index.html" | |
| cat dist/moci/app.css | ${SSH} "cat > /www/moci/app.css" | |
| cat dist/moci/manifest.json | ${SSH} "cat > /www/moci/manifest.json" | |
| cat dist/moci/icons/icon-192.png | ${SSH} "cat > /www/moci/icons/icon-192.png" | |
| cat dist/moci/icons/icon-512.png | ${SSH} "cat > /www/moci/icons/icon-512.png" | |
| cat dist/moci/js/core.js | ${SSH} "cat > /www/moci/js/core.js" | |
| cat dist/moci/js/modules/dashboard.js | ${SSH} "cat > /www/moci/js/modules/dashboard.js" | |
| cat dist/moci/js/modules/network.js | ${SSH} "cat > /www/moci/js/modules/network.js" | |
| cat dist/moci/js/modules/system.js | ${SSH} "cat > /www/moci/js/modules/system.js" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/setup-lighttpd-vm.sh` around lines 20 - 28, The script uses hardcoded
source prefixes "moci/" while the Makefile uses "./dist/moci/"; update the
source paths in the lines that run cat (those containing "cat moci/... | ${SSH}
\"cat > /www/moci/...\"") to point to the correct build output (e.g., replace
"moci/" with "./dist/moci/") or introduce a single variable (e.g., DIST_SRC) and
use it for all source paths so the deployment sources match the Makefile output.
Comment on lines
+22
to
+26
| curl -L "${BASE_URL}/openwrt-${OPENWRT_VERSION}-armsr-armv7-generic-ext4-combined-efi.img.gz" -o "${IMAGE_DIR}/combined.img.gz" | ||
|
|
||
| echo "Extracting image..." | ||
| gunzip "${IMAGE_DIR}/combined.img.gz" | ||
| mv "${IMAGE_DIR}/combined.img" "${IMAGE_FILE}" |
There was a problem hiding this comment.
Add download verification before extraction.
If curl fails (network error, 404, etc.), the script continues to gunzip a potentially missing or corrupt file. Add a check for download success.
Proposed fix
echo "Downloading OpenWrt ${OPENWRT_VERSION} ARM combined image..."
- curl -L "${BASE_URL}/openwrt-${OPENWRT_VERSION}-armsr-armv7-generic-ext4-combined-efi.img.gz" -o "${IMAGE_DIR}/combined.img.gz"
+ curl -fL "${BASE_URL}/openwrt-${OPENWRT_VERSION}-armsr-armv7-generic-ext4-combined-efi.img.gz" -o "${IMAGE_DIR}/combined.img.gz"
+
+ if [ ! -f "${IMAGE_DIR}/combined.img.gz" ]; then
+ echo "Download failed. Please check network and URL."
+ exit 1
+ fi
echo "Extracting image..."The -f flag makes curl return a non-zero exit code on HTTP errors, which combined with set -e will abort the script on download failure.
📝 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.
Suggested change
| curl -L "${BASE_URL}/openwrt-${OPENWRT_VERSION}-armsr-armv7-generic-ext4-combined-efi.img.gz" -o "${IMAGE_DIR}/combined.img.gz" | |
| echo "Extracting image..." | |
| gunzip "${IMAGE_DIR}/combined.img.gz" | |
| mv "${IMAGE_DIR}/combined.img" "${IMAGE_FILE}" | |
| curl -fL "${BASE_URL}/openwrt-${OPENWRT_VERSION}-armsr-armv7-generic-ext4-combined-efi.img.gz" -o "${IMAGE_DIR}/combined.img.gz" | |
| if [ ! -f "${IMAGE_DIR}/combined.img.gz" ]; then | |
| echo "Download failed. Please check network and URL." | |
| exit 1 | |
| fi | |
| echo "Extracting image..." | |
| gunzip "${IMAGE_DIR}/combined.img.gz" | |
| mv "${IMAGE_DIR}/combined.img" "${IMAGE_FILE}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/setup-qemu-arm.sh` around lines 22 - 26, The download step can fail
but the script will continue to extraction; update the curl invocation that
fetches
"${BASE_URL}/openwrt-${OPENWRT_VERSION}-armsr-armv7-generic-ext4-combined-efi.img.gz"
(writing to "${IMAGE_DIR}/combined.img.gz") to fail on HTTP errors so the script
aborts on download failure (e.g., enable the -f behavior or check curl's exit
status) and ensure any subsequent commands that reference
"${IMAGE_DIR}/combined.img.gz" or "${IMAGE_FILE}" only run when the download
succeeded.
| -M virt \ | ||
| -cpu cortex-a15 \ | ||
| -m 256M \ | ||
| -bios /opt/homebrew/share/qemu/edk2-arm-code.fd \ |
There was a problem hiding this comment.
Hardcoded BIOS path is macOS ARM (Apple Silicon) specific.
The path /opt/homebrew/share/qemu/edk2-arm-code.fd only works on Apple Silicon Macs. Intel Macs use /usr/local/share/qemu/ and Linux systems have different paths (e.g., /usr/share/qemu/ or /usr/share/AAVMF/).
Consider making this configurable or auto-detecting the platform.
Proposed fix
+# Find EDK2 BIOS - check common locations
+BIOS_PATHS=(
+ "/opt/homebrew/share/qemu/edk2-arm-code.fd" # macOS ARM (Homebrew)
+ "/usr/local/share/qemu/edk2-arm-code.fd" # macOS Intel (Homebrew)
+ "/usr/share/qemu/edk2-arm-code.fd" # Linux
+ "/usr/share/AAVMF/AAVMF_CODE.fd" # Debian/Ubuntu
+)
+
+BIOS=""
+for path in "${BIOS_PATHS[@]}"; do
+ if [ -f "$path" ]; then
+ BIOS="$path"
+ break
+ fi
+done
+
+if [ -z "$BIOS" ]; then
+ echo "EDK2 ARM BIOS not found. Please install QEMU with EFI support."
+ exit 1
+fi
+
qemu-system-arm \
-M virt \
-cpu cortex-a15 \
-m 256M \
- -bios /opt/homebrew/share/qemu/edk2-arm-code.fd \
+ -bios "${BIOS}" \📝 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.
Suggested change
| -bios /opt/homebrew/share/qemu/edk2-arm-code.fd \ | |
| # Find EDK2 BIOS - check common locations | |
| BIOS_PATHS=( | |
| "/opt/homebrew/share/qemu/edk2-arm-code.fd" # macOS ARM (Homebrew) | |
| "/usr/local/share/qemu/edk2-arm-code.fd" # macOS Intel (Homebrew) | |
| "/usr/share/qemu/edk2-arm-code.fd" # Linux | |
| "/usr/share/AAVMF/AAVMF_CODE.fd" # Debian/Ubuntu | |
| ) | |
| BIOS="" | |
| for path in "${BIOS_PATHS[@]}"; do | |
| if [ -f "$path" ]; then | |
| BIOS="$path" | |
| break | |
| fi | |
| done | |
| if [ -z "$BIOS" ]; then | |
| echo "EDK2 ARM BIOS not found. Please install QEMU with EFI support." | |
| exit 1 | |
| fi | |
| qemu-system-arm \ | |
| -M virt \ | |
| -cpu cortex-a15 \ | |
| -m 256M \ | |
| -bios "${BIOS}" \ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/start-vm-arm.sh` at line 25, The hardcoded BIOS path in
scripts/start-vm-arm.sh (the -bios /opt/homebrew/share/qemu/edk2-arm-code.fd
argument) is macOS ARM specific; change the script to accept a configurable BIOS
path (e.g., read BIOS_PATH or QEMU_BIOS env var) and fall back to auto-detection
based on the platform (uname) by checking common locations
(/opt/homebrew/share/qemu/, /usr/local/share/qemu/, /usr/share/qemu/,
/usr/share/AAVMF/) before invoking qemu-system-aarch64; update the script to use
that resolved BIOS path variable for the -bios argument and emit a clear error
if no valid file is found.
GamerClassN7
reviewed
Apr 2, 2026
| index-file.names = ( "index.html" ) | ||
| } | ||
|
|
||
| $HTTP["url"] == "/ubus" { |
There was a problem hiding this comment.
you probably need to secure it so it is not callable outside of lan right ?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/etc/lighttpd/conf.d/scripts/setup-qemu-arm.sh,scripts/start-vm-arm.sh)ARM QEMU Testing
Scripts are provided for ARM emulation testing, but note that ARM emulation on x86/ARM64 hosts is extremely slow (services may take 60+ seconds to respond). For practical testing:
Test plan
Closes #19
Summary by CodeRabbit
New Features
Tooling